Fix Version Check & Add Tests #1120
Conversation
…check-for-latest' of https://github.com/cloudposse/atmos into feature/dev-3081-fix-atmos-version-command-not-able-to-check-for-latest
ff56dd2 to
488afbd
Compare
📝 WalkthroughWalkthroughThis pull request refactors the version command in Atmos by delegating its execution logic to a new executor in the Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Cmd as cmd/version.go
participant Exec as versionExec (internal/exec/version.go)
participant GitHub as GitHub API (pkg/utils/github_utils.go)
User->>Cmd: Invoke version command
Cmd->>Exec: Execute(checkFlag)
Exec->>Exec: Print logo & version info
alt checkFlag is true
Exec->>Exec: checkRelease()
Exec->>GitHub: GetLatestRelease(owner, repo)
GitHub-->>Exec: Return latest release tag
Exec->>Exec: Compare versions and log update message
end
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (3)
✨ Finishing Touches
🪧 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 tas 8000 ks 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
|
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/exec/version.go (1)
31-48: Consider more graceful error handling in ExecuteThe
log.Fatalcall will terminate the program ifprintStyledTextfails, which seems too extreme for a UI rendering error.- if err != nil { - //nolint:revive - log.Fatal(err) - } + if err != nil { + log.Error("Failed to print styled text", "err", err) + }
🛑 Comments failed to post (1)
internal/exec/version.go (1)
50-64: 🛠️ Refactor suggestion
Improve error handling for release check failures
The current code continues execution after logging an error at debug level, which could lead to misleading output if the GitHub API call fails (empty latest release tag).
if err != nil || latestReleaseTag == "" { log.Debug("Did not get release tag", "err", err, "latestReleaseTag", latestReleaseTag) + log.Warn("Unable to check for the latest version of Atmos") + return }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (v versionExec) checkRelease() { // Check for the latest Atmos release on GitHub latestReleaseTag, err := v.getLatestGitHubRepoRelease("cloudposse", "atmos") if err != nil || latestReleaseTag == "" { log.Debug("Did not get release tag", "err", err, "latestReleaseTag", latestReleaseTag) log.Warn("Unable to check for the latest version of Atmos") return } latestRelease := strings.TrimPrefix(latestReleaseTag, "v") currentRelease := strings.TrimPrefix(version.Version, "v") if latestRelease == currentRelease { log.Warn("You are running the latest version of Atmos", "version", latestRelease) } else { v.printMessageToUpgradeToAtmosLatestRelease(latestRelease) } }
488afbd to
c8961bb
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1120 +/- ##
==========================================
+ Coverage 17.29% 17.43% +0.13%
==========================================
Files 169 170 +1
Lines 18747 18754 +7
==========================================
+ Hits 3242 3269 +27
+ Misses 14908 14887 -21
- Partials 597 598 +1
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/exec/version.go (2)
40-41: Inline the atmosIcon variable for brevity.
Since it's used only once, consider referencing the icon inline, reducing unnecessary variables.-atmosIcon := "\U0001F47D" -v.printMessage(fmt.Sprintf("%s Atmos %s on %s/%s", atmosIcon, version.Version, runtime.GOOS, runtime.GOARCH)) +v.printMessage(fmt.Sprintf("\U0001F47D Atmos %s on %s/%s", version.Version, runtime.GOOS, runtime.GOARCH))
50-56: Show a brief user message on release check failure.
Currently, the user receives no feedback if there’s an error fetching the release. A short info-level message can enhance user awareness.if err != nil || latestReleaseTag == "" { log.Debug("Did not get release tag", "err", err, "latestReleaseTag", latestReleaseTag) + v.printMessage("Could not determine the latest available Atmos version.") return }
🛑 Comments failed to post (1)
internal/exec/version.go (1)
31-38: 🛠️ Refactor suggestion
Consider avoiding log.Fatal for better control flow.
Instead of exiting the program outright, gracefully return an error or propagate it to the caller. This helps in scenarios where the calling context prefers handling errors differently.- log.Fatal(err) + // Optionally return an error or handle it at a higher level + log.Error("Failed to print styled text", "error", err) + // return fmt.Errorf("failed to print styled text: %w", err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (v versionExec) Execute(checkFlag bool) { // Print a styled Atmos logo to the terminal v.printMessage("") err := v.printStyledText("ATMOS") if err != nil { //nolint:revive // Optionally return an error or handle it at a higher level log.Error("Failed to print styled text", "error", err) // return fmt.Errorf("failed to print styled text: %w", err) } }
|
What’s not clear to me from the PR description is what the actual fix was, in other words why was it failing and why does it work now? |
|
These changes were released in v1.165.2. |
what
--checkflagwhy
references
Summary by CodeRabbit
New Features
Bug Fixes
Chores