bug: fix odic exchange #1292
bug: fix odic exchange #1292
Conversation
x# especially if it merges an updated upstream into a topic branch.
|
💥 This pull request now has conflicts. Could you fix it @mcalhoun? 🙏 |
📝 WalkthroughWalkthroughThis update refactors configuration handling for Atmos Pro integration. It introduces explicit struct-based configuration for Pro settings and GitHub OIDC, updates API client creation to use these structs, and removes reliance on Viper/environment variable bindings. Related function signatures, struct fields, and tests are updated to support this new configuration approach. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI/User
participant ConfigLoader
participant ExecLayer
participant APIClient
CLI/User->>ConfigLoader: Load configuration (including Pro/GitHub OIDC)
ConfigLoader-->>ExecLayer: Provide AtmosConfiguration struct
ExecLayer->>APIClient: NewAtmosProAPIClientFromEnv(logger, atmosConfig)
APIClient->>APIClient: Use atmosConfig.Pro fields for setup
APIClient-->>ExecLayer: Initialized API client
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1292 +/- ##
==========================================
+ Coverage 50.57% 50.65% +0.07%
==========================================
Files 236 236
Lines 25714 25725 +11
==========================================
+ Hits 13005 13030 +25
+ Misses 11083 11065 -18
- Partials 1626 1630 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/config/load.go (1)
377-378: Fix comment formatting.The comment should end with a period for consistency.
-// This ensures that global viper.GetString() calls work properly throughout the application +// This ensures that global viper.GetString() calls work properly throughout the application.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/root.go(1 hunks)pkg/config/load.go(2 hunks)pkg/pro/api_client.go(1 hunks)pkg/pro/api_client_get_github_oidc_token_test.go(4 hunks)pkg/pro/api_client_test.go(7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
pkg/pro/api_client_test.go (1)
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
pkg/config/load.go (1)
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
🧬 Code Graph Analysis (2)
cmd/root.go (1)
pkg/config/load.go (1)
SetupGlobalViperEnv(379-381)
pkg/config/load.go (1)
pkg/config/const.go (4)
AtmosProBaseUrlEnvVarName(89-89)AtmosProEndpointEnvVarName(90-90)AtmosProTokenEnvVarName(91-91)AtmosProWorkspaceIDEnvVarName(92-92)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (7)
pkg/pro/api_client.go (1)
247-248: LGTM - Clean environment variable standardizationGood move replacing the prefixed keys with direct environment variable names. This aligns perfectly with the global Viper setup approach and makes the code more straightforward.
cmd/root.go (1)
138-140: Excellent early setup of global environment bindingsPerfect placement - this ensures all environment variables are properly bound before any Viper calls throughout the application. The explanatory comments make the necessity crystal clear.
pkg/pro/api_client_test.go (1)
62-67: Tests properly updated to match the new environment variable approachThe consistent pattern of direct environment variable binding throughout all test cases looks good. The comments explaining this mimics the main application behavior are helpful for maintainability.
Also applies to: 88-93, 146-151, 177-182, 205-210, 242-247, 264-269
pkg/pro/api_client_get_github_oidc_token_test.go (1)
41-42: Consistent test updates align with the new environment variable strategyThe direct binding approach matches the other test files perfectly. Good consistency across the test suite.
Also applies to: 91-92, 124-125, 149-150
pkg/config/load.go (3)
104-108: Good addition of Atmos Pro environment variables.The bindings follow the established pattern and use proper constants. This aligns with the PR objective of standardizing configuration management.
379-380:✅ Verification successful
Verify that SetupGlobalViperEnv is called during application initialization.
Ensure this function is properly integrated into the application startup to make the global bindings effective.
🏁 Script executed:
#!/bin/bash # Description: Verify that SetupGlobalViperEnv is called in the application # Expected: Should find calls to this function in initialization code echo "=== Searching for SetupGlobalViperEnv function calls ===" rg -A 5 -B 5 "SetupGlobalViperEnv" echo "=== Checking cmd/root.go for integration ===" if [ -f "cmd/root.go" ]; then cat cmd/root.go | grep -A 10 -B 10 "SetupGlobalViperEnv" || echo "Not found in cmd/root.go" fiLength of output: 2661
SetupGlobalViperEnv invocation verified
I confirmed that
SetupGlobalViperEnv()is called at application startup, before anyviper.GetString()calls:
- cmd/root.go, in
func Execute() error:
cfg.SetupGlobalViperEnv()No changes needed here.
409-410:✅ Verification successful
Verify GitHub OIDC environment variable binding strategy.
The GitHub OIDC variables use the same string for both the viper key and environment variable name. Confirm this is the intended approach, as it differs from other bindings that use structured keys like "settings.xyz".
🏁 Script executed:
#!/bin/bash # Description: Check how GitHub OIDC environment variables are accessed in the codebase # Expected: Should find usage patterns that confirm the binding strategy echo "=== Searching for GitHub OIDC environment variable usage ===" rg -A 3 -B 3 "ACTIONS_ID_TOKEN_REQUEST" echo "=== Searching for viper.GetString usage with these keys ===" rg -A 2 -B 2 'viper\.GetString.*ACTIONS_ID_TOKEN'Length of output: 19532
No action required: GitHub OIDC env var binding is intentional and consistent.
I’ve verified that
bindEnvGlobal("ACTIONS_ID_TOKEN_REQUEST_URL", "ACTIONS_ID_TOKEN_REQUEST_URL")and its TOKEN counterpart are used throughout—including ingetGitHubOIDCToken()and all related tests—so the key names matching the env var names is by design.
pkg/config/load.go
Outdated
| // setEnvGlobal configures environment variable bindings for the global viper instance. | ||
| func setEnvGlobal() { | ||
| // Use the global viper instance functions | ||
| bindEnvGlobal("settings.github_token", "GITHUB_TOKEN") | ||
| bindEnvGlobal("settings.inject_github_token", "ATMOS_INJECT_GITHUB_TOKEN") | ||
| bindEnvGlobal("settings.atmos_github_token", "ATMOS_GITHUB_TOKEN") | ||
|
|
||
| bindEnvGlobal("settings.bitbucket_token", "BITBUCKET_TOKEN") | ||
| bindEnvGlobal("settings.atmos_bitbucket_token", "ATMOS_BITBUCKET_TOKEN") | ||
| bindEnvGlobal("settings.inject_bitbucket_token", "ATMOS_INJECT_BITBUCKET_TOKEN") | ||
| bindEnvGlobal("settings.bitbucket_username", "BITBUCKET_USERNAME") | ||
|
|
||
| bindEnvGlobal("settings.gitlab_token", "GITLAB_TOKEN") | ||
| bindEnvGlobal("settings.inject_gitlab_token", "ATMOS_INJECT_GITLAB_TOKEN") | ||
| bindEnvGlobal("settings.atmos_gitlab_token", "ATMOS_GITLAB_TOKEN") | ||
|
|
||
| bindEnvGlobal("settings.terminal.pager", "ATMOS_PAGER", "PAGER") | ||
| bindEnvGlobal("settings.terminal.no_color", "ATMOS_NO_COLOR", "NO_COLOR") | ||
|
|
||
| // Atmos Pro | ||
| bindEnvGlobal(AtmosProBaseUrlEnvVarName, AtmosProBaseUrlEnvVarName) | ||
| bindEnvGlobal(AtmosProEndpointEnvVarName, AtmosProEndpointEnvVarName) | ||
| bindEnvGlobal(AtmosProTokenEnvVarName, AtmosProTokenEnvVarName) | ||
| bindEnvGlobal(AtmosProWorkspaceIDEnvVarName, AtmosProWorkspaceIDEnvVarName) | ||
|
|
||
| // GitHub OIDC | ||
| bindEnvGlobal("ACTIONS_ID_TOKEN_REQUEST_URL", "ACTIONS_ID_TOKEN_REQUEST_URL") | ||
| bindEnvGlobal("ACTIONS_ID_TOKEN_REQUEST_TOKEN", "ACTIONS_ID_TOKEN_REQUEST_TOKEN") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to eliminate code duplication.
The environment variable bindings are duplicated between setEnv and setEnvGlobal. This creates maintenance overhead where changes need to be made in two places.
Consider extracting the bindings into a shared function or data structure:
+// envBindings returns the standard environment variable bindings
+func envBindings() [][]string {
+ return [][]string{
+ {"settings.github_token", "GITHUB_TOKEN"},
+ {"settings.inject_github_token", "ATMOS_INJECT_GITHUB_TOKEN"},
+ // ... rest of bindings
+ }
+}
+
func setEnv(v *viper.Viper) {
- bindEnv(v, "settings.github_token", "GITHUB_TOKEN")
- // ... other bindings
+ for _, binding := range envBindings() {
+ bindEnv(v, binding...)
+ }
}
func setEnvGlobal() {
- bindEnvGlobal("settings.github_token", "GITHUB_TOKEN")
- // ... other bindings
+ for _, binding := range envBindings() {
+ bindEnvGlobal(binding...)
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/config/load.go from lines 383 to 411, the environment variable bindings
in setEnvGlobal are duplicated in setEnv, causing maintenance overhead. Refactor
by extracting the common environment variable bindings into a shared function or
data structure that both setEnv and setEnvGlobal can call or iterate over,
eliminating duplication and centralizing the binding logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/config/load.go (1)
377-418: Excellent global Viper environment setup implementation.The new global environment setup functions follow the established patterns and ensure environment variables are available through the global Viper instance. This addresses the standardization objectives mentioned in the PR.
Minor note: The comments could use periods for consistency with the existing code style.
-// SetupGlobalViperEnv configures the global viper instance to read environment variables -// This ensures that global viper.GetString() calls work properly throughout the application +// SetupGlobalViperEnv configures the global viper instance to read environment variables. +// This ensures that global viper.GetString() calls work properly throughout the application.-// setEnvGlobal configures environment variable bindings for the global viper instance +// setEnvGlobal configures environment variable bindings for the global viper instance.-// bindEnvGlobal binds environment variables to the global viper instance +// bindEnvGlobal binds environment variables to the global viper instance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/root.go(1 hunks)pkg/config/load.go(2 hunks)pkg/pro/api_client.go(1 hunks)pkg/pro/api_client_get_github_oidc_token_test.go(4 hunks)pkg/pro/api_client_test.go(7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
pkg/config/load.go (1)
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
pkg/pro/api_client_test.go (1)
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
🧬 Code Graph Analysis (2)
cmd/root.go (1)
pkg/config/load.go (1)
SetupGlobalViperEnv(379-381)
pkg/config/load.go (1)
pkg/config/const.go (4)
AtmosProBaseUrlEnvVarName(89-89)AtmosProEndpointEnvVarName(90-90)AtmosProTokenEnvVarName(91-91)AtmosProWorkspaceIDEnvVarName(92-92)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: Summary
🔇 Additional comments (11)
pkg/config/load.go (1)
104-108: Good addition of Atmos Pro environment variable bindings.This properly integrates the Atmos Pro configuration with the existing environment variable binding pattern.
pkg/pro/api_client.go (1)
247-248: Proper alignment with global environment variable bindings.The switch from nested Viper keys to direct environment variable keys aligns well with the global Viper setup introduced in
pkg/config/load.go. This simplifies the configuration access pattern.cmd/root.go (1)
138-140: Perfect placement for global environment variable setup.Calling
cfg.SetupGlobalViperEnv()at the start of the Execute function ensures environment variables are properly bound before any configuration access throughout the application. The comments clearly explain the timing requirement.pkg/pro/api_client_get_github_oidc_token_test.go (1)
39-42: Excellent test consistency with main application changes.The updated environment variable bindings in tests now match the production code pattern, ensuring consistent behavior between test and runtime environments. The comments clearly indicate alignment with the main application approach.
Also applies to: 89-92, 122-125, 147-150
pkg/pro/api_client_test.go (7)
61-67: Solid standardization approach.The explicit environment variable bindings align perfectly with the codebase standard of using Viper instead of direct
os.Getenvcalls. This ensures tests mirror the main application's configuration setup.
87-93: Consistent implementation across test cases.Same binding pattern applied correctly, maintaining test isolation while following the established Viper configuration approach.
145-151: Proper test environment setup.The bindings ensure this test case has the same environment variable configuration that the main application provides globally.
176-182: Well-maintained consistency.Fourth test case follows the same reliable pattern, ensuring predictable environment variable handling.
204-210: Continues the solid pattern.Environment variable binding setup remains consistent and correct for this failure case scenario.
241-247: Reliable test setup maintained.The binding approach ensures this token exchange failure test operates with the same configuration foundation as production code.
263-269: Complete standardization achieved.Final test case completes the consistent application of Viper environment variable bindings across all scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/pro/api_client_test.go (1)
51-347: Consider refactoring to reduce test setup duplication.The test cases contain significant duplication in AtmosConfiguration struct creation. Static analysis correctly identifies this pattern.
Consider extracting a helper function to reduce duplication:
+func createTestAtmosConfig() *schema.AtmosConfiguration { + return &schema.AtmosConfiguration{ + Settings: schema.AtmosSettings{ + Pro: schema.ProSettings{ + BaseURL: os.Getenv("ATMOS_PRO_BASE_URL"), + Endpoint: os.Getenv("ATMOS_PRO_ENDPOINT"), + Token: os.Getenv("ATMOS_PRO_TOKEN"), + WorkspaceID: os.Getenv("ATMOS_PRO_WORKSPACE_ID"), + GithubOIDC: schema.GithubOIDCSettings{ + RequestURL: os.Getenv("ACTIONS_ID_TOKEN_REQUEST_URL"), + RequestToken: os.Getenv("ACTIONS_ID_TOKEN_REQUEST_TOKEN"), + }, + }, + }, + } +}Then replace the repeated struct creation with
atmosConfig := createTestAtmosConfig().🧰 Tools
🪛 golangci-lint (1.64.8)
[error] 226-226: 226-260 lines are duplicate of
pkg/pro/api_client_test.go:313-346(dupl)
[error] 313-313: 313-346 lines are duplicate of
pkg/pro/api_client_test.go:226-260(dupl)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/exec/describe_affected.go(1 hunks)internal/exec/pro.go(4 hunks)pkg/config/load.go(2 hunks)pkg/pro/api_client.go(3 hunks)pkg/pro/api_client_get_github_oidc_token_test.go(4 hunks)pkg/pro/api_client_test.go(3 hunks)pkg/schema/schema.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/schema/schema.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/config/load.go
- pkg/pro/api_client_get_github_oidc_token_test.go
- pkg/pro/api_client.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/exec/describe_affected.go (1)
pkg/pro/api_client.go (1)
NewAtmosProAPIClientFromEnv(67-107)
🪛 golangci-lint (1.64.8)
pkg/pro/api_client_test.go
[error] 226-226: 226-260 lines are duplicate of pkg/pro/api_client_test.go:313-346
(dupl)
[error] 313-313: 313-346 lines are duplicate of pkg/pro/api_client_test.go:226-260
(dupl)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: prepare
- GitHub Check: PR Semver Labels
- GitHub Check: Analyze (go)
- GitHub Check: autofix
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (10)
internal/exec/describe_affected.go (1)
227-227: LGTM - Function signature correctly updated.The addition of
d.atmosConfigparameter aligns with the updatedNewAtmosProAPIClientFromEnvfunction signature that now requires the AtmosConfiguration context.internal/exec/pro.go (5)
11-11: Import addition looks good.Adding the schema package import is necessary for the AtmosConfiguration type usage.
16-20: Struct update correctly adds configuration context.The addition of
AtmosConfig schema.AtmosConfigurationfield enables passing structured configuration to the API client instead of relying on environment variables.
67-70: Configuration field properly initialized.The
AtmosConfig: atmosConfigassignment correctly populates the struct with the parsed configuration context.
151-151: API client call correctly updated.The addition of
&a.AtmosConfigparameter matches the updated function signature and provides the necessary configuration context.
193-193: Consistent update for unlock command.Same correct pattern applied to the unlock command's API client creation.
pkg/pro/api_client_test.go (4)
14-14: Schema import addition is correct.Adding the schema package import supports the new AtmosConfiguration struct usage throughout the tests.
67-80: Test refactoring correctly implements structured configuration.The explicit construction of
AtmosConfigurationstruct replaces the previous Viper-based approach, providing better test isolation and clarity.Also applies to: 82-82
107-120: Consistent pattern for default values test.Good coverage of the default URL/endpoint behavior with explicit configuration.
Also applies to: 122-122
162-175: OIDC flow test properly updated.The test maintains coverage of the OIDC token exchange scenario with the new configuration approach.
Also applies to: 177-177
what
Move the Atmos Pro config to the Atmos settings
why
To follow the same canonical pattern we are using to read settings for other parts of the Atmos application.
Summary by CodeRabbit
Summary by CodeRabbit