EDM-2646: Revert to manually generating the PATCHes for applications#387
Conversation
WalkthroughThe patch generation in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant getApplicationPatches as getApplicationPatches()
participant Detectors as Change Detectors
participant Patches as Patch Array
Caller->>getApplicationPatches: call(currentApps, updatedApps)
alt currentApps empty
getApplicationPatches->>Patches: create add-all patches
else updatedApps empty
getApplicationPatches->>Patches: create remove-all patches
else lengths differ
getApplicationPatches->>Patches: replace entire applications array
else same length
loop for each index i
getApplicationPatches->>Detectors: hasApplicationChanged(current[i], updated[i])
Detectors->>Detectors: check inline/image/single-container, volumes, ports, resources, envs
Detectors-->>getApplicationPatches: changed? (true/false)
alt changed
getApplicationPatches->>Patches: add replace patch at index i
end
end
end
getApplicationPatches-->>Caller: return Patches
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (1)
392-404: Redundant environment variable check for single container apps.
hasApplicationChangedalready checksenvVarsat lines 422-433 before callinghasSingleContainerAppChanged, which performs the same check again at lines 392-404. Consider removing the duplicate check fromhasSingleContainerAppChangedto avoid redundant computation.Also applies to: 436-438
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration-tests
- GitHub Check: Build
- GitHub Check: Build ocp plugin
- GitHub Check: Lint
🔇 Additional comments (3)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (3)
297-313: LGTM!The inline application change detection logic correctly compares file counts, encoding, paths, and content using index-based comparison.
488-500: LGTM - Index-based granular patching.The approach correctly emits targeted
replacepatches only for applications that have actually changed. The index-based comparison works correctly because array length changes already trigger a full array replacement. For same-length arrays with reordering, individual replacements achieve the correct end state.
468-474: LGTM - Correctly handles adding first application(s).This addresses the PR objective of fixing the issue where adding the first application failed with the json-patch library.
celdrake
force-pushed
the
EDM-2646-fix-application-patching
branch
from
479bb2f to
1ab39bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (2)
libs/types/index.ts (8)
InlineApplicationProviderSpec(126-126)EncodingType(81-81)ApplicationVolume(16-16)ImageMountVolumeProviderSpec(122-122)ImagePullPolicy(123-123)ApplicationProviderSpec(11-11)ImageApplicationProviderSpec(121-121)PatchRequest(147-147)libs/ui-components/src/types/deviceSpec.ts (8)
QuadletInlineAppForm(76-81)ComposeInlineAppForm(89-94)ApplicationVolumeForm(143-149)PortMapping(56-59)AppForm(113-118)isSingleContainerAppForm(134-135)QuadletImageAppForm(70-74)ComposeImageAppForm(83-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration-tests
- GitHub Check: Build ocp plugin
- GitHub Check: Build
- GitHub Check: Lint
🔇 Additional comments (6)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (6)
297-313: LGTM!The inline file comparison logic correctly handles undefined
base64by defaulting tofalse.
315-348: LGTM!Volume comparison covers all relevant properties with sensible defaults for optional fields.
350-360: LGTM!Port comparison is straightforward with helpful documentation about ordering behavior.
362-372: LGTM!Resource limits comparison correctly handles undefined values by defaulting to empty strings.
374-387: Good fix for the falsy value issue.Using
variable.name in envVarscorrectly handles environment variables with falsy values like empty strings or0. This addresses the concerns from the past review.
454-499: LGTM!The refactored approach correctly handles all edge cases (adding first apps, removing all, length changes) and generates precise per-index patches when the array length is unchanged. This is a solid improvement over the previous library-based approach.
celdrake
force-pushed
the
EDM-2646-fix-application-patching
branch
from
1ab39bd to
7f0d550
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR #371 (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
📚 Learning: 2025-11-26T10:44:58.434Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 387
File: libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts:447-451
Timestamp: 2025-11-26T10:44:58.434Z
Learning: In the flightctl-ui codebase, the UI only allows users to modify volumes for Single Container applications. Volumes cannot be modified through the UI for Quadlet/Compose image applications or inline applications, even though the backend API supports volumes for these application types.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 Learning: 2025-06-04T06:41:15.969Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 308
File: libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts:204-204
Timestamp: 2025-06-04T06:41:15.969Z
Learning: In the flightctl-ui codebase, volume functionality is not yet implemented in the backend. TODO comments related to volume preservation (like in toAPIApplication function) are intentional placeholders for when backend volume support is added.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
📚 Learning: 2025-10-29T16:47:29.614Z
Learnt from: celdrake
Repo: flightctl/flightctl-ui PR: 371
File: libs/types/models/AppType.ts:10-10
Timestamp: 2025-10-29T16:47:29.614Z
Learning: PR #371 (flightctl/flightctl-ui) adds the AppTypeQuadlet enum member and related types as a preparatory change. Full implementation of quadlet application support in the UI will be added later, after backend support is available.
Applied to files:
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration-tests
- GitHub Check: Build ocp plugin
- GitHub Check: Build
- GitHub Check: Lint
🔇 Additional comments (6)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (6)
21-49: New application form imports are consistent and used.
ComposeImageAppForm,PortMapping, andQuadletImageAppFormare all used in the new helpers (arePortsEqual,hasApplicationChanged), and the import surface stays aligned with the existingAppForm/type‑guard model. No issues here.
315-348: Volume comparison logic matches form conversion semantics.
areVolumesEqualcompares name, image reference, pull policy (defaulting toPullIfNotPresent), and mount path, using the same defaults asconvertVolumesToForm. Order-sensitive comparison is reasonable here since the API models volumes as an ordered array. This helper looks sound.
350-372: Ports and resource limits equality checks behave as expected.
arePortsEqualnormalizes back to the"host:container"string and is intentionally order-sensitive.areResourceLimitsEqualnormalizes missing values to'', so{ cpu: undefined, memory: undefined }andundefinedare treated the same, while real values (including'0') are still distinguished.Both helpers are consistent with
getApplicationValues/toAPIApplicationand should avoid spurious changes.
374-387: Environment variable equality correctly handles falsy values and order.
areEnvVariablesEqualfixes the earlier falsy‑value bug by:
- Comparing counts (
Object.keys(envVars).length) against the form array length.- Requiring that each
variable.nameexists in the map and matches by strict equality.This ignores ordering differences (good) and no longer treats empty strings or other falsy values as implicit changes.
414-451: Overall application change detection is coherent; inline volumes intentionally ignored.The structure of
hasApplicationChangedcleanly separates cases:
- Early exit on specType/appType/name changes and env var changes.
- Delegation to
hasSingleContainerAppChangedfor single-container apps.- Direct image + volume comparison for Quadlet/Compose image apps.
- Fallback to inline file comparison for inline apps.
Per the recorded learnings, the UI only exposes volume editing for single-container applications, not for Quadlet/Compose or inline apps, so omitting a volume comparison in the inline branch is intentional and avoids chasing backend capabilities the UI doesn’t surface. Based on learnings, this split looks appropriate.
454-499: Manual application patch generation meets PR goals and avoids json-patch edge cases.The new
getApplicationPatcheslogic:
- Handles the “no apps → some apps” and “some apps → no apps” transitions with
add/removeat${basePath}/applications.- Replaces the entire array when lengths differ.
- When lengths are equal, only emits
replaceoperations for indices wherehasApplicationChangedreturnstrue.This should fix the previous issues with the third-party json-patch library (e.g., empty string resource limits and first-app additions) while also reducing unnecessary PATCHes by only touching genuinely changed applications. Behavior for reordering (same length, different ordering) is to emit per-index replaces, which is acceptable given the array-based API model.
2 participants
Attempting to PATCH the applications using "json patch" library didn't work well, it was failing to handle cases such as adding the first application or having empty resource limits (it would try to patch to an empty string which the API rejects)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.