This repository was archived by the owner on Sep 9, 2026. It is now read-only.
Repository navigation
OSAC-1551: Add tenant scoping to the CLI - #810
Closed
DakCrowder wants to merge 1 commit into
Closed
DakCrowder wants to merge 1 commit into
DakCrowder wants to merge 1 commit into
Conversation
Add multi-tenant support to the osac CLI, enabling users to scope operations to a specific tenant via a saved setting or a per-command flag. Core changes: - Add `osac tenant` command to view, set, and clear the active tenant (persisted in the OS keyring alongside credentials) - Add global `--tenant` flag with precedence over the saved setting - Add TENANT column into CLI display tables when no tenant is selected and the resource is tenant-scoped Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dakota Crowder <dcrowder@redhat.com>
|
@DakCrowder: This pull request references OSAC-1551 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. 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 NOT APPROVED This pull-request has been approved by: DakCrowder The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughAdds tenant scoping across the CLI: new ChangesTenant Scoping for CLI
Sequence Diagram(s)sequenceDiagram
participant User
participant RootCmd
participant TenantCmd
participant Settings
participant ReflectionHelper
participant gRPC
User->>RootCmd: run --tenant foo <subcommand>
RootCmd->>Settings: Tenant()
Settings-->>RootCmd: saved tenant
RootCmd->>RootCmd: override with --tenant flag if changed
RootCmd->>RootCmd: TenantIntoContext(ctx, effective tenant)
User->>TenantCmd: tenant set NAME
TenantCmd->>gRPC: connect
TenantCmd->>ReflectionHelper: Lookup("tenant")
ReflectionHelper-->>TenantCmd: objectHelper
TenantCmd->>ReflectionHelper: FindObject(ctx, NAME)
ReflectionHelper-->>TenantCmd: resolved tenant proto
TenantCmd->>Settings: SetTenant(NAME) + Save()
TenantCmd-->>User: "Tenant 'NAME' saved."
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 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 |
Contributor
Author
|
/hold |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/cmd/cli/tenant/templates/no_tenant.txt`:
- Around line 5-9: The tenant help template contains an outdated example
command, since `{{ binary }} get tenants` is not valid for this CLI flow. Update
the example text in the no-tenant template used by the tenant command help to
show the correct positional usage, such as `{{ binary }} tenant` or `{{ binary
}} tenant my-tenant`, and keep the surrounding help copy aligned with the
`tenant` command behavior.
In `@internal/cmd/cli/tenant/tenant_cmd_test.go`:
- Around line 41-44: The spec named “Looks up tenant resource type directly” in
tenant_cmd_test.go is a no-op because it only creates a runnerContext and
discards it without asserting anything. Update this test to actually exercise
the tenant lookup behavior using runnerContext and add real expectations against
the lookup path, or remove the It block entirely if there is no testable
behavior yet. Make sure the fix is tied to the runnerContext setup in this test
so the case provides meaningful coverage instead of always passing.
In `@internal/cmd/cli/tenant/tenant_cmd.go`:
- Around line 105-126: showCurrentTenant is reading the persisted tenant from
settings instead of the effective tenant already resolved by root command
argument handling, so --tenant overrides are ignored. Update showCurrentTenant
to use the tenant value from the command context (the one populated by the
root_cmd path) as the source of truth, and only fall back to settings if that
context value is missing; keep the existing no_tenant/current_tenant/findTenant
rendering flow intact.
- Around line 75-102: In runnerContext.run, add upfront validation for
conflicting tenant command inputs instead of letting the current branching
silently drop them: reject cases where c.args.clear is set together with any
positional NAME, reject more than one positional argument, and reject --output
when a NAME is provided so it is only accepted in the no-arg path. Keep the
existing showCurrentTenant and setTenant flow, but make the command fail fast
with a clear error before calling settings.SetTenant, c.console.Render, or
c.setTenant when the invocation is ambiguous.
In `@internal/reflection/reflection_helper.go`:
- Around line 785-828: Normalize the alignment of the tenantScopedTypes map in
reflection_helper.go so golangci-lint/gofmt stops failing; the map literal in
the tenantScopedTypes declaration has misaligned boolean values, so reformat
that block with gofmt (or by editing the entries around tenantScopedTypes) to
make the true columns consistently aligned across all osac.public.v1 and
osac.private.v1 types.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 686c4d85-fe00-4b26-a9ec-91a727121de9
📒 Files selected for processing (29)
internal/cmd/cli/create/cluster/create_cluster_cmd.gointernal/cmd/cli/create/computeinstance/create_compute_instance_cmd.gointernal/cmd/cli/create/create_cmd.gointernal/cmd/cli/create/publicip/create_publicip_cmd.gointernal/cmd/cli/create/securitygroup/create_securitygroup_cmd.gointernal/cmd/cli/create/subnet/create_subnet_cmd.gointernal/cmd/cli/create/virtualnetwork/create_virtualnetwork_cmd.gointernal/cmd/cli/root_cmd.gointernal/cmd/cli/tenant/templates/current_tenant.txtinternal/cmd/cli/tenant/templates/multiple_matches.txtinternal/cmd/cli/tenant/templates/no_matches.txtinternal/cmd/cli/tenant/templates/no_tenant.txtinternal/cmd/cli/tenant/templates/tenant_cleared.txtinternal/cmd/cli/tenant/templates/tenant_set.txtinternal/cmd/cli/tenant/tenant_cmd.gointernal/cmd/cli/tenant/tenant_cmd_test.gointernal/cmd/cli/tenant/tenant_suite_test.gointernal/config/config_context.gointernal/config/config_context_test.gointernal/config/config_settings.gointernal/config/config_settings_test.gointernal/reflection/reflection_helper.gointernal/reflection/reflection_helper_mock.gointernal/reflection/reflection_helper_test.gointernal/reflection/reflection_metadata.gointernal/reflection/reflection_object_helper_mock.gointernal/rendering/table_renderer.gointernal/rendering/table_renderer_compute_instance_test.gointernal/rendering/table_renderer_test.go
Comment on lines
+5
to
+9
| {{ binary }} get tenants | ||
|
|
||
| To set a tenant: | ||
|
|
||
| {{ binary }} tenant <name> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find tenant command Use string and registered resources to confirm help text.
fd -t f 'tenant_cmd.go' internal/cmd/cli | xargs rg -n 'Use:|set-tenant|--clear'
rg -n 'Use:\s*"tenants?' internal/cmd/cliRepository: osac-project/fulfillment-service
Length of output: 350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map tenant-related CLI command definitions and help templates.
ast-grep outline internal/cmd/cli/tenant/tenant_cmd.go --view expanded || true
rg -n 'get tenants|tenant --clear|tenant \[FLAG\.\.\.\] \[NAME\]|Use:' internal/cmd/cli -g '!**/*_test.go'
fd -t f . internal/cmd/cli | sed -n '1,200p'Repository: osac-project/fulfillment-service
Length of output: 15372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' internal/cmd/cli/get/get_cmd.go
printf '\n--- tenant cmd ---\n'
sed -n '1,280p' internal/cmd/cli/tenant/tenant_cmd.goRepository: osac-project/fulfillment-service
Length of output: 11872
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '\bget tenants\b|\bget tenant\b|tenant templates/no_(tenant|matches)\.txt|Lookup\("tenant"\)|OBJECT \[ID\|NAME\]' internal/cmd/cli docs README.mdRepository: osac-project/fulfillment-service
Length of output: 505
Replace the stale tenant help example. {{ binary }} get tenants is not a valid command here; the command takes an optional positional name, so the example should use {{ binary }} tenant or {{ binary }} tenant my-tenant instead.
🤖 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 `@internal/cmd/cli/tenant/templates/no_tenant.txt` around lines 5 - 9, The
tenant help template contains an outdated example command, since `{{ binary }}
get tenants` is not valid for this CLI flow. Update the example text in the
no-tenant template used by the tenant command help to show the correct
positional usage, such as `{{ binary }} tenant` or `{{ binary }} tenant
my-tenant`, and keep the surrounding help copy aligned with the `tenant` command
behavior.
Comment on lines
+41
to
+44
| It("Looks up tenant resource type directly", func() { | ||
| runner := &runnerContext{} | ||
| _ = runner | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This spec asserts nothing — it's a green light wired to no bulb.
Looks up tenant resource type directly builds a runnerContext and immediately discards it with _ = runner. There's no Expect, so it passes unconditionally and gives false coverage for tenant lookup. Either flesh it out to actually exercise the lookup path or remove it until you can.
🤖 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 `@internal/cmd/cli/tenant/tenant_cmd_test.go` around lines 41 - 44, The spec
named “Looks up tenant resource type directly” in tenant_cmd_test.go is a no-op
because it only creates a runnerContext and discards it without asserting
anything. Update this test to actually exercise the tenant lookup behavior using
runnerContext and add real expectations against the lookup path, or remove the
It block entirely if there is no testable behavior yet. Make sure the fix is
tied to the runnerContext setup in this test so the case provides meaningful
coverage instead of always passing.
Comment on lines
+75
to
+102
| func (c *runnerContext) run(cmd *cobra.Command, args []string) error { | ||
| ctx := cmd.Context() | ||
|
|
||
| c.logger = logging.LoggerFromContext(ctx) | ||
| c.console = terminal.ConsoleFromContext(ctx) | ||
|
|
||
| err := c.console.AddTemplates(templatesFS, "templates") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to load templates: %w", err) | ||
| } | ||
|
|
||
| settings := config.SettingsFromContext(ctx) | ||
|
|
||
| if c.args.clear { | ||
| settings.SetTenant("") | ||
| err = settings.Save(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to save settings: %w", err) | ||
| } | ||
| c.console.Render(ctx, "tenant_cleared.txt", nil) | ||
| return nil | ||
| } | ||
|
|
||
| if len(args) == 0 { | ||
| return c.showCurrentTenant(cmd, settings) | ||
| } | ||
|
|
||
| return c.setTenant(cmd, settings, args[0]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject conflicting tenant invocations instead of silently ignoring inputs.
This path currently clears on --clear even if a NAME is also passed, ignores positional args after args[0], and accepts --output with NAME even though that flag is only honored in the no-arg branch. These combinations should fail fast; otherwise the CLI can do something different from what the user asked for.
🤖 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 `@internal/cmd/cli/tenant/tenant_cmd.go` around lines 75 - 102, In
runnerContext.run, add upfront validation for conflicting tenant command inputs
instead of letting the current branching silently drop them: reject cases where
c.args.clear is set together with any positional NAME, reject more than one
positional argument, and reject --output when a NAME is provided so it is only
accepted in the no-arg path. Keep the existing showCurrentTenant and setTenant
flow, but make the command fail fast with a clear error before calling
settings.SetTenant, c.console.Render, or c.setTenant when the invocation is
ambiguous.
Comment on lines
+105
to
+126
| func (c *runnerContext) showCurrentTenant(cmd *cobra.Command, settings *config.Settings) error { | ||
| ctx := cmd.Context() | ||
| tenant := settings.Tenant() | ||
|
|
||
| if tenant == "" { | ||
| c.console.Render(ctx, "no_tenant.txt", nil) | ||
| return nil | ||
| } | ||
|
|
||
| if c.args.format == "" { | ||
| c.console.Render(ctx, "current_tenant.txt", map[string]any{ | ||
| "Tenant": tenant, | ||
| }) | ||
| return nil | ||
| } | ||
|
|
||
| object, err := c.findTenant(cmd, settings, tenant) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return c.renderObject(cmd, object) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor --tenant precedence when showing the current tenant.
Line 107 reads the persisted tenant from settings instead of the effective tenant that internal/cmd/cli/root_cmd.go:209-223 already resolved into the command context. osac --tenant other tenant therefore still shows/serializes the saved tenant, which breaks the advertised override behavior.
🤖 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 `@internal/cmd/cli/tenant/tenant_cmd.go` around lines 105 - 126,
showCurrentTenant is reading the persisted tenant from settings instead of the
effective tenant already resolved by root command argument handling, so --tenant
overrides are ignored. Update showCurrentTenant to use the tenant value from the
command context (the one populated by the root_cmd path) as the source of truth,
and only fall back to settings if that context value is missing; keep the
existing no_tenant/current_tenant/findTenant rendering flow intact.
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add multi-tenant support to the osac CLI, enabling users to scope operations to a specific tenant via a saved setting or a per-command flag.
Core changes:
osac tenantcommand to view, set, and clear the active tenant (persisted in the OS keyring alongside credentials)--tenantflag with precedence over the saved settingSummary by CodeRabbit
New Features
Bug Fixes