-
Notifications
You must be signed in to change notification settings - Fork 50
feat: native ignore-mutation support in applier #1883
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: native ignore-mutation support in applier #1883
Conversation
|
Skipping CI for Draft Pull Request. |
|
/test all |
401f124 to
d0620d0
Compare
|
/test all |
d0620d0 to
e4f013c
Compare
|
/test all |
e4f013c to
2315e88
Compare
|
/test all |
2496c7c to
00faa72
Compare
|
/test all |
00faa72 to
6751ae4
Compare
6751ae4 to
5658e2f
Compare
5658e2f to
462ccba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR reimplements the ignore-mutation feature to use native support in the cli-utils applier, eliminating the workaround of caching objects in Config Sync and passing cached objects to the applier. This change resolves several race conditions and side effects associated with the previous caching solution.
- Adds native
IgnoreMutationfield toCachedStatusfor tracking mutation-ignored objects - Removes the
mutationIgnoredObjectsMapcache and related caching logic from the Resources struct - Updates status computation to handle ignore-mutation objects using applier's native skip events
Reviewed Changes
Copilot reviewed 16 out of 45 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/resourcegroup/controllers/status/status.go | Adds isIgnoreMutationObject function to detect and track mutation-ignored objects in status |
| pkg/resourcegroup/controllers/resourcemap/resourcemap.go | Adds IgnoreMutation boolean field to CachedStatus struct |
| pkg/resourcegroup/controllers/resourcegroup/resourcegroup_controller_test.go | Adds comprehensive test cases for ignore-mutation status handling with different kstatus values |
| pkg/resourcegroup/controllers/resourcegroup/resourcegroup_controller.go | Updates status computation to handle ignore-mutation objects and compute reconcile status appropriately |
| pkg/remediator/watch/filteredwatcher_test.go | Removes test code for obsolete ignore-mutation cache |
| pkg/remediator/watch/filteredwatcher.go | Removes code that updated ignored objects in the Resources cache |
| pkg/metadata/metadata_test.go | Adds test validation for the return value of UpdateConfigSyncMetadata |
| pkg/metadata/metadata.go | Changes UpdateConfigSyncMetadata to return boolean indicating if updates were made |
| pkg/declared/resources_test.go | Removes all tests for obsolete ignore-mutation cache methods |
| pkg/declared/resources.go | Removes mutationIgnoredObjectsMap and all related cache management methods |
| pkg/applier/utils_test.go | Removes tests for obsolete handleIgnoredObjects function |
| pkg/applier/utils.go | Removes handleIgnoredObjects function that merged cached state with declared objects |
| pkg/applier/applier_test.go | Removes tests for mutation-ignored object caching behavior and updates remaining tests |
| pkg/applier/applier.go | Removes caching logic, adds metadata update handling for skip events, removes ignore cache cleanup from event processors |
| go.mod | Updates cli-utils and gomega dependencies to newer versions |
| e2e/testcases/ignore_mutation_test.go | Adds explicit ResourceGroup status verification for ignore-mutation objects |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request focuses on enhancing the Config Sync's applier to natively support the ignore-mutation feature, eliminating the previous workaround that involved caching objects. The changes include updating dependencies, modifying the applier's logic to handle skipped actuations due to the ignore-mutation annotation, and adjusting tests to reflect the new implementation. The most significant changes are in pkg/applier/applier.go and e2e/testcases/ignore_mutation_test.go, where the core logic for applying resources and testing the ignore-mutation feature has been updated.
This change reimplements the ignore-mutation feature using the native support in the cli-utils applier. This removes the need for the workaround of caching the object in Config Sync and passing in the cached object to the applier. This fixes several race conditions and side effects associated with the caching solution.
462ccba to
4961cc5
Compare
|
/lgtm |
|
Nit: suggest linking the change from cli-utils in commit message. Otherwise lgtm. |
Added a link in the PR description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tiffanny29631 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1b13cb4
into
GoogleContainerTools:main
This change reimplements the ignore-mutation feature using the native
support in the cli-utils applier. This removes the need for the
workaround of caching the object in Config Sync and passing in the
cached object to the applier.
This fixes several race conditions and side effects associated with the
caching solution.
Depends on new functionality in cli-utils: kubernetes-sigs/cli-utils@5895ad6
Fixes: #1668