Fix incorrect message for extension upgrades - #8584

I don't think this is quite right, I think this has just reversed the problem. Now if an extension would have been upgraded it it reports that it is up to date. For example, here's a list of all my extensions:

➜ ./bin/gh extension list
NAME             REPO                    VERSION
gh act           nektos/gh-act           v0.2.46
gh classroom     github/gh-classroom     v0.1.12
gh dash          dlvhdr/gh-dash          v3.11.0
gh repo-explore  samcoe/gh-repo-explore  v0.0.7
gh webhook       cli/gh-webhook          v0.1.1

If I upgrade one that does not need an upgrade it works correctly:

➜  ./bin/gh extension upgrade cli/gh-repo-explore --dry-run
[repo-explore]: already up to date
✓ Extension already up to date

But if I upgrade one that does need an upgrade:

➜ ./bin/gh extension upgrade gh-webhook --dry-run
[webhook]: would have upgraded from v0.1.1 to v0.1.2
✓ Extension already up to date

Similarly if I run with --all:

➜ ./bin/gh extension upgrade --all --dry-run
[act]: would have upgraded from v0.2.46 to v0.2.57
[classroom]: would have upgraded from v0.1.12 to v0.1.13
[dash]: would have upgraded from v3.11.0 to v3.12.0
[repo-explore]: already up to date
[webhook]: would have upgraded from v0.1.1 to v0.1.2
✓ Extensions already up to date

In this case I would expect it to say:

✓ Would have upgraded extensions

@williammartin Thank you for clearing this up!

I tried coming up with a new solution by checking for the upToDateError and outputting the message ✓ Already up to date if it exists. This is still a work in progress and I wanted to get your feedback to see if I'm in the right direction.

If this solution is too complicated then perhaps removing the message ✓ Would have upgraded extensions altogether might be the way to go.

Appreciate you taking the time to provide feedback!

Thanks @leevic31,

Honestly, it looks like the ExtensionManager interface should change if we really want to support this properly. Frankly, the whole thing is a bit wonky. I see what you are doing with upToDateError but I also think it ends up applying to localExtensionUpgradeError and pinnedExtensionUpgradeError, so it starts to get complicated.

Let's come at this from the point of view of the user. They want to know, "if I were to run gh extension upgrade {<name> | --all}, what is going to happen?". Well, I think that the existing output:

[act]: would have upgraded from v0.2.46 to v0.2.59
[classroom]: would have upgraded from v0.1.12 to v0.1.13
[copilot]: already up to date
[dash]: would have upgraded from v3.11.0 to v3.13.0
[repo-explore]: already up to date
[slack]: already up to date
[webhook]: would have upgraded from v0.1.1 to v0.1.2

Tells them exactly what happened or is going to happen. I don't think this final line is needed, and has in fact been broken in a variety of ways for some time. It's not even correct without --dry-run:

➜  cli git:(trunk) ✗ gh extension upgrade --all
[act]: already up to date
[classroom]: already up to date
[copilot]: already up to date
[dash]: already up to date
[repo-explore]: already up to date
[slack]: already up to date
[webhook]: already up to date
✓ Successfully upgraded extensions

Since it has never been included in the non-TTY output and won't be a breaking change for scripts, I suggest we just remove this line altogether. If someone comes back and says "I really like that summary" then fine, maybe we can put the work into resolving it. Right now I think I've spent more time reviewing this than anyone will spend not understanding the output without this line.

What do you think?

Gentle nudge on this @leevic31. No pressure, just doing housekeeping :)

Closing this because it's gone stale. Feel free to reopen it if you ever want to come back to it, thanks!