Implement Atmos Version Check Configuration #844
Conversation
Listener430
added
enhancement
|
Update documentation here: https://atmos.tools/cli/ Also, here: https://atmos.tools/cli/commands/version |
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a comprehensive version checking mechanism for the Atmos CLI. A new configuration section in Changes
Assessment against linked issues
Possibly related PRs
Suggested Reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
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: 6
🧹 Outside diff range and nitpick comments (3)
pkg/config/cache.go (1)
66-80: ConsolidateSaveCachefunctions for clarityThere are two functions,
SaveCache2andSaveCache, performing similar tasks. Merging them or renaming for clarity could improve maintainability.Also applies to: 82-94
cmd/root.go (1)
21-21: Consider scoping the configuration more tightlyWhile this follows the existing pattern, consider passing the configuration as a parameter to functions that need it, rather than relying on a global variable. This would improve testability and make dependencies more explicit.
pkg/config/config.go (1)
85-91: Document the version check configuration parametersThe version check configuration looks good, but please add comments explaining:
- The units for the timeout value (milliseconds?)
- Valid values for the frequency field
- The implications of these settings
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
atmos.yaml(1 hunks)cmd/cmd_utils.go(2 hunks)cmd/helmfile.go(1 hunks)cmd/root.go(2 hunks)cmd/terraform.go(1 hunks)go.mod(1 hunks)pkg/config/cache.go(1 hunks)pkg/config/config.go(1 hunks)pkg/config/utils.go(1 hunks)pkg/schema/schema.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/config/cache.go
39-39: Error return value of lock.Unlock is not checked
(errcheck)
🔇 Additional comments (5)
pkg/config/cache.go (1)
96-108: 🛠️ Refactor suggestion
Add a warning for unsupported frequency values
We should inform users when an unsupported frequency is provided, and we're defaulting to 'daily'.
You might add a warning like:
default:
interval = 86400 // default daily
+ fmt.Printf("Warning: Unsupported frequency '%s', defaulting to 'daily'\n", frequency)cmd/root.go (1)
89-95: LGTM! Error handling improvements look solid.
The separation of error handling for version commands vs. other commands is well implemented. The code properly handles the case where configuration is not found but version information is still needed.
Let's verify all version-related command handlers are consistent:
✅ Verification successful
Version command handling is properly implemented and consistent
The verification confirms that:
isVersionCommand()correctly checks for the "version" argument- Version command is properly registered with cobra framework
- Error handling in root.go appropriately uses
isVersionCommand()to handle configuration errors differently for version commands
The implementation maintains consistency across all version-related functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for version command handlers to ensure consistent error handling
rg -p "isVersionCommand|version command" cmd/
Length of output: 179
Script:
#!/bin/bash
# Let's check the implementation of isVersionCommand and its usage context
ast-grep --pattern 'func isVersionCommand() bool {
$$$
}'
# Also check the version command registration
rg -p "version.*Command" cmd/ -A 5
Length of output: 492
go.mod (1)
19-19: LGTM! Good choice for file locking.
The gofrs/flock library is a solid choice for implementing file locking in the cache mechanism. It's well-maintained and widely used.
atmos.yaml (1)
319-323: LGTM! Well-structured version check configuration.
The configuration follows best practices with sensible defaults:
- Version check enabled by default
- 1-second timeout is reasonable
- Daily frequency strikes a good balance
pkg/schema/schema.go (1)
29-29: LGTM! Well-designed schema for version configuration.
The schema changes are clean and follow Go best practices:
- Proper struct tags for serialization
- Optional fields marked with
omitempty - Clear separation of concerns between
VersionandVersionChecktypes
Also applies to: 131-139
Listener430
force-pushed
the
DEV-2815
branch
from
fe80497 to
0f79d1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
pkg/config/cache.go (1)
96-108: Enhance update frequency validationThe function should provide better feedback for unsupported intervals.
func ShouldCheckForUpdates(lastChecked int64, frequency string) bool { now := time.Now().Unix() var interval int64 switch frequency { case "daily": interval = 86400 case "weekly": interval = 604800 default: + u.LogWarning(schema.CliConfiguration{}, fmt.Sprintf("Unsupported update frequency '%s', defaulting to daily", frequency)) interval = 86400 // default daily } return now-lastChecked >= interval }cmd/root.go (2)
89-96: Improve error handling specificityThe error handling could be more specific about the type of configuration error encountered.
var initErr error cliConfig, initErr = cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false) if initErr != nil && !errors.Is(initErr, cfg.NotFound) { if isVersionCommand() { - u.LogTrace(schema.CliConfiguration{}, fmt.Sprintf("warning: CLI configuration 'atmos.yaml' file not found. Error: %s", initErr)) + u.LogTrace(schema.CliConfiguration{}, fmt.Sprintf("warning: Failed to initialize CLI configuration. Error: %s", initErr)) } else { u.LogErrorAndExit(schema.CliConfiguration{}, initErr) }
99-106: Consider deferring help function setupThe help function setup could be deferred until after all initialization is complete to ensure proper configuration state.
Consider moving the help function setup after the custom commands processing to ensure all configuration is properly loaded.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
atmos.yaml(1 hunks)cmd/cmd_utils.go(2 hunks)cmd/helmfile.go(1 hunks)cmd/root.go(2 hunks)cmd/terraform.go(1 hunks)go.mod(1 hunks)pkg/config/cache.go(1 hunks)pkg/config/config.go(1 hunks)pkg/config/utils.go(1 hunks)pkg/schema/schema.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- go.mod
- pkg/config/config.go
- atmos.yaml
- cmd/helmfile.go
- pkg/config/utils.go
- pkg/schema/schema.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/config/cache.go
39-39: Error return value of lock.Unlock is not checked
(errcheck)
🔇 Additional comments (7)
cmd/terraform.go (1)
39-39: Verify cliConfig initialization before use
The usage of cliConfig here could lead to runtime issues if not properly initialized.
#!/bin/bash
# Check if cliConfig is properly initialized before use
ast-grep --pattern 'cliConfig = $_'pkg/config/cache.go (1)
33-41:
Handle lock.Unlock() errors
The deferred unlock operation should handle potential errors.
- defer lock.Unlock()
+ defer func() {
+ if unlockErr := lock.Unlock(); unlockErr != nil {
+ err = errors.Wrap(unlockErr, "failed to release lock")
+ }
+ }()Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
39-39: Error return value of lock.Unlock is not checked
(errcheck)
cmd/cmd_utils.go (5)
10-10: LGTM! Import addition is appropriate.
The time package import is necessary for handling cache timestamps.
436-439: LGTM! Configuration check is well-implemented.
Early return if version checking is disabled provides clean control flow.
482-485: LGTM! Clean implementation of help message function.
Good separation of concerns by delegating version check to the new function.
470-479:
Use semantic versioning for version comparison.
String comparison of versions can lead to incorrect results. For example, "10.0.0" would be considered less than "9.0.0".
Previous suggestion still applies:
+import "github.com/hashicorp/go-version"
+latestVer, err := version.NewVersion(latestVersion)
+if err != nil {
+ u.LogTrace(cliConfig, fmt.Sprintf("Failed to parse latest version: %s", err))
+ return
+}
+currentVer, err := version.NewVersion(currentVersion)
+if err != nil {
+ u.LogTrace(cliConfig, fmt.Sprintf("Failed to parse current version: %s", err))
+ return
+}
-if latestVersion != currentVersion {
+if latestVer.GreaterThan(currentVer) {474-478:
Update cache timestamp regardless of version comparison.
The cache timestamp should be updated after every check, not just when versions differ, to properly respect the frequency settings.
Move the cache update outside the version comparison:
if latestVersion != currentVersion {
u.PrintMessageToUpgradeToAtmosLatestRelease(latestVersion)
}
+// Update the cache to mark the current timestamp
+cacheCfg.LastChecked = time.Now().Unix()
+if saveErr := cfg.SaveCache(cacheCfg); saveErr != nil {
+ u.LogWarning(cliConfig, fmt.Sprintf("Unable to save cache: %s", saveErr))
+}
|
On the version command, I think we should add a --check argument so the user can force the behavior to check for the latest version, even if the cache has a recent check. |
@osterman the version command is the only one that is not checking for cache right now , nor updates it, and displays the box all the time. So it should check for cache too and use --check to still display the box? |
|
Does the version command respect the version check config settings? We want to make sure that it does. |
@osterman thanks, sure, thank you so much for the clarifications. Have pushed the changes as requested. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/cmd_utils.go(2 hunks)cmd/version.go(2 hunks)pkg/config/utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/config/utils.go
🔇 Additional comments (3)
cmd/cmd_utils.go (3)
470-471: Use semantic versioning for version comparison
The current string comparison of versions could lead to incorrect results.
475-480: LGTM: Cache timestamp update implementation
Great implementation of updating the cache timestamp regardless of whether an update is found. This ensures proper frequency control of version checks.
432-481: LGTM: Well-structured version check implementation
The implementation is thorough and handles all edge cases:
- Respects configuration settings
- Handles cache loading errors gracefully
- Implements proper frequency control
- Updates cache timestamp appropriately
|
Does the
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
website/docs/cli/commands/version.mdx (2)
23-28: WARRIOR! Let's polish this documentation for clarity!The text needs some refinement to maintain our high standards:
-This will show the CLI version. The frequency with which the update box message in version is shown is configured in atmos.yaml. -There are two options daily and weekly. The default value is set to daily. -It is also possible to turn it off in atmos.yaml file by switching enable to false. -Or set an env variable ATMOS_VERSION_CHECK_ENABLED to false, -this overriders the config's settting. +This will show the CLI version. The frequency of the update box message is configured in the atmos.yaml file. +There are two options: daily and weekly. The default value is set to daily. +You can disable this feature in the atmos.yaml file by setting `enable: false`, +or by setting the environment variable `ATMOS_VERSION_CHECK_ENABLED=false`, +which overrides the config's setting.🧰 Tools
🪛 LanguageTool
[uncategorized] ~25-~25: Possible missing comma found.
Context: ...configured in atmos.yaml. There are two options daily and weekly. The default value is ...(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~27-~27: The singular determiner ‘this’ may not agree with the plural noun ‘overriders’. Did you mean “these”?
Context: ... ATMOS_VERSION_CHECK_ENABLED to false, this overriders the config's settting. ```...(THIS_NNS)
30-35: STAND STRONG! Let's enhance the --check flag documentation!The explanation could be more precise:
-The version command supports --check flag. -It will show the update box message irrespectively of the config settings. +The version command supports the `--check` flag, which forces the display of the update box message +regardless of your configuration settings.website/docs/cli/commands/help.mdx (1)
Line range hint
23-28: STRENGTHEN OUR DOCUMENTATION WITH A CONFIGURATION EXAMPLE!Both documentation files would benefit from showing an actual configuration example:
Consider adding this configuration example to both files:
settings: version: enabled: true # Enable/disable version check frequency: daily # Options: daily, weekly timeout: 1000 # Timeout in millisecondsThis will help warriors understand exactly how to configure these settings in their
atmos.yamlfile!Also applies to: 15-20
🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: A comma might be missing here.
Context: ...configured in atmos.yaml. There are two options daily and weekly. The default value is ...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ... It is also possible to turn it off in atmos.yaml file by switching enable to false. Or ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~19-~19: The singular determiner ‘this’ may not agree with the plural noun ‘overriders’. Did you mean “these”?
Context: ... ATMOS_VERSION_CHECK_ENABLED to false, this overriders the config's settting. ``...(THIS_NNS)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/docs/cli/commands/help.mdx(1 hunks)website/docs/cli/commands/version.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
website/docs/cli/commands/help.mdx
[uncategorized] ~17-~17: A comma might be missing here.
Context: ...configured in atmos.yaml. There are two options daily and weekly. The default value is ...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~18-~18: You might be missing the article “the” here.
Context: ... It is also possible to turn it off in atmos.yaml file by switching enable to false. Or ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[grammar] ~19-~19: The singular determiner ‘this’ may not agree with the plural noun ‘overriders’. Did you mean “these”?
Context: ... ATMOS_VERSION_CHECK_ENABLED to false, this overriders the config's settting. ``...
(THIS_NNS)
website/docs/cli/commands/version.mdx
[uncategorized] ~25-~25: Possible missing comma found.
Context: ...configured in atmos.yaml. There are two options daily and weekly. The default value is ...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~27-~27: The singular determiner ‘this’ may not agree with the plural noun ‘overriders’. Did you mean “these”?
Context: ... ATMOS_VERSION_CHECK_ENABLED to false, this overriders the config's settting. ```...
(THIS_NNS)
@osterman, thanks yes, it does, its just does not show the box because the timestamp is already there and time has not elapsed. atmos version --check does not intefere with the cache at all though (does not update timstamp either, if that ok?) |
|
Sounds like the right implementation. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
website/docs/cli/commands/version.mdx (4)
23-24: Consider enhancing the command description.The description could be more informative by mentioning that it displays both the current version and checks for updates when configured.
-This will show the CLI version. +This command displays the current Atmos CLI version and, when configured, checks for available updates.🧰 Tools
🪛 LanguageTool
[style] ~24-~24: Consider using an alternative to avoid wordiness and strengthen your wording.
Context: ... ``` This will show the CLI version. From time to time, Atmos will check for updates. The freq...(FROM_TIME_TO_TIME)
29-32: Improve formatting consistency in the numbered list.The list formatting could be more consistent, and the default value statement should be separated.
1. As an integer: Specify the number of seconds between checks (for example, 3600 for hourly checks). 2. As a duration with a suffix: Use a time suffix to indicate the interval (for example, `1m` for one minute, `5h` for five hours, or `2d` for two days). 3. As one of the predefined keywords: Choose from the following options: minute, hourly, daily, weekly, monthly, and yearly. The default is daily. -The default is to check `daily`, and if any unsupported values are passed this default will be used. + +If any unsupported values are provided, Atmos will default to checking `daily`.🧰 Tools
🪛 LanguageTool
[style] ~31-~31: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...r five hours, or2dfor two days). 3. As one of the predefined keywords: Choose ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
34-36: Improve readability of configuration options.The line wrapping makes this section harder to read. Consider restructuring for better clarity.
-It is also possible to turn off version checks in `atmos.yaml` by switching `enable` to `false`, -or by setting the `ATMOS_VERSION_CHECK_ENABLED` environment variable to `false`, which overrides -the config's settings. +Version checks can be disabled in two ways: + +1. Set `enable: false` in the `atmos.yaml` configuration file +2. Set the environment variable `ATMOS_VERSION_CHECK_ENABLED=false` (overrides the configuration file setting)
38-43: Consolidate the --check flag documentation.The flag documentation is split across multiple lines and could be more concise.
```shell atmos version --check-The version command supports a
--checkflag.
-This will force Atmos to check for a new version, irrespective of the configuration settings.
+Use the--checkflag to force an immediate version check, bypassing all configuration settings.</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: .coderabbit.yaml** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 595b08ff20cf9e086bd95f6812bdb0a1d7ec9bc6 and da2d692b132a4eeda3d94538c1d89c8ab00f1d99. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `website/docs/cli/commands/help.mdx` (1 hunks) * `website/docs/cli/commands/version.mdx` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * website/docs/cli/commands/help.mdx </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>website/docs/cli/commands/version.mdx</summary> [style] ~24-~24: Consider using an alternative to avoid wordiness and strengthen your wording. Context: ... ``` This will show the CLI version. From time to time, Atmos will check for updates. The freq... (FROM_TIME_TO_TIME) --- [style] ~31-~31: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. Context: ...r five hours, or `2d` for two days). 3. As one of the predefined keywords: Choose ... (ENGLISH_WORD_REPEAT_BEGINNING_RULE) </details> </details> </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
aknysh
added
minor
|
These changes were released in v1.127.0. |
what
This PR contains
Here are some comments/illustrations
before for non --help command the update box untintendently appear
after the fix
additionally added config in atmos.yaml
with defualt config deined as
and env variable to disable everuthing if user does not want this feature displayed at all except version in config/utils.go
after the first --help run on any command box appears: example terrraform
then atmos/cache.yaml is generated with time stamp
and the box disspaers until the time interval exceed specidifed in config
why
references
Link to the ticket - https://linear.app/cloudposse/issue/DEV-2815/atmos-version-check-configuration
Summary by CodeRabbit
Release Notes
New Features
atmos.yaml.--checkflag for theversioncommand to force update checks.Bug Fixes
Documentation
versioncommand documentation with details on configurable update check frequency and usage examples.atmos.yaml.Chores
.gitignorefor.atmos/cache.yaml.