diff --git a/cmd/install.go b/cmd/install.go index 0a4c858..5be8974 100644 --- a/cmd/install.go +++ b/cmd/install.go @@ -83,13 +83,14 @@ func newInstallCmd() *installCmd { } err = config.UpsertBinary(&config.Binary{ - RemoteName: pResult.Name, - Path: absPath, - Version: pResult.Version, - Hash: fmt.Sprintf("%x", hash), - URL: u, - Provider: p.GetID(), - PackagePath: pResult.PackagePath, + RemoteName: pResult.Name, + Path: absPath, + Version: pResult.Version, + Hash: fmt.Sprintf("%x", hash), + URL: u, + Provider: p.GetID(), + PackagePath: pResult.PackagePath, + SelectedAsset: pResult.SelectedAsset, }) if err != nil { return err diff --git a/cmd/update.go b/cmd/update.go index df1d901..9bec0fc 100644 --- a/cmd/update.go +++ b/cmd/update.go @@ -121,7 +121,7 @@ func newUpdateCmd() *updateCmd { } log.Debugf("Using provider '%s' for '%s'", p.GetID(), ui.url) - pResult, err := p.Fetch(&providers.FetchOpts{All: root.opts.all, PackagePath: b.PackagePath, SkipPatchCheck: root.opts.skipPathCheck, PackageName: b.RemoteName}) + pResult, err := p.Fetch(&providers.FetchOpts{All: root.opts.all, PackagePath: b.PackagePath, SkipPatchCheck: root.opts.skipPathCheck, PackageName: b.RemoteName, PreviousAsset: b.SelectedAsset, PreviousVersion: b.Version}) if err != nil { if root.opts.continueOnError { updateFailures[b] = fmt.Errorf("Error while fetching %v: %w", ui.url, err) @@ -136,13 +136,14 @@ func newUpdateCmd() *updateCmd { } err = config.UpsertBinary(&config.Binary{ - RemoteName: pResult.Name, - Path: b.Path, - Version: pResult.Version, - Hash: fmt.Sprintf("%x", hash), - URL: ui.url, - Provider: p.GetID(), - PackagePath: pResult.PackagePath, + RemoteName: pResult.Name, + Path: b.Path, + Version: pResult.Version, + Hash: fmt.Sprintf("%x", hash), + URL: ui.url, + Provider: p.GetID(), + PackagePath: pResult.PackagePath, + SelectedAsset: pResult.SelectedAsset, }) if err != nil { return err diff --git a/pkg/assets/assets.go b/pkg/assets/assets.go index 5d60ecd..d20f331 100644 --- a/pkg/assets/assets.go +++ b/pkg/assets/assets.go @@ -74,6 +74,7 @@ type Filter struct { name string packagePath string namePatternUsed bool + preferredUsed bool } type FilterOpts struct { @@ -94,6 +95,17 @@ type FilterOpts struct { // and the part after matches files inside archives. Without a slash the // whole pattern matches top-level asset names only. NamePattern string + + // PreferredAsset is the top-level asset name chosen on a previous + // install/upgrade and PreferredVersion is the version it was chosen at. + // On upgrades, FilterAssets compares the version-stripped form of this + // name against the version-stripped current candidates so the same + // artefact can be re-selected even though release names embed the version. + // CurrentVersion is the version of the release being fetched, used to + // strip the version from candidate names. + PreferredAsset string + PreferredVersion string + CurrentVersion string } type runtimeResolver struct{} @@ -143,6 +155,28 @@ func (f *Filter) FilterAssets(repoName string, as []*Asset) (*FilteredAsset, err } } + // On upgrades, try to re-select the artefact chosen previously. Asset + // names embed the version, so compare the version-stripped forms. The + // preference only applies to the top-level asset list (preferredUsed + // guards against the recursive call for files inside an archive, which + // is already handled by PackagePath). + var preferred string + if f.opts.PreferredAsset != "" && !f.preferredUsed { + f.preferredUsed = true + preferred = SanitizeName(f.opts.PreferredAsset, f.opts.PreferredVersion) + var prefMatches []*Asset + for _, a := range as { + if SanitizeName(a.Name, f.opts.CurrentVersion) == preferred { + prefMatches = append(prefMatches, a) + } + } + if len(prefMatches) == 1 { + a := prefMatches[0] + log.Debugf("Asset %q matches previously selected artefact, selecting automatically", a.Name) + return &FilteredAsset{RepoName: repoName, Name: a.Name, DisplayName: a.DisplayName, URL: a.URL}, nil + } + } + var matches []*FilteredAsset switch { case len(as) == 1: @@ -155,7 +189,7 @@ func (f *Filter) FilterAssets(repoName string, as []*Asset) (*FilteredAsset, err matches = f.scoreAssets(repoName, as) } - return selectCandidate(matches, toFilteredAssets(repoName, as)) + return selectCandidate(matches, toFilteredAssets(repoName, as), preferred, f.opts.CurrentVersion) } // applyNamePattern filters assets to those matching the asset portion of @@ -258,7 +292,11 @@ func keepHighestScored(matches []*FilteredAsset) []*FilteredAsset { // selectCandidate returns the single best match, or prompts the user when // multiple candidates remain. Returns an error if there are no candidates. // allAssets is the full unfiltered list offered as a fallback "List all" option. -func selectCandidate(matches []*FilteredAsset, allAssets []*FilteredAsset) (*FilteredAsset, error) { +// preferred is the version-stripped name of the previously selected artefact +// (empty when there is none); when prompting, the candidate matching it is +// offered as the default so pressing Enter keeps the same artefact. version is +// the current release version, used to strip the version from candidate names. +func selectCandidate(matches []*FilteredAsset, allAssets []*FilteredAsset, preferred, version string) (*FilteredAsset, error) { switch len(matches) { case 0: return nil, fmt.Errorf("Could not find any compatible files") @@ -276,7 +314,7 @@ func selectCandidate(matches []*FilteredAsset, allAssets []*FilteredAsset) (*Fil if len(allAssets) > len(matches) { generic = append(generic, options.LiteralStringer("Show all")) } - choice, err := options.Select("Showing "+strconv.Itoa(len(matches))+" assets out of "+strconv.Itoa(len(allAssets))+". Select an option ", generic) + choice, err := options.SelectWithDefault("Showing "+strconv.Itoa(len(matches))+" assets out of "+strconv.Itoa(len(allAssets))+". Select an option ", generic, defaultIndex(generic, preferred, version)) if err != nil { return nil, err } @@ -288,7 +326,7 @@ func selectCandidate(matches []*FilteredAsset, allAssets []*FilteredAsset) (*Fil sort.SliceStable(all, func(i, j int) bool { return all[i].String() < all[j].String() }) - choice, err = options.Select("Select from all available assets:", all) + choice, err = options.SelectWithDefault("Select from all available assets:", all, defaultIndex(all, preferred, version)) if err != nil { return nil, err } @@ -296,6 +334,20 @@ func selectCandidate(matches []*FilteredAsset, allAssets []*FilteredAsset) (*Fil return choice.(*FilteredAsset), nil } +// defaultIndex returns the index of the option whose version-stripped name +// matches preferred, or -1 when there is no preference or no match. +func defaultIndex(opts []fmt.Stringer, preferred, version string) int { + if preferred == "" { + return -1 + } + for i, o := range opts { + if fa, ok := o.(*FilteredAsset); ok && SanitizeName(fa.Name, version) == preferred { + return i + } + } + return -1 +} + // SanitizeName removes irrelevant information from the // file name in case it exists func SanitizeName(name, version string) string { diff --git a/pkg/assets/assets_test.go b/pkg/assets/assets_test.go index 50a45c3..e6a4764 100644 --- a/pkg/assets/assets_test.go +++ b/pkg/assets/assets_test.go @@ -204,6 +204,93 @@ func TestFilterAssetsNamePattern(t *testing.T) { } } +// TestFilterAssetsPreferred verifies that, on upgrades, the artefact chosen +// previously is re-selected automatically even though release asset names embed +// the (changing) version, and that scoring no longer drives the choice when a +// preference uniquely identifies a candidate. +func TestFilterAssetsPreferred(t *testing.T) { + resolver = testLinuxAMDResolver + + cases := []struct { + name string + as []*Asset + preferredAsset string + preferredVersion string + currentVersion string + want string + }{ + { + // musl and gnu variants score identically (both linux+amd64); the + // previous musl choice is re-selected across the version bump. + name: "re-selects same variant across versions", + as: []*Asset{ + {Name: "tool-1.1.0-linux-amd64-musl.tar.gz"}, + {Name: "tool-1.1.0-linux-amd64-gnu.tar.gz"}, + }, + preferredAsset: "tool-1.0.0-linux-amd64-musl.tar.gz", + preferredVersion: "1.0.0", + currentVersion: "1.1.0", + want: "tool-1.1.0-linux-amd64-musl.tar.gz", + }, + { + // preference overrides scoring: the raw binary and the archive both + // match the platform, but the previously selected archive wins. + name: "preference overrides scoring tie", + as: []*Asset{ + {Name: "tool_1.1.0_linux_amd64"}, + {Name: "tool_1.1.0_linux_amd64.tar.gz"}, + }, + preferredAsset: "tool_1.0.0_linux_amd64.tar.gz", + preferredVersion: "1.0.0", + currentVersion: "1.1.0", + want: "tool_1.1.0_linux_amd64.tar.gz", + }, + } + + for _, c := range cases { + f := NewFilter(&FilterOpts{ + PreferredAsset: c.preferredAsset, + PreferredVersion: c.preferredVersion, + CurrentVersion: c.currentVersion, + }) + got, err := f.FilterAssets("tool", c.as) + if err != nil { + t.Fatalf("%s: unexpected error: %v", c.name, err) + } + if got.Name != c.want { + t.Errorf("%s: got %q, want %q", c.name, got.Name, c.want) + } + } +} + +// TestDefaultIndex verifies the default-selection helper used to pre-select the +// previously used artefact in the interactive prompt. +func TestDefaultIndex(t *testing.T) { + resolver = testLinuxAMDResolver + + opts := []fmt.Stringer{ + &FilteredAsset{Name: "tool-2.0.0-linux-amd64-gnu.tar.gz"}, + &FilteredAsset{Name: "tool-2.0.0-linux-amd64-musl.tar.gz"}, + } + + // Preferred musl variant from a previous version resolves to index 1. + want := SanitizeName("tool-1.0.0-linux-amd64-musl.tar.gz", "1.0.0") + if got := defaultIndex(opts, want, "2.0.0"); got != 1 { + t.Errorf("defaultIndex match: got %d, want 1", got) + } + + // No preference yields no default. + if got := defaultIndex(opts, "", "2.0.0"); got != -1 { + t.Errorf("defaultIndex no-preference: got %d, want -1", got) + } + + // A preference that no longer exists yields no default. + gone := SanitizeName("tool-1.0.0-linux-amd64-static.tar.gz", "1.0.0") + if got := defaultIndex(opts, gone, "2.0.0"); got != -1 { + t.Errorf("defaultIndex missing: got %d, want -1", got) + } +} + // makeTar builds an in-memory tar archive where every entry has mode 0755. func makeTar(files map[string]string) []byte { var buf bytes.Buffer diff --git a/pkg/config/config.go b/pkg/config/config.go index e57b054..e839693 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -35,7 +35,10 @@ type Binary struct { // the package path in config so we don't ask the user to select // the path again when upgrading PackagePath string `json:"package_path"` - Pinned bool `json:"pinned"` + // SelectedAsset is the top-level release asset name chosen on install, + // re-used to default the same artefact when upgrading + SelectedAsset string `json:"selected_asset"` + Pinned bool `json:"pinned"` } func CheckAndLoad() error { diff --git a/pkg/options/options.go b/pkg/options/options.go index 2e84915..add7fe1 100644 --- a/pkg/options/options.go +++ b/pkg/options/options.go @@ -1,9 +1,12 @@ package options import ( + "bufio" "fmt" "io" + "os" "strconv" + "strings" ) type LiteralStringer string @@ -16,33 +19,55 @@ func (l LiteralStringer) String() string { // of the available options is the desired // through STDIN and returns the selected one func Select(msg string, opts []fmt.Stringer) (interface{}, error) { + return SelectWithDefault(msg, opts, -1) +} + +// SelectWithDefault behaves like Select but, when defaultIdx is a valid index +// (>= 0), marks that option as the default and returns it when the user submits +// an empty line (just presses Enter). A negative defaultIdx means no default, +// in which case empty input is rejected just like Select historically did. +func SelectWithDefault(msg string, opts []fmt.Stringer, defaultIdx int) (interface{}, error) { if len(opts) == 1 { return opts[0], nil } fmt.Printf("\n%s\n", msg) for i, o := range opts { - fmt.Printf("\n [%d] %s", i+1, o) + if i == defaultIdx { + fmt.Printf("\n [%d] %s (default)", i+1, o) + } else { + fmt.Printf("\n [%d] %s", i+1, o) + } } - var opt uint - var err error + reader := bufio.NewReader(os.Stdin) for { - fmt.Printf("\n Select an option: ") - _, err = fmt.Scanln(&opt) - if err != nil || opt < 1 || int(opt) > len(opts) { - if err != nil { - if err == io.EOF { - return nil, err - } + if defaultIdx >= 0 { + fmt.Printf("\n Select an option [default %d]: ", defaultIdx+1) + } else { + fmt.Printf("\n Select an option: ") + } + line, err := reader.ReadString('\n') + input := strings.TrimSpace(line) + + if input == "" { + if defaultIdx >= 0 { + return opts[defaultIdx], nil + } + if err == io.EOF { + return nil, err } fmt.Printf("Invalid option") continue } - break - } + opt, convErr := strconv.Atoi(input) + if convErr != nil || opt < 1 || opt > len(opts) { + fmt.Printf("Invalid option") + continue + } - return opts[opt-1], nil + return opts[opt-1], nil + } } // SelectCustom prompts the user which diff --git a/pkg/providers/codeberg.go b/pkg/providers/codeberg.go index 2a5ae6d..704c3a1 100644 --- a/pkg/providers/codeberg.go +++ b/pkg/providers/codeberg.go @@ -46,11 +46,13 @@ func (c *codeberg) Fetch(opts *FetchOpts) (*File, error) { return nil, err } + version := release.TagName + candidates := []*assets.Asset{} for _, a := range release.Attachments { candidates = append(candidates, &assets.Asset{Name: a.Name, URL: a.DownloadURL}) } - f := assets.NewFilter(&assets.FilterOpts{SkipScoring: opts.All, PackagePath: opts.PackagePath, SkipPathCheck: opts.SkipPatchCheck, PackageName: opts.PackageName, NamePattern: opts.NamePattern}) + f := assets.NewFilter(&assets.FilterOpts{SkipScoring: opts.All, PackagePath: opts.PackagePath, SkipPathCheck: opts.SkipPatchCheck, PackageName: opts.PackageName, NamePattern: opts.NamePattern, PreferredAsset: opts.PreviousAsset, PreferredVersion: opts.PreviousVersion, CurrentVersion: version}) gf, err := f.FilterAssets(c.repo, candidates) if err != nil { @@ -67,12 +69,10 @@ func (c *codeberg) Fetch(opts *FetchOpts) (*File, error) { return nil, err } - version := release.TagName - // TODO calculate file hash. Not sure if we can / should do it here // since we don't want to read the file unnecesarily. Additionally, sometimes // releases have .sha256 files, so it'd be nice to check for those also - file := &File{Data: outFile.Source, Name: outFile.Name, Version: version, PackagePath: outFile.PackagePath} + file := &File{Data: outFile.Source, Name: outFile.Name, Version: version, PackagePath: outFile.PackagePath, SelectedAsset: gf.Name} return file, nil } diff --git a/pkg/providers/github.go b/pkg/providers/github.go index a412756..c8071ce 100644 --- a/pkg/providers/github.go +++ b/pkg/providers/github.go @@ -54,11 +54,13 @@ func (g *gitHub) Fetch(opts *FetchOpts) (*File, error) { return nil, err } + version := release.GetTagName() + candidates := []*assets.Asset{} for _, a := range release.Assets { candidates = append(candidates, &assets.Asset{Name: a.GetName(), URL: a.GetURL()}) } - f := assets.NewFilter(&assets.FilterOpts{SkipScoring: opts.All, PackagePath: opts.PackagePath, SkipPathCheck: opts.SkipPatchCheck, PackageName: opts.PackageName, NamePattern: opts.NamePattern}) + f := assets.NewFilter(&assets.FilterOpts{SkipScoring: opts.All, PackagePath: opts.PackagePath, SkipPathCheck: opts.SkipPatchCheck, PackageName: opts.PackageName, NamePattern: opts.NamePattern, PreferredAsset: opts.PreviousAsset, PreferredVersion: opts.PreviousVersion, CurrentVersion: version}) gf, err := f.FilterAssets(g.repo, candidates) if err != nil { @@ -75,12 +77,10 @@ func (g *gitHub) Fetch(opts *FetchOpts) (*File, error) { return nil, err } - version := release.GetTagName() - // TODO calculate file hash. Not sure if we can / should do it here // since we don't want to read the file unnecessarily. Additionally, sometimes // releases have .sha256 files, so it'd be nice to check for those also - file := &File{Data: outFile.Source, Name: outFile.Name, Version: version, PackagePath: outFile.PackagePath} + file := &File{Data: outFile.Source, Name: outFile.Name, Version: version, PackagePath: outFile.PackagePath, SelectedAsset: gf.Name} return file, nil } diff --git a/pkg/providers/gitlab.go b/pkg/providers/gitlab.go index d2d6fb0..6136162 100644 --- a/pkg/providers/gitlab.go +++ b/pkg/providers/gitlab.go @@ -156,7 +156,9 @@ func (g *gitLab) Fetch(opts *FetchOpts) (*File, error) { return nil, err } - f := assets.NewFilter(&assets.FilterOpts{SkipScoring: opts.All, PackagePath: opts.PackagePath, SkipPathCheck: opts.SkipPatchCheck, NamePattern: opts.NamePattern}) + version := release.TagName + + f := assets.NewFilter(&assets.FilterOpts{SkipScoring: opts.All, PackagePath: opts.PackagePath, SkipPathCheck: opts.SkipPatchCheck, NamePattern: opts.NamePattern, PreferredAsset: opts.PreviousAsset, PreferredVersion: opts.PreviousVersion, CurrentVersion: version}) gf, err := f.FilterAssets(g.repo, candidates) if err != nil { @@ -175,12 +177,10 @@ func (g *gitLab) Fetch(opts *FetchOpts) (*File, error) { return nil, err } - version := release.TagName - // TODO calculate file hash. Not sure if we can / should do it here // since we don't want to read the file unnecessarily. Additionally, sometimes // releases have .sha256 files, so it'd be nice to check for those also - file := &File{Data: outFile.Source, Name: outFile.Name, Version: version} + file := &File{Data: outFile.Source, Name: outFile.Name, Version: version, SelectedAsset: gf.Name} return file, nil } diff --git a/pkg/providers/hashicorp.go b/pkg/providers/hashicorp.go index dccef7e..8f41369 100644 --- a/pkg/providers/hashicorp.go +++ b/pkg/providers/hashicorp.go @@ -101,7 +101,9 @@ func (g *hashiCorp) Fetch(opts *FetchOpts) (*File, error) { candidates = append(candidates, &assets.Asset{Name: link.Filename, URL: link.URL}) } - f := assets.NewFilter(&assets.FilterOpts{SkipScoring: opts.All, PackagePath: opts.PackagePath, SkipPathCheck: opts.SkipPatchCheck, NamePattern: opts.NamePattern}) + version := release.Version + + f := assets.NewFilter(&assets.FilterOpts{SkipScoring: opts.All, PackagePath: opts.PackagePath, SkipPathCheck: opts.SkipPatchCheck, NamePattern: opts.NamePattern, PreferredAsset: opts.PreviousAsset, PreferredVersion: opts.PreviousVersion, CurrentVersion: version}) gf, err := f.FilterAssets(g.repo, candidates) if err != nil { return nil, err @@ -112,12 +114,10 @@ func (g *hashiCorp) Fetch(opts *FetchOpts) (*File, error) { return nil, err } - version := release.Version - // TODO calculate file hash. Not sure if we can / should do it here // since we don't want to read the file unnecessarily. Additionally, sometimes // releases have .sha256 files, so it'd be nice to check for those also - file := &File{Data: outFile.Source, Name: outFile.Name, Version: version} + file := &File{Data: outFile.Source, Name: outFile.Name, Version: version, SelectedAsset: gf.Name} return file, nil } diff --git a/pkg/providers/providers.go b/pkg/providers/providers.go index 36b8985..8df8b66 100644 --- a/pkg/providers/providers.go +++ b/pkg/providers/providers.go @@ -18,6 +18,9 @@ type File struct { Version string Length int64 PackagePath string + // SelectedAsset is the top-level release asset name that was chosen, + // stored so the same artefact can be defaulted on the next upgrade + SelectedAsset string } func (f *File) Hash() ([]byte, error) { @@ -35,6 +38,11 @@ type FetchOpts struct { SkipPatchCheck bool Version string NamePattern string + // PreviousAsset is the top-level asset name selected on the previous + // install/upgrade, and PreviousVersion is the version it was selected at. + // On upgrades these let bin re-select the same artefact across versions. + PreviousAsset string + PreviousVersion string } type Provider interface {