feat: implement update checker logic and tests - #1232
Conversation
37681cc to
a492b5e
Compare
5f754fe to
e930b6a
Compare
a34ffff to
85ea73b
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces an update-checking subsystem for shopware-cli, including a cached version manifest fetch, user-managed notification preferences, and a CLI notification shown after command execution.
Changes:
- Added
internal/updatepackage to fetch/cache latest release info and decide whether update checks should run. - Added
update-notificationscommand to enable/disable/show update notification preference. - Wired an asynchronous update check into
cmd/root.goto print a styled update notice tostderrwhen an update is available.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| internal/update/update.go | Implements update checking, caching, CI/dev suppression, install-context detection, and notification rendering. |
| internal/update/update_test.go | Adds unit tests for version comparison, caching, gating logic, and install-context detection. |
| cmd/update_notifications.go | Adds update-notifications {status,enable,disable} for managing persisted opt-out. |
| cmd/root.go | Runs update check in a background goroutine and prints update notification at end of run. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| brewPrefixBytes, err := exec.CommandContext(context.Background(), brewExe, "--prefix").Output() | ||
| if err != nil { | ||
| return false | ||
| } |
| // Fetch latest release info. | ||
| latestReleaseInfo, err := fetchLatestReleaseInfo(ctx, client) | ||
| if latestReleaseInfo == nil || err != nil { | ||
| return nil, err | ||
| } |
| if !lastCheck.IsZero() && time.Since(lastCheck) < updateCheckInterval { | ||
| return nil, ErrNoUpdateAvailable | ||
| } |
| if version == "dev" { | ||
| return false | ||
| } | ||
|
|
||
| if IsCI() { | ||
| return false | ||
| } | ||
|
|
||
| if IsGitHubActions() { | ||
| return false | ||
| } | ||
|
|
||
| return true |
| if err != nil { | ||
| logging.FromContext(ctx).Errorln(err) | ||
| return 1 | ||
| } |
b265fae to
7a085dd
Compare
7a085dd to
7744dc2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (6)
internal/update/update.go:67
- When a cache entry is still fresh (< updateCheckInterval), CheckForUpdate returns ErrNoUpdateAvailable without comparing the cached version to the installed version. This means users won’t get an update hint on subsequent runs within 24h even if the cached version is newer, and it also prevents using the cache to avoid network calls while still reporting available updates.
if !lastCheck.IsZero() && time.Since(lastCheck) < updateCheckInterval {
return nil, ErrNoUpdateAvailable
}
internal/update/update.go:284
- IsUnderHomebrew runs
brew --prefixwith context.Background(), so a slow/hung brew invocation can delay or hang CLI shutdown when rendering the update hint. Use a bounded timeout for this probe.
brewPrefixBytes, err := exec.CommandContext(context.Background(), brewExe, "--prefix").Output()
if err != nil {
return false
}
internal/update/update_test.go:341
- This test enforces that update hints are suppressed for the full updateCheckInterval (second call returns ErrNoUpdateAvailable). That behavior contradicts the goal of showing an update notification when an update is available; caching should primarily avoid the network call, not suppress returning cached update info.
second, err := CheckForUpdate(t.Context(), "v0.1.0", client)
require.ErrorIs(t, err, ErrNoUpdateAvailable)
assert.Nil(t, second)
internal/update/update.go:123
- ShouldCheckForUpdate treats "-n" as a no-update flag, but "-n" is already used for "--no-interaction" in cmd/root.go. This unintentionally disables update hints whenever users run with --no-interaction/-n.
for _, arg := range args {
if arg == "--no-update-hint" || arg == "-n" {
return false
}
}
internal/update/update_test.go:129
- This test currently expects ErrNoUpdateAvailable when the cache is still fresh, even though the cached version is newer than the installed version. If CheckForUpdate is meant to use the cache to avoid network calls while still reporting available updates, the cached release should be returned here with requestCount staying at 0.
This issue also appears on line 338 of the same file.
rel, checkErr := CheckForUpdate(t.Context(), "v1.0.0", client)
require.ErrorIs(t, checkErr, ErrNoUpdateAvailable)
assert.Nil(t, rel)
assert.Equal(t, 0, requestCount)
cmd/root.go:122
- run() blocks on
<-updateChanafter command execution. For fast commands, this can add up to ~300ms to every invocation (or longer if the goroutine stalls), which defeats the intent of doing the update check in the background.
// Wait for the update check to finish and print a message to stderr if an update is available
newRelease := <-updateChan
if newRelease != nil {
binaryPath, err := os.Executable()
if err != nil {
7744dc2 to
b7c1776
Compare
What changed?
internal/update.gothat holds logic to check if an CLI update is available.cmd/root.goto run the update check in a background goroutine and print an update notification if an update is available.Implementation heavily inspired by the github cli repository
Why?
Users have no way of knowing whether their installed version is current or out of date.
Implementation Details
Flow [update available and requirements met]:
(*) Requirements for running an update check:
updateCheckInterval, so can be adjusted)devbuild or in CI environment / GitHub ActionOther noteworthy details:
How was it tested?
Unit tests in
internal/update/update_test.gocover:ShouldCheckForUpdatebehavior for all suppression conditions, including CI, development builds, and user opt-out.Feedback wanted
✨ Would especially appreciate feedback on the following things:
connection I sometimes receive a
context cancelederror caused by the timeout.1. Is that acceptable or should update notifications also be shown after failed commands?version.jsonbecomes the source of truth for the latest CLI release, should this be documented to ensure future release-process changes continue to update it?Related issue or description
#1091