revert "refactor loading and checking of atmos config" #885
Conversation
This reverts commit 80058a8.
mcalhoun
added
no-release
📝 WalkthroughWalkthroughThe pull request introduces a systematic enhancement to the Atmos CLI configuration validation process across multiple command files. The changes primarily involve adding a Changes
Possibly related PRs
Suggested Reviewers
The changes represent a robust approach to standardizing configuration validation across the Atmos CLI, ensuring consistent error handling and configuration checks. Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 0
🧹 Nitpick comments (2)
cmd/helmfile.go (1)
40-41: Great AdditionCalling checkAtmosConfig() ensures configuration validity before proceeding with Helmfile logic.
Could you consider adding a short inline comment indicating why configuration verification is mandatory at this phase?
cmd/root.go (1)
21-21: Consider scoping the global variable more narrowly.While adding a global variable for storing configuration simplifies the code, it might make concurrency or reusability more difficult in the future.
📜 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(1 hunks)cmd/cmd_utils.go(6 hunks)cmd/describe_affected.go(1 hunks)cmd/describe_component.go(1 hunks)cmd/describe_dependents.go(1 hunks)cmd/describe_stacks.go(1 hunks)cmd/helmfile.go(1 hunks)cmd/helmfile_generate_varfile.go(1 hunks)cmd/list_components.go(2 hunks)cmd/list_stacks.go(2 hunks)cmd/pro_lock.go(1 hunks)cmd/pro_unlock.go(1 hunks)cmd/root.go(3 hunks)cmd/terraform.go(2 hunks)cmd/terraform_generate_backend.go(1 hunks)cmd/terraform_generate_backends.go(1 hunks)cmd/terraform_generate_varfile.go(1 hunks)cmd/terraform_generate_varfiles.go(1 hunks)cmd/validate_component.go(1 hunks)cmd/validate_stacks.go(1 hunks)cmd/vendor_diff.go(1 hunks)cmd/vendor_pull.go(1 hunks)cmd/version.go(1 hunks)
🧰 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 (38)
cmd/list_components.go (3)
7-7: Import usage looks fine
The newly added import for config aligns with your existing usage.
23-25: Configuration check is well-placed
Calling checkAtmosConfig() before other operations is good for ensuring a valid state.
28-33: Initialization logic looks tidy
Assigning configAndStacksInfo and handling the error quickly keeps things straightforward.
cmd/list_stacks.go (3)
7-7: Import usage matches pattern
The config import here is consistent with other commands.
24-26: Proactive validation
Placing checkAtmosConfig() near the start helps prevent deeper command failures.
29-34: Error handling is effective
Exiting early on a config error prevents further issues down the line.
cmd/pro_unlock.go (1)
18-20: Clear config check
Adding checkAtmosConfig() improves consistency with other commands.
cmd/validate_stacks.go (1)
20-22: Validation happens early
Short-circuiting on bad config avoids wasted effort in validating stacks.
cmd/pro_lock.go (1)
18-20: Configuration validation is consistent
This check is in line with the approach used across other commands.
cmd/terraform_generate_backend.go (1)
18-19: Looks good
This reinstatement of configuration checking is aligned with the rest of the code. No issues found.
cmd/vendor_diff.go (1)
18-19: Smooth integration
Calling checkAtmosConfig() up front is consistent. No concerns noted.
cmd/helmfile_generate_varfile.go (1)
18-19: All set
The addition of checkAtmosConfig() maintains consistency with other commands.
cmd/terraform_generate_varfile.go (1)
18-19: No concerns
Pre-flight config checks are useful. This is straightforward and compliant with existing patterns.
cmd/helmfile.go (1)
37-37: Seems Good
The updated function call uses the local variable directly, simplifying the code.
cmd/describe_dependents.go (1)
19-20: Wise Check
Invoking checkAtmosConfig() early helps avoid misconfiguration issues. The approach is consistent with other commands.
cmd/terraform.go (3)
39-39: Slight Improvement
Switching away from taking a pointer reduces confusion about config mutability. Nicely done.
42-43: Essential Configuration Validation
Ensures config correctness before executing terraform commands. This keeps behavior consistent.
20-21: Check Intent
It looks like this segment is commented out. Could you clarify if this is intentional or if you meant for the check to remain active?
✅ Verification successful
Let me investigate this by checking the codebase for more context about this commented function.
This will help us understand:
- If the function exists elsewhere in the codebase
- How Atmos configuration is handled in general
- The full context of the terraform.go file where this comment appears
Let me check the content of the terraform.go file specifically to understand the context of this commented line.
Let me check if this check is performed elsewhere in the same file, as it might have been moved to a different location.
The commented line is intentional and safe to ignore
The check is actually performed later in the same function. The commented line at the beginning was likely moved down to be executed after the help check, as we can see the active checkAtmosConfig() call is present after the help check but before executing terraform. This is a better logical flow as it prevents unnecessary configuration checks when only help is requested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for checkAtmosConfig function definition and usage
rg "checkAtmosConfig" -A 5
# Look for any Atmos configuration related code
rg "Atmos.*config" -i
# Check the specific file content
cat cmd/terraform.go
Length of output: 67538
Script:
#!/bin/bash
# Get the specific content around the commented line
rg -A 10 -B 10 "//checkAtmosConfig\(\)" cmd/terraform.go
Length of output: 873
Script:
#!/bin/bash
# Get the full content of terraform.go to see where the check might be performed
cat cmd/terraform.go | grep -A 50 "Run: func"
Length of output: 1178
cmd/vendor_pull.go (1)
20-20: Thoughtful Enhancement
Including WithStackValidation(...) is a good step toward ensuring a proper config check when the --stack flag is used.
cmd/describe_component.go (1)
18-19: Validate configuration before command execution
Performing the Atmos configuration check here ensures errors are caught early. Keep up the consistency across commands.
cmd/version.go (1)
56-56: Avoid caching updates if it’s not desired
Consider the learning about not updating the timestamp when using the --check flag. Confirm whether caching updates are actually needed in this context.
✅ Verification successful
Let me gather more information about the update checking functionality to verify this concern.
Let me check how the --check flag is handled in the version command.
Let me check the version command implementation specifically.
Cache update behavior is correct and intentional
The code is properly handling caching updates. When the --check flag is used, it performs a direct version check without caching (lines 39-53). The CheckForAtmosUpdateAndPrintMessage is only called when --check flag is not used (line 56), which is the intended behavior for normal version checks that should respect the caching configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Let's first look at the CheckForAtmosUpdateAndPrintMessage function implementation
ast-grep --pattern 'func CheckForAtmosUpdateAndPrintMessage($_) {
$$$
}'
# Also search for any check flag related logic
rg -A 5 "check.*flag"
# Look for update caching related code
rg -i "cache.*update"
Length of output: 5493
Script:
#!/bin/bash
# Look at the version command implementation to see how the --check flag is handled
ast-grep --pattern 'func newVersionCmd($_) {
$$$
}'
# Also check for any version check configuration handling
rg -A 5 "Version.*Check"
Length of output: 1713
Script:
#!/bin/bash
# Look at the content of the version.go file
cat cmd/version.go
Length of output: 1825
cmd/validate_component.go (1)
24-25: Configuration check for consistent validation
Including the config check is a good addition. It helps maintain uniform validation flow across the application.
cmd/describe_stacks.go (1)
18-19: Ensuring Atmos config correctness
Validating configuration at the start helps avoid downstream errors. Good practice to keep it consistent with other commands.
cmd/terraform_generate_varfiles.go (1)
18-19: Good addition of early config check
Verifying the atmos configuration at the beginning helps ensure correctness before proceeding with command execution.
cmd/terraform_generate_backends.go (1)
18-19: Checking config upfront is a solid approach
Validating configuration at the outset safeguards against unexpected issues during backend generation.
cmd/describe_affected.go (1)
18-20: Early configuration validation boosts reliability
Ensuring the atmos configuration is in order first helps prevent errors further along in the workflow.
cmd/atlantis_generate_repo_config.go (1)
18-19: Ensuring config correctness is beneficial
Performing the config check at the outset supports consistent behavior and lowers the chance of run-time errors.
cmd/root.go (2)
48-49: Clear separation of concerns.
Invoking checkAtmosConfig() here ensures the configuration is validated before proceeding with the main command logic.
89-90: Re-initializing the global config.
The approach reinitializes 'atmosConfig' in Execute(). Ensure threading is not a concern since this is a CLI. Otherwise, a more local scoping might be preferable.
cmd/cmd_utils.go (9)
367-367: Flexible validation options.
Allowing optional validation checks via AtmosValidateOption is a neat extension point for future expansions.
385-385: Graceful exit.
Printing a helpful message and exiting is user-friendly. No suggestions.
444-444: Helpful warning.
Logging a warning rather than silently failing is beneficial for troubleshooting.
457-457: Consistent logging.
Maintaining consistent log levels for errors ensures users can easily filter or parse logs.
462-462: Fallback messaging.
Providing a fallback message (like "No release information available") is a good practice to keep the user informed in case of unavailable data.
478-478: Cache update failure is now logged.
This is helpful for diagnosing any issues with writing to the cache.
377-381: Global variable overshadowing possibility.
Be mindful that 'atmosConfig, err := ...' might overshadow the global variable if declared in a different scope. Confirm that both references indeed refer to the same variable.
435-435: Non-pointer signature.
Changing the parameter type to a value instead of a pointer can simplify usage. Verify that large structures aren’t inadvertently copied if performance is critical.
✅ Verification successful
Let me gather information about the structure size and its usage to verify this concern.
Based on the inspection of AtmosConfiguration, I can now make a definitive assessment. The struct contains numerous fields but most are pointers or small value types (strings, bools). The largest embedded structs are well-defined configurations that are meant to be passed around. The struct is used extensively throughout the codebase as a value type, suggesting it was designed to be copied efficiently.
Using value semantics is appropriate here.
- The struct is primarily composed of configuration data that needs to be read but rarely modified
- Most fields are small value types or pointers
- The codebase consistently uses value semantics for this type across numerous function calls
- No performance issues are apparent from the widespread value passing of this configuration struct
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check the definition of AtmosConfiguration to understand its size
ast-grep --pattern 'type AtmosConfiguration struct {
$$$
}'
# Look for other usages of AtmosConfiguration to see how it's typically passed
rg "AtmosConfiguration" -A 2 -B 2
Length of output: 66327
485-485: Potential cache update on help command.
By calling CheckForAtmosUpdateAndPrintMessage() here, the cache timestamp may be updated even when displaying help. Evaluate whether this behavior is desired or should be conditional.
✅ Verification successful
Let me gather information about the cache update behavior and the help command implementation.
Let me check how this update check is integrated with the help command.
Cache update behavior on help command is intentional and properly implemented.
The cache update behavior during help command execution is an intentional design choice, implemented through customHelpMessageToUpgradeToAtmosLatestRelease. The function has built-in safeguards:
- Version checking can be disabled via config (
atmosConfig.Version.Check.Enabled) - Cache updates only occur based on frequency checks (
ShouldCheckForUpdates) - The behavior is consistent with the tool's update notification strategy across commands
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# First, let's find the implementation of CheckForAtmosUpdateAndPrintMessage
ast-grep --pattern 'func CheckForAtmosUpdateAndPrintMessage($$$)'
# Let's also look for help command related code
rg -A 5 "help command"
# Search for cache update related code
rg -i "cache.*update"
Length of output: 3403
Script:
#!/bin/bash
# Let's look at the context where CheckForAtmosUpdateAndPrintMessage is called
rg -B 5 -A 5 "CheckForAtmosUpdateAndPrintMessage"
# Also check the root command implementation
ast-grep --pattern 'func Execute($$$)'
Length of output: 7258
mcalhoun
deleted the
revert-869-chore/load-atmos-config-in-root-command
branch
|
These changes were released in v1.131.0. |
what
Reverts #869
why
Because this PR introduced some regressions with help commands.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Documentation