refactor: update namespace sync configuration to use label selector #1408
+82
−111
Conversation
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
- Changed `targetNamespaceLabelKey` to `targetNamespaceLabelSelector` in `values.yaml` for clarity. - Updated deployment template to reflect the new label selector usage. - Modified main application logic to validate and parse the label selector. - Removed deprecated `IsTargetNamespace` function in favor of a more flexible label selector approach. - Adjusted related tests and documentation accordingly.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the namespace sync configuration to use a more flexible label selector approach instead of a simple label key check. The change allows for more sophisticated namespace targeting using Kubernetes label selectors (e.g., environment=production or team in (platform,infrastructure)). Additionally, it adds context cancellation checks in loops to prevent potential goroutine leaks when operations are cancelled.
Key changes:
- Replaced
targetNamespaceLabelKeywithtargetNamespaceLabelSelectorthroughout the codebase - Added context cancellation checks in
walkReferencesandReconcileloops - Removed the deprecated
NamespaceHasLabelKeyfunction and its tests - Updated configuration files and command-line flags to support the new label selector format
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/controllers/namespacesync/suite_test.go | Updated test setup to parse and use label selector instead of label key |
| pkg/controllers/namespacesync/references.go | Added context cancellation check in loop to prevent goroutine leaks |
| pkg/controllers/namespacesync/label_test.go | Removed tests for deprecated NamespaceHasLabelKey function |
| pkg/controllers/namespacesync/label.go | Removed deprecated NamespaceHasLabelKey function |
| pkg/controllers/namespacesync/flags.go | Added new label selector flag and deprecated old label key flag |
| pkg/controllers/namespacesync/copy.go | Removed commented-out code for zeroing ManagedFields and OwnerReferences |
| pkg/controllers/namespacesync/controller.go | Refactored to use label selector with pre-allocated slices and added context checks |
| cmd/main.go | Added label selector parsing and validation logic |
| charts/cluster-api-runtime-extensions-nutanix/values.yaml | Updated configuration key from targetNamespaceLabelKey to targetNamespaceLabelSelector |
| charts/cluster-api-runtime-extensions-nutanix/values.schema.json | Updated schema to reflect new configuration key |
| charts/cluster-api-runtime-extensions-nutanix/templates/deployment.yaml | Updated deployment template to use new label selector flag |
| charts/cluster-api-runtime-extensions-nutanix/README.md | Updated documentation to reflect new configuration parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dkoshkin
reviewed
Nov 24, 2025
| | namespaceSync.enabled | bool | `true` | | | ||
| | namespaceSync.sourceNamespace | string | `""` | | | ||
| | namespaceSync.targetNamespaceLabelKey | string | `"caren.nutanix.com/namespace-sync"` | | | ||
| | namespaceSync.targetNamespaceLabelSelector | string | `"caren.nutanix.com/namespace-sync"` | | |
Contributor
There was a problem hiding this comment.
This example needs to change?
Member
Author
There was a problem hiding this comment.
That's actually the default and it's still fine, it's a label expression which means label exists, matching the existing behaviour.
dlipovetsky
reviewed
Nov 25, 2025
jimmidyson
force-pushed
the
jimmi/namespacesync-refactor
branch
from
cff17a1 to
468bb51
Compare
dlipovetsky
approved these changes
Nov 25, 2025
Contributor
dlipovetsky
left a comment
dlipovetsky
left a comment
There was a problem hiding this comment.
Nice improvement. Thanks!
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.
What problem does this PR solve?:
Refactoring to simplify the label selector logic. This allows for preallocation of slices.
targetNamespaceLabelKeytotargetNamespaceLabelSelectorinvalues.yamlfor clarity.IsTargetNamespacefunction in favor of a more flexible label selector approach.Also correctly handle context timeouts in copy functions in order to ensure there are no goroutine leaks.
Which issue(s) this PR fixes:
Fixes #
How Has This Been Tested?:
Special notes for your reviewer: