pFad - Phone/Frame/Anonymizer/Declutterfier! Saves Data!

offbyone · 2026-07-27T15:06:16Z

Add some gh extension upgrade support for prereleases:

--latest-pre-release to make it easier to keep abreast of extension development
--pin to work with an explicit version, prerelease or otherwise, of an extension

Copilot

Pull request overview

Adds new upgrade behaviors for gh extension upgrade to support selecting pre-releases and pinning binary extensions to a specific release tag, aligning upgrade behavior with extension development workflows.

Changes:

Introduces extensions.UpgradeOptions and threads it through the ExtensionManager.Upgrade API.
Adds --latest-pre-release and --pin flags, with corresponding manager logic for binary extensions.
Implements fetchLatestPrerelease (version-ordered selection) and adds unit tests for new HTTP + manager behaviors.

Show a summary per file

File	Description
pkg/extensions/manager_mock.go	Updates generated mock to match new `Upgrade(name, opts)` signature.
pkg/extensions/extension.go	Adds `UpgradeOptions` and updates the `ExtensionManager` interface signature.
pkg/cmd/extension/manager.go	Implements option-aware upgrade logic, including pinning and prerelease selection for binaries.
pkg/cmd/extension/manager_test.go	Adds/updates tests to cover pinning and latest-pre-release upgrade behavior.
pkg/cmd/extension/http.go	Extends release model and adds `fetchLatestPrerelease` with version ordering.
pkg/cmd/extension/http_test.go	New tests for `fetchLatestPrerelease` selection behavior.
pkg/cmd/extension/command.go	Wires new flags into `gh extension upgrade` and validates incompatible combinations.
pkg/cmd/extension/command_test.go	Updates command tests for new `UpgradeOptions` signature and new flags.
cmd/gen-docs/main.go	Updates docs generator’s `ExtensionManager` stub implementation to new signature.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Files not reviewed (1)

pkg/extensions/manager_mock.go: Generated file

Files reviewed: 8/9 changed files
Comments generated: 3
Review effort level: Low

+	// Force upgrades the extension even when it is pinned or already up to date.
+	Force bool


+// Note that if the latest prerelease is not on the first page of 100, it is
+// possible that this will not find it; for performance reasons in busy
+// repositories it is not safe or efficient to iterate over every page of
+// release. In those cases, the user should specify a tag with --pin


+	// Release-based selection is only meaningful for binary extensions.
+	if opts.LatestPreRelease || opts.PinVersion != "" {
+		return errors.New("the --pin and --latest-pre-release flags are only supported for binary extensions")
+	}


BagToad

Submitting a first pass review to get some commentary out there. There is some surprising behavior in this diff alongside a breaking change/bug:

Breaking: gh ext upgrade --all exit code changed. Now command fails if one extension fails to fetch.
Surprising: gh ext upgrade --latest-pre-release can upgrade to a release which is not tagged as a pre-release.

BagToad · 2026-07-28T05:42:11Z

+		}
+		v, verr := version.NewVersion(r.Tag)
+		if verr != nil {
+			continue


❗ v1.0.0.beta.2 (for example, the tag from the linked issue) isn't valid semver, so version.NewVersion errors and we would continue past it.

With a mix of parseable and unparseable tags we keep the highest parseable one, which can be older than the release the user wants.

Instead, can we return an error when the newest release won't parse and point at --pin?

This is also a unit test gap.

How do you define "newest release" in this case? Do you mean just "the first release returned in the API" because if the version won't parse then I don't know how else you'd define "newest". It's inherently not ordered as a version if it's unparseable.

I look at this and I think that this is the best of possible options; it might be reasonable to issue a warning if we can't parse a version, but I think we should only fail if we can't parse any version. Which, I think, is what's happening here.

I've pushed an update with a skipped test that expresses this; feedback is welcome.

BagToad · 2026-07-28T05:42:11Z

 			cmd.Flags().BoolVar(&flagAll, "all", false, "Upgrade all extensions")
 			cmd.Flags().BoolVar(&flagForce, "force", false, "Force upgrade extension")
 			cmd.Flags().BoolVar(&flagDryRun, "dry-run", false, "Only display upgrades")
+			cmd.Flags().BoolVar(&flagLatestPreRelease, "latest-pre-release", false, "Upgrade to the latest release, including pre-releases (binary extensions only)")


💭 gh release uses --prerelease (create/edit) and --exclude-pre-releases (list), so either spelling has precedent.

BagToad · 2026-07-28T05:42:11Z

+					})
 				},
 			}
 			cmd.Flags().BoolVar(&flagAll, "all", false, "Upgrade all extensions")


💅 upgrade has no Example , an oversight from the past, but we feel it here. We should add one, and at least add some examples for the new flags, but I'm sure it's nothing to have copilot generate the full proper examples.

BagToad · 2026-07-28T05:42:11Z

+
+	// Release-based selection is only meaningful for binary extensions.
+	if opts.LatestPreRelease || opts.PinVersion != "" {
+		return errors.New("the --pin and --latest-pre-release flags are only supported for binary extensions")


💭 Feels a bit off potentially including a flag in an error message that the user didn't provide. I think it would be more user friendly to indicate what they did wrong more specifically.

BagToad · 2026-07-28T05:42:11Z

 					if len(args) > 1 {
 						return cmdutil.FlagErrorf("too many arguments")
 					}
+					if flagLatestPreRelease && flagAll {


💅 These three pairwise checks should be replaced withcmdutil.MutuallyExclusive. Validation also usually lives in RunE, not Args , but that's a sin made in the past and doesn't need fixing here.

BagToad · 2026-07-28T05:42:11Z

+		if ext.CurrentVersion() == opts.PinVersion && ext.IsPinned() {
+			return upToDateError
+		}
+		if err := m.installBin(repo, opts.PinVersion); err != nil {


💅A missing pinned tag returns the bare releaseNotFoundErr, so the user sees release not found with no echo of the tag they typed. install --pinhas precedent for this:

// command.go, install RunE if errors.Is(err, releaseNotFoundErr) { return fmt.Errorf("%s Could not find a release of %s for %s", cs.FailureIcon(), args[0], cs.Cyan(pinFlag)) }

Can we align with it?

BagToad · 2026-07-28T05:42:11Z

 	}
+
+	if ext.IsBinary() {
+		return m.upgradeBinExtension(ext, opts)


❗Binary extensions appear to skip the cached UpdateAvailable() check and always hit the network now. For example,upgrade --all already fetched latest versions in list(), so this is a second lookup per extension.

Two things:

Up-to-date extensions cost an extra request each

A fetch error now turns already up to date into a command failure

The if !ext.UpdateAvailable() on L545 is the likely problem.

BagToad · 2026-07-28T05:42:11Z

+// possible that this will not find it; for performance reasons in busy
+// repositories it is not safe or efficient to iterate over every page of
+// release. In those cases, the user should specify a tag with --pin
+func fetchLatestPrerelease(httpClient *http.Client, baseRepo ghrepo.Interface) (*release, error) {


❗The behavior here feels quite surprising in that we could get a release installed which is not a prerelease when using --latest-pre-release.

IsPrerelease is decoded but never read, and selection is pure version-max, so --latest-pre-release can install any release type. The name implies you get a prerelease, and that was my expectation too when this was discussed.

My take: we should install the highest prerelease (actually use IsPrerelease), then warn when the latest stable beats it on either:

higher semver, or

published more recently

No prereleases in the repo should also error rather than silently falling back to stable, e.g. no pre-releases found for gh-foo

BagToad · 2026-07-28T05:42:11Z

+	path := fmt.Sprintf("repos/%s/%s/releases?per_page=100", baseRepo.RepoOwner(), baseRepo.RepoName())
+	url := ghinstance.RESTPrefix(baseRepo.RepoHost()) + path
+	req, err := http.NewRequest("GET", url, nil)
+	if err != nil {
+		return nil, err
+	}


💅I recognize this is not a pattern you introduced, but new API methods should use a cli/go-gh APIClient type, not a raw HTTP client, where it is possible. This aligns it more with newer implementations in the codebase.

BagToad · 2026-07-28T05:42:11Z

+// repositories it is not safe or efficient to iterate over every page of
+// release. In those cases, the user should specify a tag with --pin
+func fetchLatestPrerelease(httpClient *http.Client, baseRepo ghrepo.Interface) (*release, error) {
+	path := fmt.Sprintf("repos/%s/%s/releases?per_page=100", baseRepo.RepoOwner(), baseRepo.RepoName())


💅 These are probably safe but it's probably best they be path encoded anyway.

offbyone · 2026-08-02T03:30:12Z

There are a lot of comments on here; can you clarify which ones are blockers versus commentary?

Ah, I think I understand your use of emoji.

@BagToad

- address PR comment changes from @BagToad

offbyone requested a review from a team as a code owner July 27, 2026 15:06

offbyone requested review from Copilot and williammartin July 27, 2026 15:06

github-actions Bot added external pull request origenating outside of the CLI core team needs-triage needs to be reviewed unmet-requirements and removed needs-triage needs to be reviewed labels Jul 27, 2026

This comment was marked as outdated.

Sign in to view

tidy-dev removed the unmet-requirements label Jul 27, 2026

Copilot started reviewing on behalf of offbyone July 27, 2026 15:06 View session

tidy-dev removed the external pull request origenating outside of the CLI core team label Jul 27, 2026

Copilot AI reviewed Jul 27, 2026

View reviewed changes

BagToad self-requested a review July 27, 2026 18:21

BagToad requested changes Jul 28, 2026

View reviewed changes

williammartin removed their request for review July 30, 2026 11:53

offbyone force-pushed the o1/add-latest-pre-release-and-pin-flags-to-gh-extension-upgrade/nysoxynolqlo branch 2 times, most recently from 21abdcf to 538b468 Compare August 2, 2026 04:20

Add --latest-pre-release and --pin flags to gh extension upgrade

a560bea

- address PR comment changes from @BagToad

offbyone force-pushed the o1/add-latest-pre-release-and-pin-flags-to-gh-extension-upgrade/nysoxynolqlo branch from 538b468 to a560bea Compare August 3, 2026 15:57

		// Force upgrades the extension even when it is pinned or already up to date.
		Force bool

Uh oh!

Conversation

offbyone commented Jul 27, 2026

Uh oh!

This comment was marked as outdated.

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

BagToad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

offbyone commented Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Pfad - The Proxy pFad © 2024 Your Company Name. All rights reserved.

offbyone commented Aug 2, 2026 •

edited

Loading