From 7f2c9b2ba80c08af45e64079acf978c821fc7f49 Mon Sep 17 00:00:00 2001 From: Stefan Koelle Date: Sat, 22 Aug 2026 10:08:48 +0200 Subject: [PATCH] refactor: address code review findings for self-update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - go.mod: 1.25.12 → 1.25 (fix nonexistent patch version) - Remove init() log mutation, pass logger explicitly via initUpdater() - Add 10s HTTP timeout via context.WithTimeout on API calls - Fix version comparison using proper semver (current.LessThan(newVer)) - Extract shared initUpdater() helper to eliminate duplicate creation - Dev build warning now goes to stderr consistently - Exit with code 0 after successful interactive update (binary replaced) --- cmd/kctl-tui/main.go | 4 +- cmd/kctl-tui/update.go | 129 +++++++++++++++++++---------------------- 2 files changed, 63 insertions(+), 70 deletions(-) diff --git a/cmd/kctl-tui/main.go b/cmd/kctl-tui/main.go index a97520c..6c1e386 100644 --- a/cmd/kctl-tui/main.go +++ b/cmd/kctl-tui/main.go @@ -83,7 +83,9 @@ func main() { } } - checkForUpdateInteractive(verbose) + if checkForUpdateInteractive(verbose) { + os.Exit(0) + } m := newFullModel() p := tea.NewProgram(m, tea.WithAltScreen()) diff --git a/cmd/kctl-tui/update.go b/cmd/kctl-tui/update.go index a4f4591..7e5a940 100644 --- a/cmd/kctl-tui/update.go +++ b/cmd/kctl-tui/update.go @@ -7,48 +7,77 @@ import ( "bufio" "context" "fmt" - "log" "os" "strings" + "time" "github.com/Masterminds/semver/v3" "github.com/creativeprojects/go-selfupdate" ) -const githubSlug = "skoelle/kctl-tui" +const ( + githubSlug = "skoelle/kctl-tui" + updateTimeout = 10 * time.Second +) -func runUpdate(verbose bool) error { +func initUpdater(verbose bool) (*selfupdate.Updater, error) { if verbose { selfupdate.SetLogger(&verboseLogger{}) } - current := version - if current == "dev" { + source, err := selfupdate.NewGitHubSource(selfupdate.GitHubConfig{}) + if err != nil { + return nil, fmt.Errorf("failed to init GitHub source: %w", err) + } + + return selfupdate.NewUpdater(selfupdate.Config{ + Source: source, + }) +} + +func runUpdate(verbose bool) error { + if version == "dev" { fmt.Fprintln(os.Stderr, "WARNING: running dev build — cannot compare versions") - fmt.Println("Skipping version check. Build from a tagged release to enable self-update.") + fmt.Fprintln(os.Stderr, "Skipping version check. Build from a tagged release to enable self-update.") return nil } - fmt.Printf("Current version: %s\n", current) - fmt.Println("Checking for updates...") + updater, err := initUpdater(verbose) + if err != nil { + return err + } - rel, err := detectLatest() + ctx, cancel := context.WithTimeout(context.Background(), updateTimeout) + defer cancel() + + repo := selfupdate.ParseSlug(githubSlug) + rel, found, err := updater.DetectLatest(ctx, repo) if err != nil { return fmt.Errorf("failed to check for updates: %w", err) } - - if rel == nil { + if !found { fmt.Println("Already up-to-date.") return nil } - fmt.Printf("Found version %s. Updating...\n", rel.Version()) + current, _ := semver.NewVersion(version) + newVersion := rel.Version() - if err := applyUpdate(rel); err != nil { + newVer, _ := semver.NewVersion(newVersion) + + if current != nil && !current.LessThan(newVer) { + fmt.Println("Already up-to-date.") + return nil + } + + fmt.Printf("Current version: %s\n", version) + fmt.Printf("Found version %s. Updating...\n", newVersion) + + if err := updater.UpdateTo(ctx, rel, ""); err != nil { return fmt.Errorf("update failed: %w", err) } - fmt.Printf("Updated from %s to %s\n", current, rel.Version()) + fmt.Printf("Updated from %s to %s\n", current, newVersion) return nil } @@ -59,27 +88,34 @@ func checkForUpdateInteractive(verbose bool) bool { return false } - if verbose { - selfupdate.SetLogger(&verboseLogger{}) - } - - rel, err := detectLatest() + updater, err := initUpdater(verbose) if err != nil { - // Silently ignore network errors — don't block startup if verbose { fmt.Fprintf(os.Stderr, "Update check failed: %v\n", err) } return false } - if rel == nil { + ctx, cancel := context.WithTimeout(context.Background(), updateTimeout) + defer cancel() + + repo := selfupdate.ParseSlug(githubSlug) + rel, found, err := updater.DetectLatest(ctx, repo) + if err != nil { + if verbose { + fmt.Fprintf(os.Stderr, "Update check failed: %v\n", err) + } + return false + } + if !found { return false } current, _ := semver.NewVersion(version) newVersion := rel.Version() + newVer, _ := semver.NewVersion(newVersion) - if current != nil && !rel.GreaterThan(current.String()) { + if current != nil && !current.LessThan(newVer) { return false } @@ -94,56 +130,15 @@ func checkForUpdateInteractive(verbose bool) bool { } fmt.Println("Updating...") - if err := applyUpdate(rel); err != nil { + if err := updater.UpdateTo(ctx, rel, ""); err != nil { fmt.Fprintf(os.Stderr, "Update failed: %v\n", err) return false } - fmt.Printf("Updated to %s. Starting kctl-tui...\n", newVersion) + fmt.Printf("Updated to %s. Please restart kctl-tui.\n", newVersion) return true } -func detectLatest() (*selfupdate.Release, error) { - source, err := selfupdate.NewGitHubSource(selfupdate.GitHubConfig{}) - if err != nil { - return nil, fmt.Errorf("failed to init GitHub source: %w", err) - } - - updater, err := selfupdate.NewUpdater(selfupdate.Config{ - Source: source, - }) - if err != nil { - return nil, fmt.Errorf("failed to create updater: %w", err) - } - - repo := selfupdate.ParseSlug(githubSlug) - rel, found, err := updater.DetectLatest(context.Background(), repo) - if err != nil { - return nil, err - } - if !found { - return nil, nil - } - - return rel, nil -} - -func applyUpdate(rel *selfupdate.Release) error { - source, err := selfupdate.NewGitHubSource(selfupdate.GitHubConfig{}) - if err != nil { - return fmt.Errorf("failed to init GitHub source: %w", err) - } - - updater, err := selfupdate.NewUpdater(selfupdate.Config{ - Source: source, - }) - if err != nil { - return fmt.Errorf("failed to create updater: %w", err) - } - - return updater.UpdateTo(context.Background(), rel, "") -} - type verboseLogger struct{} func (l *verboseLogger) Print(v ...any) { @@ -153,7 +148,3 @@ func (l *verboseLogger) Print(v ...any) { func (l *verboseLogger) Printf(format string, v ...any) { fmt.Fprintf(os.Stderr, format, v...) } - -func init() { - log.SetOutput(os.Stderr) -}