-
Notifications
You must be signed in to change notification settings - Fork 133
feat(version): online update checking #1712
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: John Harris <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds online version checking functionality to the deck version command that notifies users when a newer version is available by querying the GitHub API.
Key changes:
- Adds GitHub API integration to check for latest release version
- Implements semantic version comparison logic with comprehensive test coverage
- Provides a flag to suppress the version check for users who prefer not to have online connectivity
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/version.go | Implements GitHub API client, version comparison logic, and updates version command with optional online checking |
| cmd/version_test.go | Adds comprehensive test coverage for version comparison logic and documents expected API behavior |
Comments suppressed due to low confidence (4)
cmd/version_test.go:61
- The checkLatestVersion function is not actually tested here. Consider refactoring the function to accept a base URL parameter or use dependency injection to make it testable with the mock server.
// We would need to modify checkLatestVersion to accept a custom URL for testing
cmd/version_test.go:71
- This test case documents expected behavior but doesn't actually verify it. The error handling behavior of checkLatestVersion should be explicitly tested.
// The function should return an error but not crash
cmd/version_test.go:81
- This test case documents expected behavior but doesn't actually verify it. The JSON parsing error handling of checkLatestVersion should be explicitly tested.
// The function should handle JSON decode errors gracefully
cmd/version_test.go:92
- This test case documents expected behavior but doesn't actually verify it. The timeout behavior of checkLatestVersion should be explicitly tested.
// The function should timeout after 2 seconds
| fmt.Sscanf(currentParts[i], "%d", ¤tNum) | ||
| fmt.Sscanf(latestParts[i], "%d", &latestNum) |
Copilot
AI
Jul 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt.Sscanf ignores errors and will silently fail to parse non-numeric version parts. This could lead to incorrect version comparisons. Consider using strconv.Atoi and handle the error explicitly to ensure reliable version parsing.
| fmt.Sscanf(currentParts[i], "%d", ¤tNum) | |
| fmt.Sscanf(latestParts[i], "%d", &latestNum) | |
| currentNum, err := strconv.Atoi(currentParts[i]) | |
| if err != nil { | |
| // Handle invalid version part (e.g., log error and treat as 0) | |
| currentNum = 0 | |
| } | |
| latestNum, err := strconv.Atoi(latestParts[i]) | |
| if err != nil { | |
| // Handle invalid version part (e.g., log error and treat as 0) | |
| latestNum = 0 | |
| } |
| fmt.Sscanf(currentParts[i], "%d", ¤tNum) | ||
| fmt.Sscanf(latestParts[i], "%d", &latestNum) |
Copilot
AI
Jul 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt.Sscanf ignores errors and will silently fail to parse non-numeric version parts. This could lead to incorrect version comparisons. Consider using strconv.Atoi and handle the error explicitly to ensure reliable version parsing.
| fmt.Sscanf(currentParts[i], "%d", ¤tNum) | |
| fmt.Sscanf(latestParts[i], "%d", &latestNum) | |
| currentNum, err := strconv.Atoi(currentParts[i]) | |
| if err != nil { | |
| currentNum = 0 // Treat invalid parts as 0 | |
| } | |
| latestNum, err := strconv.Atoi(latestParts[i]) | |
| if err != nil { | |
| latestNum = 0 // Treat invalid parts as 0 | |
| } |
| {"reverse mixed v prefix", "v1.49.2", "1.50.0", true}, | ||
| {"longer version newer", "1.49.2", "1.49.2.1", true}, | ||
| {"longer version older", "1.49.2.1", "1.49.2", false}, | ||
| {"dev version", "dev", "1.49.2", true}, |
Copilot
AI
Jul 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test expects compareVersions("dev", "1.49.2") to return true, but the compareVersions function will treat "dev" as a numeric value (which will parse as 0) and compare it against 1, returning true incorrectly. This test case reveals that the version comparison logic doesn't handle non-semantic version strings properly.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1712 +/- ##
==========================================
+ Coverage 28.19% 28.26% +0.07%
==========================================
Files 67 67
Lines 6949 7006 +57
==========================================
+ Hits 1959 1980 +21
- Misses 4848 4884 +36
Partials 142 142 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This PR adds an online version check when running
deck versionthat will notify the user if there is a newer version, when it was released, and where they can download it. This check can be skipped by passing--suppress-version-checktodeck version.