refactor loading and checking of atmos config #869
Conversation
mcalhoun
added
the
no-release
📝 WalkthroughWalkthroughThis pull request introduces comprehensive modifications to the command execution flow across multiple files in the Changes
Suggested labels
Suggested reviewers
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
|
![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
🔭 Outside diff range comments (4)
cmd/terraform.go (1)
Line range hint
29-29: Consider using the context-stored config for error handling.Currently using an empty AtmosConfiguration for error logging. For consistency with the PR's objectives, consider using the context-stored config here as well.
- u.LogErrorAndExit(schema.AtmosConfiguration{}, err) + u.LogErrorAndExit(atmosConfig, err)Also applies to: 42-42
cmd/vendor_pull.go (1)
Line range hint
25-25: Consider using the context-stored config for error handling.Similar to terraform.go, consider using the retrieved atmosConfig for error logging instead of an empty configuration.
- u.LogErrorAndExit(schema.AtmosConfiguration{}, err) + u.LogErrorAndExit(atmosConfig, err)cmd/version.go (1)
Line range hint
28-28: Use context-stored config for error handling.For consistency, use the context-stored config for error logging here as well.
- u.LogErrorAndExit(schema.AtmosConfiguration{}, err) + atmosConfig := cmd.Context().Value(contextKey("atmos_config")).(schema.AtmosConfiguration) + u.LogErrorAndExit(atmosConfig, err)cmd/cmd_utils.go (1)
Line range hint
367-383: Add nil pointer check for atmosConfigThe function now correctly accepts a pointer to optimize memory usage, but we should add a nil check before dereferencing the pointer to prevent potential panics.
func checkAtmosConfig(atmosConfig *schema.AtmosConfiguration, opts ...AtmosValidateOption) { + if atmosConfig == nil { + os.Exit(1) + } vCfg := &ValidateConfig{ CheckStack: true, // Default value true to check the stack }
🧹 Nitpick comments (5)
cmd/root.go (2)
86-86: Check for potential error-handling or logging gap
There is a blank line added at 86, probably just whitespace. If this space was introduced accidentally, consider removing for cleanliness.
99-101: Ensure no duplication of config checks
Invoking checkAtmosConfig(&atmosConfig) is logical here, although it duplicates logic found in other commands if repeated. Consider centralizing or gating additional checks to remain DRY (Don’t Repeat Yourself).cmd/helmfile.go (1)
37-38: Pointer usage for config update checks
Passing a pointer to CheckForAtmosUpdateAndPrintMessage avoids an unnecessary copy. This is a solid move for performance and clarity, particularly if the function modifies or logs from the config object.cmd/version.go (1)
Line range hint
34-44: Simplify GitHub release check error handling.The error handling flow can be simplified by removing redundant checks.
latestReleaseTag, err := u.GetLatestGitHubRepoRelease("cloudposse", "atmos") -if err == nil && latestReleaseTag != "" { - if err != nil { - u.LogWarning(schema.AtmosConfiguration{}, fmt.Sprintf("Failed to check for updates: %v", err)) - return - } - if latestReleaseTag == "" { - u.LogWarning(schema.AtmosConfiguration{}, "No release information available") - return - } +if err != nil { + u.LogWarning(atmosConfig, fmt.Sprintf("Failed to check for updates: %v", err)) + return +} +if latestReleaseTag == "" { + u.LogWarning(atmosConfig, "No release information available") + return +}cmd/cmd_utils.go (1)
Line range hint
430-475: Standardize error handling styleWhile the pointer usage is correct, there's inconsistency in error handling. Some errors use
LogWarningwhile others directly return. Consider standardizing the approach.func CheckForAtmosUpdateAndPrintMessage(atmosConfig *schema.AtmosConfiguration) { if !atmosConfig.Version.Check.Enabled { return } cacheCfg, err := cfg.LoadCache() if err != nil { - u.LogWarning(*atmosConfig, fmt.Sprintf("Could not load cache: %s", err)) - return + return // Silent return for cache issues as they're non-critical }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
cmd/atlantis_generate_repo_config.go(0 hunks)cmd/cmd_utils.go(6 hunks)cmd/describe_affected.go(0 hunks)cmd/describe_component.go(0 hunks)cmd/describe_dependents.go(0 hunks)cmd/describe_stacks.go(0 hunks)cmd/helmfile.go(1 hunks)cmd/helmfile_generate_varfile.go(0 hunks)cmd/list_components.go(1 hunks)cmd/list_stacks.go(1 hunks)cmd/pro_lock.go(0 hunks)cmd/pro_unlock.go(0 hunks)cmd/root.go(4 hunks)cmd/terraform.go(1 hunks)cmd/terraform_generate_backend.go(0 hunks)cmd/terraform_generate_backends.go(0 hunks)cmd/terraform_generate_varfile.go(0 hunks)cmd/terraform_generate_varfiles.go(0 hunks)cmd/validate_component.go(0 hunks)cmd/validate_stacks.go(0 hunks)cmd/vendor_diff.go(0 hunks)cmd/vendor_pull.go(1 hunks)cmd/version.go(1 hunks)
💤 Files with no reviewable changes (15)
- cmd/pro_unlock.go
- cmd/helmfile_generate_varfile.go
- cmd/describe_dependents.go
- cmd/pro_lock.go
- cmd/describe_affected.go
- cmd/validate_stacks.go
- cmd/terraform_generate_backends.go
- cmd/terraform_generate_varfile.go
- cmd/atlantis_generate_repo_config.go
- cmd/validate_component.go
- cmd/terraform_generate_varfiles.go
- cmd/describe_stacks.go
- cmd/vendor_diff.go
- cmd/terraform_generate_backend.go
- cmd/describe_component.go
🧰 Additional context used
📓 Learnings (2)
cmd/version.go (1)
Learnt from: Listener430
PR: cloudposse/atmos#844
File: cmd/version.go:34-44
Timestamp: 2024-12-13T15:28:13.630Z
Learning: In `cmd/version.go`, when handling the `--check` flag in the `versionCmd`, avoid using `CheckForAtmosUpdateAndPrintMessage(cliConfig)` as it updates the cache timestamp, which may not be desired in this context.
cmd/cmd_utils.go (2)
Learnt from: Listener430
PR: cloudposse/atmos#844
File: cmd/version.go:34-44
Timestamp: 2024-12-13T15:28:13.630Z
Learning: In `cmd/version.go`, when handling the `--check` flag in the `versionCmd`, avoid using `CheckForAtmosUpdateAndPrintMessage(cliConfig)` as it updates the cache timestamp, which may not be desired in this context.
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-11-12T05:52:05.088Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
🔇 Additional comments (9)
cmd/root.go (4)
4-4: Use of context for concurrency and cross-cutting concerns
Importing the "context" package is a great choice for passing data across function boundaries and handling potential concurrency needs. Ensure consistent usage of the context throughout the codebase, especially for any blocking or potentially long-running tasks that might benefit from cancellation or timeout.
22-23: Good practice: dedicated context key type
Defining a distinct contextKey type helps avoid key collisions and clarifies ownership of context values. This is a neat, maintainable approach.
90-90: Initialization with explicit argument
Passing "true" to InitCliConfig is a design decision that might alter initialization behavior. Verify that all callers rely on or expect this new configuration path.
102-105: Excellent use of context
Storing atmosConfig in the context fosters a more composable design. Confirm that all child commands are responsibly retrieving and handling the data, minimizing the risk of nil type assertions.
cmd/list_components.go (1)
24-24: Context-based config retrieval
Using cmd.Context().Value(...) to obtain atmosConfig streamlines initialization. Verify that type assertions never fail; consider checking for nil or errors to avoid runtime panics.
cmd/list_stacks.go (1)
25-25: Context-based atmosConfig
Grabbing atmosConfig from the context is consistent with the new pattern. Great improvement in code cohesion. Ensure that the same approach is used in all relevant commands for uniformity.
cmd/terraform.go (1)
36-37: LGTM! Proper usage of command context for config retrieval.
The change aligns with the PR's objective of using the context-stored config instead of loading it repeatedly.
cmd/vendor_pull.go (1)
20-21: LGTM! Proper implementation of pointer-based config check.
The changes align perfectly with the PR objectives by:
- Using context-stored config
- Passing config pointer to checkAtmosConfig
- Including stack validation as needed
cmd/version.go (1)
56-57:
Based on previous feedback, using CheckForAtmosUpdateAndPrintMessage here might unintentionally update the cache timestamp. Consider if this is the desired behavior when running the version command.
mcalhoun
changed the title
This reverts commit 80058a8. Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
|
These changes were released in v1.131.0. |
what
checkAtmosConfigis called on the root command and then only called where necessarycheckAtmosConfigtakes a pointer to the atmos config, so it doesn't load it inline and doesn't have to create another copy in memoryCheckForAtmosUpdateAndPrintMessageto take a pointer to the atmos config so it doesn't have to create another copy in memorywhy
To improve performance
references
Created this PR to replace #861 because significant refactors occurred since #861 was created, but before it was merged.
Summary by CodeRabbit
New Features
Bug Fixes
Chores