Repository navigation
fix(platform): make *-rd sources depend on cozystack-engine - #3041
Conversation
Every *-application PackageSource that ships a system/*-rd component renders an ApplicationDefinition (cozystack.io/v1alpha1). That CRD is installed by the application-definition-crd component of cozystack-engine. Without an explicit dependsOn edge, a fresh install where Flux reconciles a *-application source before cozystack-engine fails to render with `no matches for kind "ApplicationDefinition" in version "cozystack.io/v1alpha1"`, which cascades into a broad install timeout unrelated to whatever change triggered the reconcile. Add `- cozystack.cozystack-engine` to the dependsOn of every source that ships a system/*-rd component; gateway-application already carried the edge for the TenantGateway CRD. Add a generic invariant test that fails if any such source is missing the edge, so future sources cannot reintroduce the race. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
github-actions
Bot
added
area/platform
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (32)
📝 WalkthroughWalkthroughPackageSource manifests now add ChangesEngine dependency ordering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…election Every *-application source declares dependsOn cozystack.cozystack-engine so its *-rd HelmRelease waits for the ApplicationDefinition CRD to register before it reconciles. That is a universal install-ordering edge, not a behavioral one. The test-impact selector reused the same dependsOn graph to pick which e2e app suites to run for a diff. Because the engine transitively depends on postgres-operator, keycloak and cert-manager, a change to any of those fanned out through the engine to every app, defeating test-impact analysis. Exclude cozystack.cozystack-engine as a propagation hub in the reverse-dep walk so the engine stays reachable but no longer pushes selection to its app-dependents. A change to the engine itself still triggers the full suite via the existing no-app-descendants safety net. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
dosubot
Bot
added
the
area/testing
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a race condition during cluster installation where Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request ensures that all application PackageSources containing a system/*-rd component explicitly declare a dependency on cozystack.cozystack-engine to guarantee the ApplicationDefinition CRD is installed prior to reconciliation. A new BATS test is added to enforce this invariant. To prevent this ordering dependency from triggering E2E tests for all applications, hack/select-e2e.sh is updated to exclude cozystack.cozystack-engine from test-impact propagation, backed by a new test. The reviewer feedback suggests using optional chaining (?.) in the jq queries of the new BATS test to handle potentially malformed PackageSource files more robustly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| rd_total="$(printf '%s' "$json" | jq -s ' | ||
| [ .[] | ||
| | select(.kind == "PackageSource") | ||
| | (.spec.variants // [])[] | ||
| | select((.components // []) | any(.path // "" | test("^system/[a-z0-9-]+-rd$"))) | ||
| ] | length | ||
| ')" |
There was a problem hiding this comment.
To make the test more robust and prevent potential jq execution failures if a malformed or incomplete PackageSource file is present in the directory, consider using optional chaining (?.) for accessing .spec.variants. This aligns with defensive programming best practices.
rd_total="$(printf '%s' "$json" | jq -s '
[ .[]
| select(.kind == "PackageSource")
| (.spec?.variants // [])[]
| select((.components // []) | any(.path // "" | test("^system/[a-z0-9-]+-rd$")))
] | length
')"
| offenders="$(printf '%s' "$json" | jq -rs ' | ||
| [ .[] | ||
| | select(.kind == "PackageSource") | ||
| | .metadata.name as $name | ||
| | (.spec.variants // [])[] | ||
| | select((.components // []) | any(.path // "" | test("^system/[a-z0-9-]+-rd$"))) | ||
| | select(((.dependsOn // []) | any(. == "cozystack.cozystack-engine")) | not) | ||
| | "\($name) [variant: \(.name)]" | ||
| ] | .[] | ||
| ')" |
There was a problem hiding this comment.
Similarly, use optional chaining (?.) when extracting the metadata name and variants to ensure the test does not crash on malformed files missing these fields.
offenders="$(printf '%s' "$json" | jq -rs '
[ .[]
| select(.kind == "PackageSource")
| .metadata?.name as $name
| (.spec?.variants // [])[]
| select((.components // []) | any(.path // "" | test("^system/[a-z0-9-]+-rd$")))
| select(((.dependsOn // []) | any(. == "cozystack.cozystack-engine")) | not)
| "\\($name) [variant: \\(.name?)]"
] | .[]
')"
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Reviewed the diff. Edge correctly added to 29 *-application sources, no cycle (engine depends only on networking/cert-manager, and on keycloak/keycloak-operator in the oidc variant; none of those route back to *-application). bats invariant prevents future drift. Excluding engine from the propagation hub in test-impact analysis is correct (this is a lifecycle edge, not a behavioral one) and the new "engine-dependency change does not fan out" test pins that. LGTM.
2 participants
Problem
On an unlucky Flux reconcile order, a
*-applicationPackageSource that ships asystem/*-rdcomponent can install before theapplicationdefinitions.cozystack.ioCRD exists. Every*-rdchart renders anApplicationDefinition(cozystack.io/v1alpha1); that CRD is installed by theapplication-definition-crdcomponent ofcozystack.cozystack-engine. When an*-rdHelmRelease renders first, it hard-fails withno matches for kind "ApplicationDefinition" in version "cozystack.io/v1alpha1", and that cascades into a broad install timeout — surfacing even on changes that have nothing to do with ordering, because it is a latent race.Fix
Add
- cozystack.cozystack-engineto thedependsOnof every*-applicationsource that ships asystem/*-rdcomponent (29 sources).gateway-applicationalready carried this edge — it was added earlier for the TenantGateway CRD, the same bug class, but only for that one source. A new invariant test fails if any source with asystem/*-rdcomponent is missing the edge.Why
The ordering was previously implicit, relying on Flux happening to reconcile
cozystack-enginebefore the*-applicationsources. Making the edge explicit removes the race deterministically. No dependency cycle is introduced:cozystack-enginedepends only onnetworkingandcert-manager(plus keycloak in its oidc variant), none of which reach back to any*-application.Test
hack/check-applicationdefinition-crd-ordering.bats(auto-discovered bymake bats-unit-tests, runs in CI viamake unit-tests): parses every source and fails if any PackageSource variant with asystem/*-rdcomponent lacks thecozystack.cozystack-engineedge — so future sources are covered, not just today's set.helm unittest packages/core/platform: 13 suites / 66 tests pass; the source edits do not disturb existing assertions.Summary by CodeRabbit
Bug Fixes
Tests