Repository navigation
Fix isManagedByHelm to check the existing CRD, not the desired one - #27
Merged
gianlucam76 merged 1 commit intoSep 18, 2026
Merged
Conversation
gianlucam76
self-requested a review
September 16, 2026 13:39
Member
|
Thank you @magic-peach this is a good catch. Fix is correct. Can you please
Thank you again. Nice catch. |
gianlucam76
reviewed
Sep 17, 2026
gianlucam76
left a comment
gianlucam76
left a comment
Member
There was a problem hiding this comment.
great catch. Can you please:
- use gingko for the unit test so to be consistent with every other Sveltos repo.
- fix the lint issue
processCustomResourceDefinition fetches the live CustomResourceDefinition from the cluster into customResourceDefinition, then calls isManagedByHelm(u), where u is the freshly parsed desired CRD manifest from the embedded YAML. That manifest never carries the app.kubernetes.io/managed-by label (confirmed there are zero occurrences of that label across pkg/crds/crds.go), so isManagedByHelm always returned false and the Job force-updated every CRD on every run, including ones actually managed by Helm. Fixed by passing the fetched customResourceDefinition instead of u, and changed isManagedByHelm's signature to take a *apiextensionsv1.CustomResourceDefinition so the compiler enforces this going forward. Added cmd/main_test.go covering the Helm managed skip path, the normal update path, and the create-when-missing path using the controller-runtime fake client. Reverting the fix locally and rerunning the new test confirms it fails against the original bug. Signed-off-by: Akanksha Trehun <akankshatrehun@gmail.com>
magic-peach
force-pushed
the
fix/helm-managed-crd-check
branch
from
September 18, 2026 10:30
39d9411 to
17276d8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
processCustomResourceDefinition fetches the live CustomResourceDefinition from the cluster into customResourceDefinition, then calls isManagedByHelm(u), where u is the freshly parsed desired CRD manifest from the embedded YAML in pkg/crds/crds.go.
That embedded manifest never carries the app.kubernetes.io/managed-by label (confirmed zero occurrences of that label across pkg/crds/crds.go), so isManagedByHelm always returned false regardless of whether the live CRD in the cluster was actually Helm managed. As a result the Job force updated every CRD on every run, including ones that should have been left alone.
Fixed by passing the fetched customResourceDefinition instead of u, and changed isManagedByHelm's signature to take a *apiextensionsv1.CustomResourceDefinition so the compiler enforces this going forward.
Added cmd/main_test.go with three tests using the controller-runtime fake client:
Reverting the fix locally and rerunning the new test confirms it fails against the original code and passes against the fix.