-
Notifications
You must be signed in to change notification settings - Fork 4
Single Namespace restored #498
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
base: release/1.1.z
Are you sure you want to change the base?
Single Namespace restored #498
Conversation
Reviewer's GuideConfigure the controller-runtime manager to optionally watch a single namespace via a WATCH_NAMESPACE environment variable, and wire that variable into both the controller deployment and CSV manifests. Sequence diagram for manager startup with optional single-namespace cachesequenceDiagram
participant Kubelet
participant OperatorPod
participant Main as main_go
participant Manager as ControllerRuntimeManager
participant KubeAPI as KubernetesAPI
Kubelet->>OperatorPod: Start pod with env WATCH_NAMESPACE (from metadata.namespace)
OperatorPod->>Main: Execute main()
Main->>Main: Read WATCH_NAMESPACE from environment
alt WATCH_NAMESPACE is set
Main->>Main: Build cache.Options with DefaultNamespaces[WATCH_NAMESPACE]
Main->>Manager: Create NewManager(config, Options{Cache: cache.Options})
Manager->>KubeAPI: List/watch resources in WATCH_NAMESPACE only
else WATCH_NAMESPACE is empty
Main->>Main: Use default cache.Options (all namespaces)
Main->>Manager: Create NewManager(config, Options{Cache: default})
Manager->>KubeAPI: List/watch resources in all namespaces
end
Manager-->>Main: Manager initialized
Main->>Manager: Start()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- Instead of using a package-level
watchNamespacevariable initialized viaos.Getenv, consider reading the environment variable insidemain()to avoid global state and make the configuration behavior easier to test and reason about. - Since this PR only supports a single namespace, using
ctrl.Options{Namespace: watchNamespace}may be clearer and more idiomatic than configuringcache.Options{DefaultNamespaces: map[string]cache.Config{...}}for a single entry.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of using a package-level `watchNamespace` variable initialized via `os.Getenv`, consider reading the environment variable inside `main()` to avoid global state and make the configuration behavior easier to test and reason about.
- Since this PR only supports a single namespace, using `ctrl.Options{Namespace: watchNamespace}` may be clearer and more idiomatic than configuring `cache.Options{DefaultNamespaces: map[string]cache.Config{...}}` for a single entry.
## Individual Comments
### Comment 1
<location> `main.go:83` </location>
<code_context>
+ cacheOpts = cache.Options{
</code_context>
<issue_to_address>
**issue (bug_risk):** Using DefaultNamespaces for a single namespace may change cluster-scoped resource behavior compared to the default cache.
`DefaultNamespaces` is designed for multi-namespace caches and changes how cluster-scoped resources are handled, which can differ from the default single-namespace/cluster-wide cache. If this operator uses cluster-scoped resources (CRDs, ClusterRoles, etc.), this may introduce subtle behavior differences. For a single-namespace watch, prefer setting `Namespace: watchNamespace` in `cache.Options` to preserve default cache behavior while restricting namespaced objects.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if watchNamespace != "" { | ||
| setupLog.Info("Watching specific namespace", "namespace", watchNamespace) | ||
| cacheOpts = cache.Options{ | ||
| DefaultNamespaces: map[string]cache.Config{ |
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.
issue (bug_risk): Using DefaultNamespaces for a single namespace may change cluster-scoped resource behavior compared to the default cache.
DefaultNamespaces is designed for multi-namespace caches and changes how cluster-scoped resources are handled, which can differ from the default single-namespace/cluster-wide cache. If this operator uses cluster-scoped resources (CRDs, ClusterRoles, etc.), this may introduce subtle behavior differences. For a single-namespace watch, prefer setting Namespace: watchNamespace in cache.Options to preserve default cache behavior while restricting namespaced objects.
Signed-off-by: desmax74 <[email protected]>
7187cd0 to
06542b9
Compare
Summary by Sourcery
Configure the operator to watch either a single namespace or all namespaces based on an environment variable.
New Features:
Enhancements: