Conversation
WalkthroughCommands now read a TENANT_UUID_FLAG (via viper) and, if empty, fall back to fetchTenant(); code was changed to use a tenantUUID string across commands and API client calls instead of passing TenantInfo objects. A new helper getTenantUUID centralizes flag-or-fetch behavior and tests validate the flag and flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI Command
participant Viper as viper
participant Helper as getTenantUUID
participant API as Backend / fetchTenant
User->>CLI: Run command
CLI->>Helper: getTenantUUID()
Helper->>Viper: Read TENANT_UUID_FLAG
alt flag set
Note right of Helper #DFF2E1: return tenantUUID (from flag)
Helper-->>CLI: tenantUUID, nil
else flag empty
Helper->>API: fetchTenant()
API-->>Helper: tenant (contains UUID)
Note right of Helper #FFF3CD: tenantUUID = tenant.UUID
Helper-->>CLI: tenantUUID, nil
end
CLI->>API: Call action using tenantUUID (e.g., fetchApiKey, GetOrCreateIngestionKey)
API-->>CLI: Result
CLI-->>User: Output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (8)
🧰 Additional context used🧬 Code graph analysis (6)cmd/service_account.go (1)
cmd/get_datasources_api_key.go (1)
pkg/api/client.go (3)
cmd/login.go (1)
cmd/api_key.go (1)
cmd/tenant_uuid_flag_test.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (21)
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: 1
🧹 Nitpick comments (3)
cmd/get_datasources_api_key.go (1)
17-26: LGTM - Consistent flag handling pattern.The tenant UUID flag logic correctly prioritizes the flag value before falling back to
fetchTenant(). The creation of a partialTenantInfostruct on Line 25 ensures downstream code receives the required type.Note:
fetchDatasourcesAPIKeyreceives a*api.TenantInfobut only uses it for API calls. Consider refactoring to accepttenantUUID stringdirectly for consistency with other refactored functions likefetchServiceAccountTokenandfetchApiKey.cmd/ingestion_key_test.go (1)
10-44: Good test coverage for flag state management.The table-driven test validates that viper correctly stores and retrieves the tenant UUID flag value. The setup/teardown properly isolates test state.
Note: These tests verify viper state but don't exercise the command execution path. Consider adding integration tests that mock
fetchTenantand verify the command uses the flag value correctly when set vs. falling back tofetchTenantwhen empty.cmd/tenant_uuid_flag_test.go (1)
69-89: Validates flag accessibility across commands.Ensures all 5 affected commands can access the persistent flag. The test correctly iterates through the command list.
Minor suggestion: The test retrieves
tc.cmdbut then checksRootCmd.PersistentFlags(). Consider using the command's own persistent flags lookup to make the test more accurate, though the current approach works since persistent flags inherit from parent commands.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
cmd/api_key.go(2 hunks)cmd/generate_client_token.go(2 hunks)cmd/get_datasources_api_key.go(2 hunks)cmd/ingestion_key.go(3 hunks)cmd/ingestion_key_test.go(1 hunks)cmd/service_account.go(2 hunks)cmd/tenant_uuid_flag_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
cmd/api_key.go (3)
pkg/api/tenant.go (1)
TenantInfo(20-24)cmd/root.go (1)
TENANT_UUID_FLAG(30-30)pkg/auth/api_key.go (1)
ApiKey(13-15)
cmd/ingestion_key_test.go (1)
cmd/root.go (2)
TENANT_UUID_FLAG(30-30)RootCmd(94-128)
cmd/generate_client_token.go (2)
pkg/api/tenant.go (1)
TenantInfo(20-24)cmd/root.go (1)
TENANT_UUID_FLAG(30-30)
cmd/ingestion_key.go (2)
pkg/api/tenant.go (1)
TenantInfo(20-24)cmd/root.go (1)
TENANT_UUID_FLAG(30-30)
cmd/get_datasources_api_key.go (2)
pkg/api/tenant.go (1)
TenantInfo(20-24)cmd/root.go (1)
TENANT_UUID_FLAG(30-30)
cmd/service_account.go (3)
pkg/api/tenant.go (1)
TenantInfo(20-24)cmd/root.go (1)
TENANT_UUID_FLAG(30-30)pkg/auth/sa_token.go (1)
SAToken(11-13)
cmd/tenant_uuid_flag_test.go (2)
cmd/root.go (2)
TENANT_UUID_FLAG(30-30)RootCmd(94-128)cmd/ingestion_key.go (1)
IngestionKeyCmd(15-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: goreleaser
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
cmd/api_key.go (1)
17-24: Clean implementation - no issues found.The tenant UUID handling is straightforward and
fetchApiKeycorrectly acceptstenantUUIDas a string parameter, avoiding the need for a partial struct.cmd/service_account.go (1)
17-24: LGTM - Consistent with other commands.Implementation follows the same clean pattern as
api_key.go. The tenant variable is declared but unused when the flag is provided, which is acceptable as it maintains code symmetry.cmd/ingestion_key.go (1)
28-35: LGTM - Properly integrated.The tenant UUID flag handling integrates cleanly with both
selectBackendNameandGetOrCreateIngestionKeycalls, usingtenantUUIDconsistently throughout.cmd/ingestion_key_test.go (1)
46-54: LGTM - Validates flag registration.Verifies the constant value, flag existence, and default value. Essential checks for ensuring the flag is properly configured.
cmd/tenant_uuid_flag_test.go (4)
10-27: Excellent use of test suite for state isolation.The suite pattern with
SetupTest/TearDownTestensures each test starts with a clean viper state, preventing test pollution.
29-40: Thorough validation of flag configuration.Tests verify registration, default value, usage description, and constant correctness. Complete coverage of flag metadata.
42-66: Good coverage of viper behavior.Tests cover both empty and valid UUID scenarios, ensuring viper correctly handles the flag value in both cases.
92-119: Integration-style test validates the implementation pattern.This test simulates the actual conditional logic used in the commands (
if tenantUUID == ""), providing valuable coverage of the pattern's behavior.
ryechezkel
left a comment
ryechezkel
left a comment
There was a problem hiding this comment.
Maybe easier to put the logic inside the fetchTenanat logic ?
|
@ryechezkel yes it could be DRYer, added a helper to either fetch the tenant or use the flag. |
ryechezkel
left a comment
ryechezkel
left a comment
There was a problem hiding this comment.
Why do we need tenant info and UUID? Why not just return tenant info ?
|
Cleaned up some everything uses UUID across the board |
AvitalTamir
deleted the
avital/core-6862-cli-respect-tenant-uuid-flag-in-ingestion-key-creation
branch
2 participants
--tenant-uuidflag was being ignored by 5 CLI commands (get-ingestion-key, print-api-key, get-datasources-api-key, generate-client-token, generate-service-account-token).These commands now properly check for the flag value before falling back to interactive tenant selection.
Summary by CodeRabbit
New Features
Tests