Skip to content

upgrade local extension and up-to-date are not failures#4857

Merged
mislav merged 2 commits intocli:trunkfrom
tisonkun:ext-upgrade
Dec 16, 2021
Merged

upgrade local extension and up-to-date are not failures#4857
mislav merged 2 commits intocli:trunkfrom
tisonkun:ext-upgrade

Conversation

@tisonkun
Copy link
Contributor

@tisonkun tisonkun commented Dec 5, 2021

Signed-off-by: tison wander4096@gmail.com

This is a follow-up to #4442.

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun requested a review from a team as a code owner December 5, 2021 08:59
@tisonkun tisonkun requested review from vilmibm and removed request for a team December 5, 2021 08:59
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Dec 5, 2021
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thank you for the contribution. Could you tell us more about what exactly is this fixing?

@mislav mislav added the more-info-needed More info needed from user/contributor label Dec 6, 2021
@tisonkun
Copy link
Contributor Author

tisonkun commented Dec 6, 2021

@mislav thanks for your reply.

After #4442 merged, if you upgrade all extensions, localExtensionUpgradeError & upToDateError won't be counted as failure since they are effectively lastest versions. However, that patch handles only when you run:

gh extension upgrade --all

but not:

gh extension upgrade some-ext

This PR follows up to update the logics for some-ext case.

@mislav
Copy link
Contributor

mislav commented Dec 6, 2021

if you upgrade all extensions, localExtensionUpgradeError & upToDateError won't be counted as failure since they are effectively lastest versions.

But we already don't count local extension upgrade error nor up-to-date error as failures. To demonstrate:

$ gh extension upgrade --all && echo SUCCESS
[branch]: local extensions can not be upgraded
[dependents]: already up to date
[repo-topic]: local extensions can not be upgraded
[screensaver]: already up to date
...
✓ Successfully upgraded extensions
SUCCESS

From what I can see, gh extension upgrade --all already works as I expect. What needs to change?

@tisonkun
Copy link
Contributor Author

tisonkun commented Dec 7, 2021

@mislav not if you upgrade a specific extension and thus it creates inconsistency:

$ gh extension upgrade cherry-pick
X Failed upgrading extension cherry-pick: already up to date

After this change it becomes:

$ bin/gh extension upgrade cherry-pick
already up to date
✓ Successfully upgraded extension cherry-pick

@mislav
Copy link
Contributor

mislav commented Dec 7, 2021

Aah, I see. So this change is about making extension upgrade <ext> (single extension) print an informative message and exit with zero status instead of printing a failure message and exiting with nonzero status?

@tisonkun
Copy link
Contributor Author

@mislav yes.

@tisonkun
Copy link
Contributor Author

@mislav @vilmibm any further concern?

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the wait; I had to mull this over.

A few things stood out to me: that the upgradeExtensions() logic had to change, which felt odd because the goal of the PR was not to change how upgrade --all worked, and that error handling and printing was suddenly the responsibility of upgradeExtension(), when I believe that errors should be best printed in the body of the upgrade CLI command itself. Finally, the localExtensionUpgradeError should still result in a failing exit status for the command, since the extension was unable to be upgraded (the local extension author is fully responsible for handling those repos).

I have pushed changes to illustrate what I mean. I believe the updated approach is simpler. I have also touched up some error printing from the command.

@mislav mislav merged commit 0d3dd7e into cli:trunk Dec 16, 2021
@tisonkun tisonkun deleted the ext-upgrade branch December 16, 2021 14:38
@tisonkun
Copy link
Contributor Author

@mislav thanks for your review and follow ups. I also thought of error handling things but failed to get a result as you did. Thanks for the insights :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external pull request originating outside of the CLI core team more-info-needed More info needed from user/contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants