Conversation
|
@jhernand: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (13)
Summary by CodeRabbit
WalkthroughThis PR replaces hardcoded tenant string literal "shared" with an explicit SharedTenant constant across the codebase. The auth package now exports SystemTenant and SharedTenant string variables and restructures tenant set initialization to use these constants. The DetermineDefaultTenant function is updated to reference the SharedTenant constant, and all server test files add imports of the auth package and update their metadata fixtures to use auth.SharedTenant instead of the hardcoded string. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/auth/tenancy_logic.go`:
- Around line 40-50: SystemTenant and SharedTenant are exported mutable vars;
change them to exported consts to make tenant identifiers immutable (e.g.,
replace var SystemTenant/SharedTenant string declarations with const
SystemTenant = "system" and const SharedTenant = "shared") and leave
SystemTenants and SharedTenants as vars created via
collections.NewSet(SystemTenant)/collections.NewSet(SharedTenant) so the sets
still initialize from the const values; update tenancy_logic.go accordingly,
preserving names and exports.
🪄 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: Enterprise
Run ID: afe81620-8d0f-464b-bd31-5994edb498a3
📒 Files selected for processing (14)
internal/auth/default_tenancy_logic.gointernal/auth/default_tenancy_logic_test.gointernal/auth/tenancy_logic.gointernal/servers/clusters_server_test.gointernal/servers/compute_instances_server_test.gointernal/servers/network_classes_server_test.gointernal/servers/private_clusters_server_test.gointernal/servers/private_compute_instances_server_test.gointernal/servers/private_public_ips_server_test.gointernal/servers/private_subnets_server_test.gointernal/servers/public_ips_server_test.gointernal/servers/security_groups_server_test.gointernal/servers/subnets_server_test.gointernal/servers/virtual_networks_server_test.go
| // SharedTenants is the set of tenants that are always visible to all users. | ||
| var SharedTenants = collections.NewSet("shared") | ||
| // Tenants that are always visible to all users. | ||
| var ( |
There was a problem hiding this comment.
Coderabbit also commented on this - any reason for using var and not const?
There was a problem hiding this comment.
No, no reason, fixed.
The `"shared"` and `"system"` tenant strings were used as literals in multiple places across production code and tests. This introduces `SharedTenant` and `SystemTenant` variables in the `auth` package so that the values are defined once and referenced everywhere, reducing the risk of typos and making future renames easier. The `SharedTenants` and `SystemTenants` sets now derive from these variables instead of inline strings. All test files that previously hardcoded `"shared"` in metadata tenant fields now use `auth.SharedTenant`. Related: osac-project#543 (review) Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
b1b60d3 to
7c0a6aa
Compare
|
/retest |
Summary
Introduces
SharedTenantandSystemTenantvariables in theauthpackage so that the"shared"and"system"tenant strings are defined once and referenced everywhere, reducing therisk of typos and making future renames easier. The
SharedTenantsandSystemTenantssets nowderive from these variables instead of inline strings, and all test files that previously hardcoded
"shared"in metadata tenant fields now useauth.SharedTenant.Addresses the review feedback in
#543 (review).
Test plan
ginkgo run -r internal).