adding enable_write_sre_tools flag#676
Conversation
📝 WalkthroughWalkthroughAdds an ephemeral-only ChangesSRE tools access control
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Config as Environment configuration
participant Render as render.py
participant Values as Grafana and Argo CD values
Config->>Render: enable_write_sre_tools
alt enabled in non-ephemeral environment
Render-->>Config: raise ValueError
else enabled in ephemeral environment
Render->>Values: render Grafana Admin and Argo CD admin settings
else disabled
Render->>Values: render Grafana Viewer and Argo CD read-only settings
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/test_render.py (1)
2019-2119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated fixture dict across two tests.
sre_tools_applicationsis defined identically intest_write_sre_tools_false_renders_readonly_overlayandtest_write_sre_tools_true_renders_admin_overlay_for_ephemeral. Consider extracting to a shared helper/fixture to avoid drift if the template changes.🤖 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 `@scripts/test_render.py` around lines 2019 - 2119, The two tests duplicate the same sre_tools_applications fixture data, which risks drift if the template changes. Extract the shared dict into a reusable helper or pytest fixture in this test module, then have test_write_sre_tools_false_renders_readonly_overlay and test_write_sre_tools_true_renders_admin_overlay_for_ephemeral reference that shared source instead of redefining it inline.
🤖 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.
Nitpick comments:
In `@scripts/test_render.py`:
- Around line 2019-2119: The two tests duplicate the same sre_tools_applications
fixture data, which risks drift if the template changes. Extract the shared dict
into a reusable helper or pytest fixture in this test module, then have
test_write_sre_tools_false_renders_readonly_overlay and
test_write_sre_tools_true_renders_admin_overlay_for_ephemeral reference that
shared source instead of redefining it inline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0f7bb882-2a98-4dc1-baa1-a4a80e077c93
📒 Files selected for processing (8)
config/defaults.yamldeploy/ephemeral/us-east-1/_merged_config.yamldeploy/ephemeral/us-east-1/argocd-values-regional-cluster.yamldeploy/integration/us-east-1/_merged_config.yamldeploy/integration/us-east-1/argocd-values-regional-cluster.yamldocs/development-environment.mdscripts/render.pyscripts/test_render.py
| @@ -1 +1 @@ | |||
| # GENERATED FILE - DO NOT EDIT MANUALLY | |||
There was a problem hiding this comment.
suggestion: default to write enabled for sre tools for ephemeral environments. This makes sense as a default.
There was a problem hiding this comment.
Changed the default and re-tested the provisioning. Ready for review.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/test_render.py (1)
2019-2136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the
sre_tools_applicationsdictionary to avoid duplication.The
sre_tools_applicationsdictionary setup is duplicated acrosstest_write_sre_tools_false_renders_readonly_overlayandtest_write_sre_tools_true_renders_admin_overlay_for_ephemeral. Consider extracting it into a class-level constant (e.g.,SRE_TOOLS_APPLICATIONS = {...}) or a shared Pytest fixture to improve test maintainability and adhere to DRY principles.🤖 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 `@scripts/test_render.py` around lines 2019 - 2136, Extract the duplicated sre_tools_applications dictionary from test_write_sre_tools_false_renders_readonly_overlay and test_write_sre_tools_true_renders_admin_overlay_for_ephemeral into a shared class-level constant or pytest fixture, then reuse it in both tests without changing their existing assertions or behavior.
🤖 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.
Nitpick comments:
In `@scripts/test_render.py`:
- Around line 2019-2136: Extract the duplicated sre_tools_applications
dictionary from test_write_sre_tools_false_renders_readonly_overlay and
test_write_sre_tools_true_renders_admin_overlay_for_ephemeral into a shared
class-level constant or pytest fixture, then reuse it in both tests without
changing their existing assertions or behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c0e83eca-478c-4ce3-95f1-c594f0ed8058
📒 Files selected for processing (9)
config/defaults.yamlconfig/ephemeral/defaults.yamldeploy/ephemeral/us-east-1/_merged_config.yamldeploy/ephemeral/us-east-1/argocd-values-regional-cluster.yamldeploy/integration/us-east-1/_merged_config.yamldeploy/integration/us-east-1/argocd-values-regional-cluster.yamldocs/development-environment.mdscripts/render.pyscripts/test_render.py
🚧 Files skipped from review as they are similar to previous changes (2)
- config/defaults.yaml
- scripts/render.py
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: typeid The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
ef2104e
into
openshift-online:main
2 participants
What:
ROSAENG-61363
Why:
Test steps:
Summary by CodeRabbit
New Features
regional_cluster.enable_write_sre_toolsto control SRE tool access (read-only vs admin) for ephemeral environments.Bug Fixes
Documentation
Tests