feat: enhance environment variable support for command flags#4
Merged
Conversation
- Added support for additional environment variables: ZCP_PROJECT, ZCP_REGION, ZCP_CLOUD_PROVIDER, ZCP_OUTPUT, and ZCP_DEBUG to override corresponding command flags. - Updated documentation to reflect new environment variable usage. - Modified command implementations to resolve project, region, and cloud provider from environment variables if not provided as flags. - Improved example commands in documentation to use new defaults. - Enhanced tests to verify environment variable precedence over command flags for project, region, and cloud provider.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 10 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis release (v0.0.9) adds environment-variable overrides (ZCP_*) for profile, bearer token, API URL, project, region, cloud provider, output, and debug; changes profile/APIURL precedence; introduces resolve helpers used before command validation across many create commands; updates default API URL and docs; and adds related tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI command"
participant Resolve as "resolve helpers"
participant Env as "Environment (ZCP_*)"
participant Config as "Config/Profile"
participant Client as "HTTP Client / API"
CLI->>Resolve: call resolveProject/Region/CloudProvider
Resolve->>Env: read ZCP_PROJECT / ZCP_REGION / ZCP_CLOUD_PROVIDER
Resolve->>Config: consult profile/defaults if env not set
Resolve-->>CLI: return resolved values
CLI->>Client: buildClientAndPrinter (uses ZCP_OUTPUT, ZCP_DEBUG, ZCP_BEARER_TOKEN, ZCP_API_URL)
Client->>Env: read ZCP_BEARER_TOKEN / ZCP_API_URL as overrides
Client->>Config: use profile values if env not provided
Client-->>CLI: initialized client ready to call API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/commands/vmbackup.go (1)
92-109:⚠️ Potential issue | 🟠 MajorResolve
cloud-providerandregionbefore validation invm-backup create.
projectis resolved at Line 107, butcloudProviderandregionare checked as raw flag values at Line 92 and Line 95 and never passed through resolver helpers. This prevents env defaults from working for those fields in this command.Suggested fix
RunE: func(cmd *cobra.Command, args []string) error { + cloudProvider = resolveCloudProvider(cloudProvider) if cloudProvider == "" { return fmt.Errorf("--cloud-provider is required") } + region = resolveRegion(region) if region == "" { return fmt.Errorf("--region is required") } @@ project = resolveProject(project) if project == "" { return fmt.Errorf("--project is required") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/vmbackup.go` around lines 92 - 109, The validation checks for cloudProvider and region are running on raw flag values before applying env/default resolution; update the vm-backup create flow to call the appropriate resolver helpers for cloudProvider and region (similar to how project uses resolveProject) before the required-field checks—i.e., replace direct checks of cloudProvider and region with resolved values (e.g., cloudProvider = resolveCloudProvider(cloudProvider); region = resolveRegion(region);) and then validate them along with project, billingCycle, plan, and psudoService.internal/commands/instance.go (1)
976-985:⚠️ Potential issue | 🟡 MinorInconsistent resolution:
regionandcloudProviderare not resolved via env vars.In
newInstancePurchaseAddonCmd, onlyprojectis resolved viaresolveProject(), butregionandcloudProviderare validated directly without callingresolveRegion()andresolveCloudProvider(). This is inconsistent with other commands and the PR's stated objective of supporting environment variable defaults for all three parameters.🔧 Proposed fix
project = resolveProject(project) if project == "" { return fmt.Errorf("--project is required") } + region = resolveRegion(region) if region == "" { return fmt.Errorf("--region is required") } + cloudProvider = resolveCloudProvider(cloudProvider) if cloudProvider == "" { return fmt.Errorf("--cloud-provider is required") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/instance.go` around lines 976 - 985, The validation is inconsistent: only project is normalized with resolveProject() while region and cloudProvider are checked directly; call resolveRegion(region) and resolveCloudProvider(cloudProvider) (similarly to resolveProject(project)) before the nil/empty checks so region and cloudProvider can be sourced from environment defaults, then validate the resolved region and cloudProvider and return the same error messages if they are empty; update the variables used in the subsequent checks to the resolved values (e.g., replace direct uses of region/cloudProvider with the outputs of resolveRegion/resolveCloudProvider) in newInstancePurchaseAddonCmd.
🧹 Nitpick comments (2)
internal/commands/commands_test.go (1)
73-111: Consider adding "empty when neither set" tests forresolveRegionandresolveCloudProvider.The tests for
resolveProjectinclude aTestResolveProjectEmptyWhenNeitherSetcase, but the corresponding edge case tests are missing forresolveRegionandresolveCloudProvider. For consistency and completeness, consider adding these tests.♻️ Suggested additional tests
func TestResolveRegionEmptyWhenNeitherSet(t *testing.T) { os.Unsetenv("ZCP_REGION") result := resolveRegion("") if result != "" { t.Errorf("resolveRegion = %q, want empty", result) } } func TestResolveCloudProviderEmptyWhenNeitherSet(t *testing.T) { os.Unsetenv("ZCP_CLOUD_PROVIDER") result := resolveCloudProvider("") if result != "" { t.Errorf("resolveCloudProvider = %q, want empty", result) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/commands_test.go` around lines 73 - 111, Add two tests to cover the "neither flag nor env set" edge cases: create TestResolveRegionEmptyWhenNeitherSet and TestResolveCloudProviderEmptyWhenNeitherSet that ensure resolveRegion("") and resolveCloudProvider("") return an empty string when ZCP_REGION and ZCP_CLOUD_PROVIDER are unset; in each test explicitly unset the corresponding env var (os.Unsetenv("ZCP_REGION") and os.Unsetenv("ZCP_CLOUD_PROVIDER")), call the resolver with an empty flag, and assert the result equals "" using the same t.Errorf pattern as the existing tests.internal/commands/helpers.go (1)
30-36: Consider accepting additional truthy values forZCP_DEBUG.Currently
ZCP_DEBUGonly accepts the exact string"true". Users might expect other common truthy values like"1","yes", or"TRUE"to work. This is a minor UX inconsistency but not blocking.♻️ Optional: Accept more truthy values
- if os.Getenv("ZCP_DEBUG") == "true" { + debugEnv := strings.ToLower(os.Getenv("ZCP_DEBUG")) + if debugEnv == "true" || debugEnv == "1" || debugEnv == "yes" { debugFlag = true }This would require adding
"strings"to the imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/helpers.go` around lines 30 - 36, Update the ZCP_DEBUG environment handling so it accepts common truthy values (e.g., "1", "yes", "true", case-insensitive) instead of only the exact "true"; locate the block that sets debugFlag from os.Getenv("ZCP_DEBUG") and replace the strict comparison with a check that normalizes/case-folds the value or uses a boolean parser (e.g., strings.EqualFold or strconv.ParseBool) to set debugFlag accordingly; add the "strings" or "strconv" import as needed and ensure debugFlag is set to true for "1", "yes", "true" (any case) and false otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/config_test.go`:
- Around line 140-155: TestResolveProfileEnvProfile fails when an ambient
ZCP_BEARER_TOKEN is present because ResolveProfile honors the bearer-token env
override; update the test to clear or override that env var (e.g., call
t.Setenv("ZCP_BEARER_TOKEN", "") or otherwise unset it) before invoking
config.ResolveProfile so the profile's BearerToken from cfg.Profiles
("staging-token") is used; locate this change in the
TestResolveProfileEnvProfile test near the ResolveProfile call in
internal/config/config_test.go.
In `@internal/config/config.go`:
- Around line 131-135: The code currently treats a missing profile key the same
as a profile with missing credentials; update the selection logic around
cfg.Profiles and variable p so you first check existence of the profile key and
return a distinct "profile not found" error when cfg.Profiles[name] is absent
(instead of falling through to the credential-check path), and only if the
profile exists validate its credentials and return "missing credentials" for
that profile; adjust the branches that reference cfg.Profiles[name], p, and the
subsequent credential checks (the selection block starting with if name != ""
and the credential-validation block around lines 145-149) to produce two
separate error cases.
In `@RELEASE_NOTES.md`:
- Around line 39-46: The table documents ZCP_PROFILE precedence backwards:
update the ZCP_PROFILE row so it reflects that CLI flag (--profile) takes
precedence over the environment variable and that ResolveProfile checks an
explicit profileName (CLI flag) first, then the ZCP_PROFILE env var, then
profile config; reference ResolveProfile and the profileName parameter when
changing the wording to match runtime behavior.
---
Outside diff comments:
In `@internal/commands/instance.go`:
- Around line 976-985: The validation is inconsistent: only project is
normalized with resolveProject() while region and cloudProvider are checked
directly; call resolveRegion(region) and resolveCloudProvider(cloudProvider)
(similarly to resolveProject(project)) before the nil/empty checks so region and
cloudProvider can be sourced from environment defaults, then validate the
resolved region and cloudProvider and return the same error messages if they are
empty; update the variables used in the subsequent checks to the resolved values
(e.g., replace direct uses of region/cloudProvider with the outputs of
resolveRegion/resolveCloudProvider) in newInstancePurchaseAddonCmd.
In `@internal/commands/vmbackup.go`:
- Around line 92-109: The validation checks for cloudProvider and region are
running on raw flag values before applying env/default resolution; update the
vm-backup create flow to call the appropriate resolver helpers for cloudProvider
and region (similar to how project uses resolveProject) before the
required-field checks—i.e., replace direct checks of cloudProvider and region
with resolved values (e.g., cloudProvider = resolveCloudProvider(cloudProvider);
region = resolveRegion(region);) and then validate them along with project,
billingCycle, plan, and psudoService.
---
Nitpick comments:
In `@internal/commands/commands_test.go`:
- Around line 73-111: Add two tests to cover the "neither flag nor env set" edge
cases: create TestResolveRegionEmptyWhenNeitherSet and
TestResolveCloudProviderEmptyWhenNeitherSet that ensure resolveRegion("") and
resolveCloudProvider("") return an empty string when ZCP_REGION and
ZCP_CLOUD_PROVIDER are unset; in each test explicitly unset the corresponding
env var (os.Unsetenv("ZCP_REGION") and os.Unsetenv("ZCP_CLOUD_PROVIDER")), call
the resolver with an empty flag, and assert the result equals "" using the same
t.Errorf pattern as the existing tests.
In `@internal/commands/helpers.go`:
- Around line 30-36: Update the ZCP_DEBUG environment handling so it accepts
common truthy values (e.g., "1", "yes", "true", case-insensitive) instead of
only the exact "true"; locate the block that sets debugFlag from
os.Getenv("ZCP_DEBUG") and replace the strict comparison with a check that
normalizes/case-folds the value or uses a boolean parser (e.g.,
strings.EqualFold or strconv.ParseBool) to set debugFlag accordingly; add the
"strings" or "strconv" import as needed and ensure debugFlag is set to true for
"1", "yes", "true" (any case) and false otherwise.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48b457da-50d1-49f0-904c-52e0986f5a48
📒 Files selected for processing (28)
CHANGELOG.mdREADME.mdRELEASE_NOTES.mddocs/command-taxonomy.mddocs/configuration.mddocs/roadmap.mdinternal/commands/affinitygroup.gointernal/commands/autoscale.gointernal/commands/backup.gointernal/commands/billing.gointernal/commands/commands_test.gointernal/commands/dns.gointernal/commands/helpers.gointernal/commands/instance.gointernal/commands/iso.gointernal/commands/kubernetes.gointernal/commands/loadbalancer.gointernal/commands/network.gointernal/commands/snapshot.gointernal/commands/template.gointernal/commands/virtualrouter.gointernal/commands/vmbackup.gointernal/commands/vmsnapshot.gointernal/commands/volume.gointernal/commands/vpc.gointernal/commands/vpn.gointernal/config/config.gointernal/config/config_test.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/commands/instance.go`:
- Around line 206-214: The current resolution helpers (resolveCloudProvider,
resolveProject, resolveRegion) are using flag-first precedence but the feature
requires ZCP_* environment variables to override flags; update the helpers in
internal/commands/helpers.go so they check corresponding ZCP_ environment
variables (e.g., ZCP_CLOUD_PROVIDER, ZCP_PROJECT, ZCP_REGION) before falling
back to flag values, and ensure instance command callers (the code paths in
instance create and instance purchase-addon that call
resolveCloudProvider/resolveProject/resolveRegion) do not re-order or override
this new precedence.
In `@internal/config/config_test.go`:
- Around line 131-145: TestResolveProfileEnvTokenOverridesProfile (and the
similar test at lines 167-181) can be misled by an ambient ZCP_PROFILE;
explicitly pin ZCP_PROFILE to "prod" before calling config.ResolveProfile and
assert that the returned profile's Name equals "prod". Concretely, in
TestResolveProfileEnvTokenOverridesProfile and the other test, call
t.Setenv("ZCP_PROFILE", "prod") in addition to t.Setenv("ZCP_BEARER_TOKEN", ...)
and add an assertion on p.Name (from ResolveProfile) to ensure the correct
profile was resolved.
- Around line 99-100: The subtest only clears ZCP_BEARER_TOKEN which allows an
ambient ZCP_PROFILE to leak into the `"active profile"` case; before calling
config.ResolveProfile(cfg, tt.profileName) add t.Setenv("ZCP_PROFILE", "") in
the same subtest setup so the test deterministically uses the provided
tt.profileName (look for the subtest loop that calls
t.Setenv("ZCP_BEARER_TOKEN", "") and config.ResolveProfile).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfd7fce6-4de6-4993-b5a5-d58d8244f822
📒 Files selected for processing (7)
RELEASE_NOTES.mdinternal/commands/commands_test.gointernal/commands/helpers.gointernal/commands/instance.gointernal/commands/vmbackup.gointernal/config/config.gointernal/config/config_test.go
✅ Files skipped from review due to trivial changes (1)
- RELEASE_NOTES.md
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/commands/helpers.go
- internal/commands/vmbackup.go
- internal/config/config.go
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests