Add download progress for lets self upgrade#381
Merged
Conversation
Contributor
Reviewer's GuideAdds progress reporting to the self-upgrade binary download, introduces a no-op progress observer, wires progress through the registry and upgrader layers, and unifies terminal detection via a shared utility while updating commands/tests and dependencies accordingly. Sequence diagram for self-upgrade download progresssequenceDiagram
actor User
participant Cobra as CobraCommand_upgrade
participant UpCmd as upgradeProgress
participant Upgrader as BinaryUpgrader
participant Reg as GithubRegistry
participant Obs as fetch.ProgressObserver
participant PRC as progressReadCloser
participant Ext as extract.Gz
User->>Cobra: run "lets self upgrade"
Cobra->>Cobra: initUpgradeCommand(version, appSettings)
Cobra->>UpCmd: upgradeProgress(cmd.ErrOrStderr, appSettings)
UpCmd->>UpCmd: util.IsTerminalWriter(stderr)
alt terminal
UpCmd-->>Cobra: downloadprogress.New(...)
else non_terminal
UpCmd-->>Cobra: fetch.NopObserver{}
end
Cobra->>Upgrader: NewBinaryUpgrader(registry.NewGithubRegistry(), version, WithProgress(progress))
User->>Cobra: execute upgrade command
Cobra->>Upgrader: Upgrade(ctx)
Upgrader->>Reg: DownloadReleaseBinary(ctx, packageName, latestVersion, downloadPath, up.progress)
Reg->>Obs: Start(fetch.ProgressInfo{SourceSelfUpdate, URL, ContentLength})
Obs-->>Reg: fetch.ProgressTracker
Reg->>PRC: progressReadCloser{ReadCloser: resp.Body, tracker: tracker}
Reg->>Ext: extract.Gz(ctx, PRC, dstDir, nil)
loop while reading
Ext->>PRC: Read(p)
PRC->>PRC: ReadCloser.Read(p)
PRC->>Obs: tracker.Add(int64(n))
end
Reg->>Obs: tracker.Done(err)
Reg-->>Upgrader: error or nil
Upgrader-->>Cobra: error or nil
Cobra-->>User: upgrade result
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="internal/upgrade/registry/registry.go" line_range="52-53" />
<code_context>
}
func NewGithubRegistry() *GithubRegistry {
+ transport := http.DefaultTransport.(*http.Transport).Clone()
+ transport.DisableCompression = true
+
client := &http.Client{
</code_context>
<issue_to_address>
**issue (bug_risk):** The direct type assertion on http.DefaultTransport can panic; consider a safer assertion or constructing a new transport explicitly.
This relies on `http.DefaultTransport` always being a `*http.Transport`, which may not hold if it’s overridden (e.g. in tests or by other code), causing a panic. Prefer a safe type assertion with a fallback, e.g.:
```go
base, ok := http.DefaultTransport.(*http.Transport)
if !ok {
base = &http.Transport{}
}
transport := base.Clone()
transport.DisableCompression = true
```
or construct a new `http.Transport` directly and configure it as needed.
</issue_to_address>
### Comment 2
<location path="internal/upgrade/upgrade.go" line_range="34-36" />
<code_context>
-func NewBinaryUpgrader(reg registry.RepoRegistry, currentVersion string) (*BinaryUpgrader, error) {
+type BinaryUpgraderOption func(*BinaryUpgrader)
+
+func WithProgress(progress fetch.ProgressObserver) BinaryUpgraderOption {
+ return func(upgrader *BinaryUpgrader) {
+ upgrader.progress = progress
+ }
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Defensively handle a nil ProgressObserver in WithProgress to avoid potential nil dereferences.
`BinaryUpgrader` starts with a non-nil `NopObserver`, but `WithProgress` can overwrite it with `nil`, leading to a panic when `up.progress.Start(...)` is called. Consider normalizing `nil` to a no-op observer:
```go
func WithProgress(progress fetch.ProgressObserver) BinaryUpgraderOption {
return func(upgrader *BinaryUpgrader) {
if progress == nil {
upgrader.progress = fetch.NopObserver{}
return
}
upgrader.progress = progress
}
}
```
</issue_to_address>
### Comment 3
<location path="internal/upgrade/registry/registry_test.go" line_range="85-94" />
<code_context>
+func TestGithubRegistryDownloadReleaseBinaryReportsProgress(t *testing.T) {
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen progress assertions by checking total bytes added matches Content-Length
The test currently only checks that `len(progress.adds) > 0`, which would still pass if bytes were under‑ or over‑reported. To better validate the progress wiring, assert that `sum(progress.adds)` equals `int64(len(archive))` so changes to `progressReadCloser` that mis-account bytes are caught.
Suggested implementation:
```golang
var totalAdded int64
for _, n := range progress.adds {
totalAdded += n
}
if totalAdded != int64(len(archive)) {
t.Fatalf("expected total progress bytes %d, got %d", len(archive), totalAdded)
}
```
1. Ensure this assertion block appears after the code that triggers the download and populates `progress.adds` in `TestGithubRegistryDownloadReleaseBinaryReportsProgress`.
2. If the existing assertion message or condition differs slightly (e.g. different wording or using `require.NotEmpty`), adjust the SEARCH block accordingly to exactly match the current assertion and replace it with the summing logic above.
3. If `progress.adds` is not of type `[]int64`, adapt the `totalAdded` type and the summation loop to match the actual element type, converting to `int64` for the comparison if needed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+52
to
+53
| transport := http.DefaultTransport.(*http.Transport).Clone() | ||
| transport.DisableCompression = true |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The direct type assertion on http.DefaultTransport can panic; consider a safer assertion or constructing a new transport explicitly.
This relies on http.DefaultTransport always being a *http.Transport, which may not hold if it’s overridden (e.g. in tests or by other code), causing a panic. Prefer a safe type assertion with a fallback, e.g.:
base, ok := http.DefaultTransport.(*http.Transport)
if !ok {
base = &http.Transport{}
}
transport := base.Clone()
transport.DisableCompression = trueor construct a new http.Transport directly and configure it as needed.
Comment on lines
+34
to
+36
| func WithProgress(progress fetch.ProgressObserver) BinaryUpgraderOption { | ||
| return func(upgrader *BinaryUpgrader) { | ||
| upgrader.progress = progress |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Defensively handle a nil ProgressObserver in WithProgress to avoid potential nil dereferences.
BinaryUpgrader starts with a non-nil NopObserver, but WithProgress can overwrite it with nil, leading to a panic when up.progress.Start(...) is called. Consider normalizing nil to a no-op observer:
func WithProgress(progress fetch.ProgressObserver) BinaryUpgraderOption {
return func(upgrader *BinaryUpgrader) {
if progress == nil {
upgrader.progress = fetch.NopObserver{}
return
}
upgrader.progress = progress
}
}
Comment on lines
+85
to
+94
| func TestGithubRegistryDownloadReleaseBinaryReportsProgress(t *testing.T) { | ||
| archive := releaseArchive(t, "updated binary") | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if got := r.URL.Path; got != "/releases/download/v0.0.2/lets_Test_x86_64.tar.gz" { | ||
| t.Fatalf("unexpected path %q", got) | ||
| } | ||
|
|
||
| w.Header().Set("Content-Type", "application/gzip") | ||
| w.Header().Set("Content-Length", strconv.Itoa(len(archive))) | ||
| _, _ = w.Write(archive) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Strengthen progress assertions by checking total bytes added matches Content-Length
The test currently only checks that len(progress.adds) > 0, which would still pass if bytes were under‑ or over‑reported. To better validate the progress wiring, assert that sum(progress.adds) equals int64(len(archive)) so changes to progressReadCloser that mis-account bytes are caught.
Suggested implementation:
var totalAdded int64
for _, n := range progress.adds {
totalAdded += n
}
if totalAdded != int64(len(archive)) {
t.Fatalf("expected total progress bytes %d, got %d", len(archive), totalAdded)
}- Ensure this assertion block appears after the code that triggers the download and populates
progress.addsinTestGithubRegistryDownloadReleaseBinaryReportsProgress. - If the existing assertion message or condition differs slightly (e.g. different wording or using
require.NotEmpty), adjust the SEARCH block accordingly to exactly match the current assertion and replace it with the summing logic above. - If
progress.addsis not of type[]int64, adapt thetotalAddedtype and the summation loop to match the actual element type, converting toint64for the comparison if needed.
Wire a fetch.ProgressObserver through the registry download so the tar.gz stream reports byte progress while extracting, and disable HTTP compression so Content-Length stays usable for the bar. The upgrade command builds its own observer from cobra's stderr stream (cmd.ErrOrStderr) and the already-loaded settings value, rather than threading the observer down from the composition root. A fetch.NopObserver replaces nil as the no-progress default, removing nil checks across the download path. Unify terminal detection behind util.IsTerminalWriter (charmbracelet/x/term), used by cli, the upgrade command, and downloadprogress. This drops the direct mattn/go-isatty dependency; Cygwin/MSYS terminals are no longer treated as interactive, matching what logging and error rendering already did.
kindermax
force-pushed
the
progress-bar-for-self-updates
branch
from
1a59f3e to
7fd90c2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements #367
Wire a fetch.ProgressObserver through the registry download so the tar.gz stream reports byte progress while extracting, and disable HTTP compression so Content-Length stays usable for the bar.
The upgrade command builds its own observer from cobra's stderr stream (cmd.ErrOrStderr) and the already-loaded settings value, rather than threading the observer down from the composition root. A fetch.NopObserver replaces nil as the no-progress default, removing nil checks across the download path.
Unify terminal detection behind util.IsTerminalWriter (charmbracelet/x/term), used by cli, the upgrade command, and downloadprogress. This drops the direct mattn/go-isatty dependency; Cygwin/MSYS terminals are no longer treated as interactive, matching what logging and error rendering already did.
Summary by Sourcery
Add interactive download progress reporting to the self-upgrade flow and centralize terminal detection for progress rendering.
New Features:
Enhancements:
Documentation:
Tests: