fix: make auto rbac create optional #585
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds support for a Changestelemetry.rbac.create opt-out
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/helm/telemetry-sidecar_test.go (1)
144-146: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMake telemetry enablement explicit in this test setup.
To isolate the
rbac.createbehavior and avoid future default-value drift, settelemetry.enabled=truein this test’sSetValues.Suggested update
options := &helm.Options{SetValues: map[string]string{ + "telemetry.enabled": "true", "telemetry.rbac.create": "false", }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/helm/telemetry-sidecar_test.go` around lines 144 - 146, In the helm.Options SetValues map, add an explicit entry for "telemetry.enabled": "true" alongside the existing "telemetry.rbac.create": "false" entry. This makes the telemetry enablement explicit in the test setup and prevents issues from future default-value changes, ensuring the test isolates the rbac.create behavior specifically.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@helm/fiftyone-teams-app/README.md`:
- Line 1089: In the README.md documentation for the telemetry.rbac.create
configuration value, replace the reference to serviceAccounts with the full Helm
key path telemetry.serviceAccounts. This ensures users refer to the correct
configuration path when following the documentation and prevents them from
attempting to set the configuration at the wrong level in their values file.
In `@tests/unit/helm/telemetry-rolebinding_test.go`:
- Around line 75-76: The current test assertion using s.ErrorContains with a
single generic error message does not independently verify that both the Role
and RoleBinding templates are skipped when telemetry.rbac.create=false. Instead
of making a single RenderTemplateE call and checking for a generic error, you
should render each template separately (Role and RoleBinding) within the test
and verify that each one individually returns an error containing "could not
find template". This ensures both templates are actually being gated by the
telemetry.rbac.create setting rather than just one of them being absent.
---
Nitpick comments:
In `@tests/unit/helm/telemetry-sidecar_test.go`:
- Around line 144-146: In the helm.Options SetValues map, add an explicit entry
for "telemetry.enabled": "true" alongside the existing "telemetry.rbac.create":
"false" entry. This makes the telemetry enablement explicit in the test setup
and prevents issues from future default-value changes, ensuring the test
isolates the rbac.create behavior specifically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f20f534a-4220-4ec2-acd2-5236c08111bd
⛔ Files ignored due to path filters (4)
helm/fiftyone-teams-app/templates/telemetry-role.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/telemetry-rolebinding.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/values.schema.jsonis excluded by!**/*.jsonhelm/fiftyone-teams-app/values.yamlis excluded by!**/*.yaml
📒 Files selected for processing (4)
helm/fiftyone-teams-app/README.mdhelm/fiftyone-teams-app/templates/NOTES.txttests/unit/helm/telemetry-rolebinding_test.gotests/unit/helm/telemetry-sidecar_test.go
| _, err := helm.RenderTemplateE(s.T(), options, s.chartPath, s.releaseName, s.templates) | ||
| s.ErrorContains(err, "could not find template") |
There was a problem hiding this comment.
Tighten this assertion to verify each template is skipped independently.
The current check can pass if only one of the Role/RoleBinding templates is absent; it doesn’t prove both are gated by telemetry.rbac.create=false.
Suggested test hardening
func (s *telemetryRoleBindingTemplateTest) TestRbacCreateDisabled() {
options := &helm.Options{SetValues: map[string]string{
"telemetry.enabled": "true",
"telemetry.rbac.create": "false",
}}
- _, err := helm.RenderTemplateE(s.T(), options, s.chartPath, s.releaseName, s.templates)
- s.ErrorContains(err, "could not find template")
+ for _, tmpl := range s.templates {
+ _, err := helm.RenderTemplateE(s.T(), options, s.chartPath, s.releaseName, []string{tmpl})
+ s.ErrorContains(err, "could not find template", "%s should be skipped", tmpl)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, err := helm.RenderTemplateE(s.T(), options, s.chartPath, s.releaseName, s.templates) | |
| s.ErrorContains(err, "could not find template") | |
| for _, tmpl := range s.templates { | |
| _, err := helm.RenderTemplateE(s.T(), options, s.chartPath, s.releaseName, []string{tmpl}) | |
| s.ErrorContains(err, "could not find template", "%s should be skipped", tmpl) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/helm/telemetry-rolebinding_test.go` around lines 75 - 76, The
current test assertion using s.ErrorContains with a single generic error message
does not independently verify that both the Role and RoleBinding templates are
skipped when telemetry.rbac.create=false. Instead of making a single
RenderTemplateE call and checking for a generic error, you should render each
template separately (Role and RoleBinding) within the test and verify that each
one individually returns an error containing "could not find template". This
ensures both templates are actually being gated by the telemetry.rbac.create
setting rather than just one of them being absent.
kaixi-wang
force-pushed
the
kacey/optional-role-creation
branch
from
7beaf00 to
8233a66
Compare
2 participants
Rationale
Allow configuring role/rolebinding to be handled independently for telemetry or at least allow metrics to still work
(CLASS ran into an issue: https://voxel51.slack.com/archives/C05BYM8P01M/p1782136883542679)
Review Priority
Changes
Checklist
Testing