feat(deploy): support single namespace #1091
Conversation
|
Warning Rate limit exceeded@Rory-Z has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (10)
WalkthroughThe pull request introduces a new configuration option Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Rory-Z
force-pushed
the
feat/support-single-namespace
branch
from
87c6a14 to
653755b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (3)
deploy/charts/emqx-operator/README.md (1)
37-37: LGTM! Consider enhancing documentation.The parameter is well-documented and consistent with the implementation. Consider adding:
- Security implications of using single namespace mode
- Common use cases for when to use single vs. all namespaces
- Impact on existing deployments when switching modes
main.go (1)
183-195: Consider adding validation for namespace valueWhile the implementation is correct, consider validating the namespace value when it's non-empty to ensure it's a valid Kubernetes namespace name.
func getWatchNamespace() string { var watchNamespaceEnvVar = "WATCH_NAMESPACE" ns, found := os.LookupEnv(watchNamespaceEnvVar) - if !found { + if !found || ns == "" { return metav1.NamespaceAll } + // Optional: Validate namespace name + if err := validateNamespaceName(ns); err != nil { + setupLog.Error(err, "invalid namespace name, falling back to all namespaces") + return metav1.NamespaceAll + } return ns } + +func validateNamespaceName(ns string) error { + if len(ns) > 253 || !metav1.NameRegex.MatchString(ns) { + return fmt.Errorf("invalid namespace name: %s", ns) + } + return nil +}.github/workflows/deploy.yaml (1)
35-39: Add comments to explain matrix configurationConsider adding comments to explain the purpose of the single_namespace matrix parameter and why we test both configurations.
single_namespace: - false include: + # Test single namespace deployment mode - install: helm single_namespace: true
🛑 Comments failed to post (1)
deploy/charts/emqx-operator/templates/controller-manager-rbac.yaml (1)
19-28:
⚠️ Potential issueFix inconsistent roleRef kind
The roleRef still points to ClusterRole even when using RoleBinding in single namespace mode. This needs to be adjusted based on the singleNamespace value.
roleRef: apiGroup: rbac.authorization.k8s.io - kind: ClusterRole + kind: {{ if .Values.singleNamespace }}Role{{ else }}ClusterRole{{ end }} name: {{ include "emqx-operator.fullname" . }}-manager-roleCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
main.go (1)
108-110: Consider adding validation for the watch namespace valueWhile the cache configuration is correct, consider adding validation for the namespace value to ensure it's a valid Kubernetes namespace name when not set to watch all namespaces.
Cache: cache.Options{ - DefaultNamespaces: map[string]cache.Config{getWatchNamespace(): {}}, + DefaultNamespaces: map[string]cache.Config{validateWatchNamespace(getWatchNamespace()): {}}, },Add this helper function:
func validateWatchNamespace(ns string) string { if ns == metav1.NamespaceAll { return ns } // Basic validation for namespace name if len(ns) > 253 || !regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`).MatchString(ns) { setupLog.Error(nil, "invalid namespace name, falling back to all namespaces", "namespace", ns) return metav1.NamespaceAll } return ns }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/deploy.yaml(2 hunks)deploy/charts/emqx-operator/README.md(1 hunks)deploy/charts/emqx-operator/templates/controller-manager-rbac.yaml(2 hunks)deploy/charts/emqx-operator/templates/controller-manager.yaml(1 hunks)deploy/charts/emqx-operator/values.yaml(1 hunks)main.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- deploy/charts/emqx-operator/README.md
- deploy/charts/emqx-operator/values.yaml
- deploy/charts/emqx-operator/templates/controller-manager.yaml
- deploy/charts/emqx-operator/templates/controller-manager-rbac.yaml
🔇 Additional comments (5)
.github/workflows/deploy.yaml (2)
35-43: LGTM: Matrix configuration properly handles single namespace testing
The matrix configuration correctly includes both default (all namespaces) and single namespace test scenarios, ensuring comprehensive testing of the new feature.
75-75: LGTM: Helm parameter correctly configured
The singleNamespace parameter is properly passed to the helm installation command.
main.go (3)
31-35: LGTM: Required imports properly added
The new imports for cache and metav1 packages are correctly added and necessary for the namespace watching functionality.
183-195: LGTM: Well-implemented namespace configuration
The getWatchNamespace function is well-documented and correctly implements the namespace configuration logic:
- Properly handles environment variable lookup
- Returns
metav1.NamespaceAllas a safe default - Documentation clearly explains the behavior
However, let's verify the usage of this environment variable across the codebase:
✅ Verification successful
Let me analyze the usage of WATCH_NAMESPACE specifically to verify its consistency across the codebase.
Based on the verification results, I can now provide the final response:
LGTM: Namespace configuration is properly implemented and consistently used
The WATCH_NAMESPACE environment variable is:
- Correctly implemented in
main.gowith proper fallback to cluster-wide scope - Consistently configured in the Helm chart's controller manager deployment when
singleNamespaceis enabled - Not conflicting with any other namespace-related environment variables
- Properly integrated with the operator's deployment configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any hardcoded namespace references or other namespace-related configurations
# Check for namespace-related environment variables
rg -l "WATCH_NAMESPACE|namespace" --type yaml
# Check for potential namespace-related configurations in Helm charts
fd . deploy/charts -e yaml -x cat {} \; | grep -i "namespace:"
# Look for potential namespace-related code
ast-grep --pattern 'namespace'
Length of output: 12241
Script:
#!/bin/bash
# Check for WATCH_NAMESPACE environment variable usage
rg "WATCH_NAMESPACE" -B 2 -A 2
# Check manager deployment configuration for namespace-related env vars
cat config/manager/manager.yaml
# Look for other namespace-related environment variables that might conflict
rg "env:.*namespace" -i
Length of output: 2721
Line range hint 67-77: Verify RBAC permissions for single namespace mode
The RBAC annotations define cluster-wide permissions, but when running in single namespace mode, these should be scoped to the specific namespace. Ensure that the Helm charts create appropriate Role/RoleBinding instead of ClusterRole/ClusterRoleBinding when singleNamespace is true.
✅ Verification successful
Let me gather more information to verify the conditional logic for single namespace mode.
RBAC permissions are correctly configured for single namespace mode
The Helm charts already implement the proper RBAC scoping based on the singleNamespace value:
- When
singleNamespace: true, it creates Role and RoleBinding scoped to the deployment namespace - When
singleNamespace: false, it creates ClusterRole and ClusterRoleBinding for cluster-wide access - The conditional logic is properly implemented in
controller-manager-rbac.yaml - The operator's watch namespace is also correctly set via the
WATCH_NAMESPACEenvironment variable in the deployment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check RBAC configurations in Helm charts
# Check for Role/ClusterRole definitions
fd . deploy/charts -e yaml -x cat {} \; | grep -A 5 "kind: \(Cluster\)\?Role"
# Check for RoleBinding/ClusterRoleBinding definitions
fd . deploy/charts -e yaml -e tpl -x cat {} \; | grep -A 5 "kind: \(Cluster\)\?RoleBinding"
Length of output: 2017
Script:
#!/bin/bash
# Check for singleNamespace conditional logic in Helm templates
fd . deploy/charts -e yaml -e tpl -x grep -l "singleNamespace" {} \; -x cat {} \;
# Also check the values file for singleNamespace configuration
fd values.yaml deploy/charts -x cat {} \;
Length of output: 11303
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1091 +/- ##
=======================================
Coverage 76.12% 76.12%
=======================================
Files 69 69
Lines 6250 6250
=======================================
Hits 4758 4758
Misses 1251 1251
Partials 241 241 ☔ View full report in Codecov by Sentry. |
Rory-Z
force-pushed
the
feat/support-single-namespace
branch
3 times, most recently
from
41e6ac7 to
0a55fbb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
.github/workflows/deploy.yaml (1)
Line range hint
91-101: Update failure handling steps for single namespace supportThe failure handling steps need to be updated to look in the correct namespace when
single_namespace=true.Apply this diff to fix the issue:
- if: failure() - run: kubectl logs -l "control-plane=controller-manager" -n emqx-operator-system -c manager --tail=1000 + run: kubectl logs -l "control-plane=controller-manager" -n ${{ matrix.single_namespace && 'default' || 'emqx-operator-system' }} -c manager --tail=1000
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/deploy.yaml(2 hunks)Dockerfile(1 hunks)deploy/charts/emqx-operator/README.md(1 hunks)deploy/charts/emqx-operator/templates/controller-manager-rbac.yaml(1 hunks)deploy/charts/emqx-operator/templates/controller-manager.yaml(1 hunks)deploy/charts/emqx-operator/values.yaml(1 hunks)main.go(3 hunks)sidecar/reloader/Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- Dockerfile
- deploy/charts/emqx-operator/templates/controller-manager.yaml
- deploy/charts/emqx-operator/README.md
- deploy/charts/emqx-operator/values.yaml
- main.go
- sidecar/reloader/Dockerfile
- deploy/charts/emqx-operator/templates/controller-manager-rbac.yaml
🔇 Additional comments (1)
.github/workflows/deploy.yaml (1)
35-43: Consider extending single namespace testing to all EMQX versions
The single namespace testing is currently limited to v2beta1 versions. Consider adding test cases for other versions (v1beta3, v1beta4, v2alpha1) to ensure the feature works consistently across all supported EMQX versions.
Let's verify which versions support single namespace:
Rory-Z
force-pushed
the
feat/support-single-namespace
branch
from
0a55fbb to
afe9128
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
main.go (1)
108-110: Add logging to indicate namespace operation modeWhile the implementation is correct, it would be helpful to log whether the operator is running in single-namespace or all-namespaces mode for operational clarity.
Add logging before the manager starts:
Cache: cache.Options{ DefaultNamespaces: getWatchNamespace(), }, + // Add after cache configuration + setupLog.Info("namespace mode", "watchNamespace", func() string { + if ns := os.Getenv("WATCH_NAMESPACE"); ns != "" { + return ns + } + return "all namespaces" + }())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/deploy.yaml(2 hunks)Dockerfile(1 hunks)deploy/charts/emqx-operator/README.md(1 hunks)deploy/charts/emqx-operator/templates/controller-manager-rbac.yaml(1 hunks)deploy/charts/emqx-operator/templates/controller-manager.yaml(1 hunks)deploy/charts/emqx-operator/values.yaml(1 hunks)main.go(3 hunks)sidecar/reloader/Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- deploy/charts/emqx-operator/README.md
- deploy/charts/emqx-operator/values.yaml
- Dockerfile
- sidecar/reloader/Dockerfile
- deploy/charts/emqx-operator/templates/controller-manager.yaml
- deploy/charts/emqx-operator/templates/controller-manager-rbac.yaml
- .github/workflows/deploy.yaml
🔇 Additional comments (2)
main.go (2)
37-37: LGTM: Import statements are correctly added
The new imports for cache and metav1 packages are necessary for implementing the namespace-scoped operation capability.
Also applies to: 41-42
Line range hint 67-77: Verify RBAC permissions for namespace-scoped operation
The current RBAC annotations look correct for the operator's functionality. However, let's verify that they work correctly in single-namespace mode.
✅ Verification successful
RBAC permissions are correctly configured for both single-namespace and cluster-wide operations
The RBAC permissions are properly set up to work in both single-namespace and cluster-wide modes:
- The operator supports single-namespace operation through the
WATCH_NAMESPACEenvironment variable - When
WATCH_NAMESPACEis set, the operator watches only the specified namespace - When
WATCH_NAMESPACEis not set, it defaults to watching all namespaces - The RBAC annotations in the code provide the necessary permissions for both modes of operation
The current RBAC setup follows Kubernetes best practices for operator namespace scoping.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if RBAC annotations include necessary namespace-scoped permissions
# Look for namespace-related RBAC rules
rg --type go "//\+kubebuilder:rbac.*namespaces.*" .
# Check if there are any direct namespace API calls that might need additional permissions
ast-grep --pattern 'client.$$$Namespace$$$' .
Length of output: 101
Script:
#!/bin/bash
# Let's verify the namespace-related operations more comprehensively
# Check for any namespace-related code patterns
rg -t go "namespace" -B 2 -A 2
# Look for controller configuration related to namespaces
ast-grep --pattern 'NewManager($$$options$$$)'
# Check for namespace-related reconciliation logic
ast-grep --pattern 'Reconcile($$$ctx$$$, $$$req$$$)'
# Look for namespace-related configuration or setup
rg -t go "WithNamespace|AllNamespaces" -B 2 -A 2
Length of output: 10541
Script:
#!/bin/bash
# Let's check for manager setup and watch namespace configurations
# Check for manager setup in main.go
rg -t go "mgr, err := ctrl.NewManager" -B 5 -A 10 main.go
# Look for watch namespace configurations
rg -t go "WatchNamespace" -B 2 -A 2
# Check for any namespace-related flags or environment variables
rg -t go "(flag.String|os.Getenv).*namespace" -B 2 -A 2
# Look for any namespace-related controller configurations
rg -t go "controller.Options|controller.Config" -B 2 -A 2
Length of output: 1133
Script:
#!/bin/bash
# Let's check the watch namespace implementation
# Check the complete getWatchNamespace function
rg -t go -A 15 "func getWatchNamespace" main.go
# Look for any environment variable usage related to namespaces
rg -t go "WATCH_NAMESPACE" -B 2 -A 5
# Check for any namespace-related cache configurations
rg -t go "cache.Config" -B 2 -A 5
Length of output: 1255
Rory-Z
force-pushed
the
feat/support-single-namespace
branch
2 times, most recently
from
0cb82b5 to
c7a1f9b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
.github/workflows/deploy.yaml (1)
Line range hint
97-97: Update failure handling for single namespace supportThe failure handling command still uses the hardcoded namespace
emqx-operator-system. This should be updated to use the conditional namespace like other commands.Apply this diff to fix the issue:
- - if: failure() - run: kubectl logs -l "control-plane=controller-manager" -n emqx-operator-system -c manager --tail=1000 + - if: failure() + run: kubectl logs -l "control-plane=controller-manager" -n ${{ matrix.single_namespace && 'default' || 'emqx-operator-system' }} -c manager --tail=1000
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/deploy.yaml(2 hunks)Dockerfile(1 hunks)deploy/charts/emqx-operator/README.md(1 hunks)deploy/charts/emqx-operator/templates/controller-manager-rbac.yaml(1 hunks)deploy/charts/emqx-operator/templates/controller-manager.yaml(1 hunks)deploy/charts/emqx-operator/values.yaml(1 hunks)main.go(3 hunks)sidecar/reloader/Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- deploy/charts/emqx-operator/templates/controller-manager.yaml
- deploy/charts/emqx-operator/values.yaml
- main.go
- Dockerfile
- sidecar/reloader/Dockerfile
- deploy/charts/emqx-operator/README.md
- deploy/charts/emqx-operator/templates/controller-manager-rbac.yaml
🔇 Additional comments (2)
.github/workflows/deploy.yaml (2)
75-76: LGTM! Helm deployment changes look good
The conditional namespace selection and singleNamespace parameter are properly implemented.
80-83: Previous issue has been properly addressed
The operator readiness check now correctly uses the conditional namespace selection, fixing the issue raised in the previous review.
.github/workflows/deploy.yaml
Outdated
| single_namespace: | ||
| - false | ||
| include: | ||
| - install: helm | ||
| emqx: [EMQX, emqx, "config/samples/emqx/v2beta1/emqx-slim.yaml"] | ||
| single_namespace: true | ||
| - install: helm | ||
| emqx: [EMQX, emqx, "config/samples/emqx/v2beta1/emqx-full.yaml"] | ||
| single_namespace: true |
There was a problem hiding this comment.
💡 Codebase verification
Single namespace testing should be extended to v1beta3 and v1beta4 versions
The verification shows that all EMQX CRDs (EmqxBroker, EmqxEnterprise, EMQX) are namespace-scoped resources, and there are active sample configurations for v1beta3 and v1beta4 versions in the codebase. Since the single namespace feature is relevant for all API versions, the test matrix should include these versions to ensure comprehensive coverage.
- Add matrix entries for v1beta3 and v1beta4 samples with
single_namespace: true:config/samples/emqx/v1beta4/emqxbroker-slim.yamlconfig/samples/emqx/v1beta4/emqxenterprise-slim.yamlconfig/samples/emqx/v1beta3/emqxbroker-slim.yamlconfig/samples/emqx/v1beta3/emqxenterprise-slim.yaml
🔗 Analysis chain
Consider expanding single namespace testing coverage
The single namespace testing is currently limited to v2beta1 versions. Consider adding test cases for other versions (v2alpha1, v1beta4, v1beta3) to ensure the feature works consistently across all supported versions.
Let me verify if the single namespace feature is applicable to older API versions by checking the CRD definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check which EMQX versions are currently supported in the codebase
rg -g "*.yaml" "kind:\s*(EMQX|EmqxBroker|EmqxEnterprise)" config/samples/emqx/
Length of output: 858
Script:
#!/bin/bash
# Check CRD definitions for namespace-scoped vs cluster-scoped settings
rg -g "*.yaml" "scope:" config/crd/bases/
Length of output: 374
Rory-Z
force-pushed
the
feat/support-single-namespace
branch
2 times, most recently
from
589747f to
abe5ac5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
.github/workflows/deploy.yaml (1)
Line range hint
94-94: Update debug steps for single namespace supportThe error handling steps still reference the fixed namespace
emqx-operator-system. These should be updated to use the conditional namespace like the operator readiness check:- run: kubectl logs -l "control-plane=controller-manager" -n emqx-operator-system -c manager --tail=1000 + run: kubectl logs -l "control-plane=controller-manager" -n ${{ matrix.single_namespace && 'default' || 'emqx-operator-system' }} -c manager --tail=1000Also applies to: 96-96
🧹 Nitpick comments (3)
RELEASE.md (1)
18-18: Enhancement description needs more detailsThe single namespace feature description could be more comprehensive. Consider adding:
- How to enable this feature (e.g., Helm chart configuration)
- Impact on existing deployments
- Migration considerations for users upgrading from previous versions
deploy/charts/emqx-operator/templates/controller-manager-rbac.yaml (2)
43-52: Document the namespace scoping behavior in values.yaml.The Role/ClusterRole implementation is correct. Consider adding documentation in
values.yamlto explain the security implications ofsingleNamespace.Add this to
values.yaml:+# -- Configure the operator to watch a single namespace or all namespaces +# When true, the operator will only watch and manage resources in its own namespace +# When false, the operator will watch and manage resources across all namespaces +singleNamespace: false
19-52: Consider documenting upgrade implications.The namespace scoping feature is a significant architectural change that enhances multi-tenant support. Consider documenting:
- Upgrade path from cluster-wide to namespace-scoped deployment
- Impact on existing resources when switching modes
- Recommendations for choosing between modes
Would you like me to help draft the upgrade documentation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/deploy.yaml(2 hunks)Dockerfile(1 hunks)RELEASE.md(1 hunks)deploy/charts/emqx-operator/Chart.yaml(1 hunks)deploy/charts/emqx-operator/README.md(1 hunks)deploy/charts/emqx-operator/templates/controller-manager-rbac.yaml(1 hunks)deploy/charts/emqx-operator/templates/controller-manager.yaml(1 hunks)deploy/charts/emqx-operator/values.yaml(1 hunks)main.go(3 hunks)sidecar/reloader/Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- deploy/charts/emqx-operator/templates/controller-manager.yaml
- Dockerfile
- deploy/charts/emqx-operator/README.md
- deploy/charts/emqx-operator/values.yaml
- sidecar/reloader/Dockerfile
- main.go
🔇 Additional comments (3)
.github/workflows/deploy.yaml (2)
35-40: Single namespace testing should be extended to v1beta3 and v1beta4 versions
72-73: LGTM: Helm installation and operator readiness check
The changes correctly implement:
- Setting the singleNamespace parameter in Helm install
- Conditional namespace selection
- Updated operator readiness check
Also applies to: 77-80
deploy/charts/emqx-operator/templates/controller-manager-rbac.yaml (1)
19-35: LGTM! Verify namespace isolation.
The conditional RoleBinding/ClusterRoleBinding implementation follows the principle of least privilege. When singleNamespace is true, the operator's permissions are correctly scoped to the release namespace.
Let's verify that no cluster-wide permissions are granted when singleNamespace is true:
✅ Verification successful
Let me gather more context about the conditional logic to ensure the cluster-wide permissions are properly gated.
Let me check the values file to ensure the singleNamespace option is properly documented and defaulted.
RBAC configuration correctly implements namespace isolation
The implementation properly handles namespace-scoped vs cluster-scoped permissions:
- When
singleNamespace: true, it creates Role and RoleBinding scoped to the release namespace - When
singleNamespace: false(default), it creates ClusterRole and ClusterRoleBinding - The conditional logic is complete and consistent across all RBAC resources
- The option is well-documented in values.yaml with clear description
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify no cluster-wide permissions when singleNamespace is true
# Check for any ClusterRole/ClusterRoleBinding references when singleNamespace is true
rg -A 5 'ClusterRole|ClusterRoleBinding' deploy/charts/emqx-operator/templates/
Length of output: 1763
Script:
#!/bin/bash
# Check the complete RBAC template file to understand the conditional structure
cat deploy/charts/emqx-operator/templates/controller-manager-rbac.yaml
Length of output: 4596
Script:
#!/bin/bash
# Check the values.yaml file for singleNamespace configuration
cat deploy/charts/emqx-operator/values.yaml | grep -A 5 -B 5 "singleNamespace"
Length of output: 514
Rory-Z
force-pushed
the
feat/support-single-namespace
branch
from
abe5ac5 to
b18464d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
RELEASE.md (3)
Line range hint
5-15: Document migration paths for new API versionsThe new API versions (v2beta1 and v1beta4) are well documented with compatibility information. Consider adding:
- Migration guides from previous versions
- Breaking changes (if any)
- Deprecation timeline for older versions
18-18: Improve enhancement description clarity and detailThe current description has a minor grammar issue and could be more detailed. Consider this revision:
-+ Can deploy EMQX operator in once namespace scope, and the operator will only manage the resources in the same namespace. ++ EMQX operator can now be deployed in a single namespace scope, where it will only manage resources within that namespace.Consider adding:
- Configuration steps for single namespace deployment
- Use cases or scenarios where this is beneficial
- Any limitations or considerations
🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: You might be missing the article “an” here.
Context: ...later ## Enhancements 🚀 + Can deploy EMQX operator in once namespace scope, and t...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
Line range hint
22-31: Enhance installation instructions for new featuresWhile the basic installation commands are provided, consider adding:
- Instructions for single namespace deployment configuration
- Verification steps after installation
- Common troubleshooting steps
Example addition:
# For single namespace deployment helm upgrade --install emqx-operator emqx/emqx-operator \ --namespace emqx-operator-system \ --create-namespace \ --version 2.2.26-rc.1 \ --set singleNamespace=true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/deploy.yaml(2 hunks)Dockerfile(1 hunks)RELEASE.md(3 hunks)deploy/charts/emqx-operator/Chart.yaml(1 hunks)deploy/charts/emqx-operator/README.md(1 hunks)deploy/charts/emqx-operator/templates/controller-manager-rbac.yaml(1 hunks)deploy/charts/emqx-operator/templates/controller-manager.yaml(1 hunks)deploy/charts/emqx-operator/values.yaml(1 hunks)main.go(3 hunks)sidecar/reloader/Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- deploy/charts/emqx-operator/templates/controller-manager.yaml
- deploy/charts/emqx-operator/README.md
- Dockerfile
- deploy/charts/emqx-operator/values.yaml
- sidecar/reloader/Dockerfile
- .github/workflows/deploy.yaml
- main.go
- deploy/charts/emqx-operator/templates/controller-manager-rbac.yaml
- deploy/charts/emqx-operator/Chart.yaml
🧰 Additional context used
🪛 LanguageTool
RELEASE.md
[uncategorized] ~18-~18: You might be missing the article “an” here.
Context: ...later ## Enhancements 🚀 + Can deploy EMQX operator in once namespace scope, and t...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
🔇 Additional comments (1)
RELEASE.md (1)
3-3: Verify release candidate readiness
This is marked as a release candidate (-rc.1). Please ensure that:
- All necessary testing has been completed
- Any known issues or limitations are documented
✅ Verification successful
Release candidate documentation is complete and well-structured
The release notes for 2.2.26-rc.1 are well documented with:
- Clear supported versions for both EMQX and EMQX Enterprise
- Specific enhancement regarding namespace-scoped deployment
- Detailed installation/upgrade instructions with prerequisites
- Important warning about API deprecation
The documentation provides sufficient information for users to test and deploy this release candidate. No additional testing status documentation is needed as this is an infrastructure operator where testing is typically done before creating the release candidate tag.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any open issues or PRs that might affect this release
gh issue list --label "release-blocker" --state open
gh pr list --label "release-blocker" --state open
Length of output: 102
Script:
#!/bin/bash
# Let's check the git history for recent commits and tags related to this release
git tag | grep "2.2.26"
git log --oneline -n 10 --grep="2.2.26"
# Also check the content of RELEASE.md for any documented testing or known issues
cat RELEASE.md
Length of output: 976
Signed-off-by: Rory Z <[email protected]>
Rory-Z
force-pushed
the
feat/support-single-namespace
branch
from
b18464d to
2cd0ad9
Compare
Summary by CodeRabbit
New Features
singleNamespacefor the EMQX operator, allowing deployment in a single namespace.WATCH_NAMESPACEto the controller manager for dynamic namespace awareness.Bug Fixes
singleNamespacesetting.Documentation
singleNamespaceparameter in the configuration table.Chores