Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade templates without git incantation #1095

Merged
merged 1 commit into from
Feb 2, 2023

Conversation

itowlson
Copy link
Contributor

@itowlson itowlson commented Feb 1, 2023

Fixes #990.

Some details on which I would like feedback:

  • I have proposed the name spin templates upgrade rather than update. This aligns with the plugins UI, where 'update' refers to updating the catalogue and upgrade is used to actually install a new version. However, update is established in spin templates install --update (easy to alias), and is misleading if a user is moving back to an earlier version of Spin.
  • I have defaulted it to upgrading templates from all known repositories. If the user wants to be selective, they can pass --ask. An alternative approach would be to prompt by default, and require --all to upgrade all without prompting. After feedback, it now prompts by default, with a --all flag to upgrade all without prompting.
  • I have allowed the user to upgrade a specific repo (optionally, to a specific tag) via the --repo/--branch options. This is an exact synonym for install --update - is it worth bothering? I'm leaning against it now. (The user could pass --ask and just select the one repo, but could not then select a specific tag.)

The report at the end is a work in progress. It will not, actually, boo errors (deserve it though they might). This is now fixed: have at it.

The --ask prompt UI is via the dialoguer crate and looks like this:

image

I'm not much in love with it but I don't want to invest in re-implementing it. If there are better Rust terminal libraries then I'm happy to change.

Signed-off-by: itowlson ivan.towlson@fermyon.com

@rajatjindal
Copy link
Collaborator

prompt by default, and require --all to upgrade all without prompting

I like this one. this is also inline with our spin new --accept-defaults UX.

I have allowed the user to upgrade a specific repo (optionally, to a specific tag) via the --repo/--branch options

hm, i can see it can be useful when user is working on changing template from e.g. fork of a repo.

@mikkelhegn
Copy link
Member

I second the "prompt by default" option. Other than that this looks good to me!

Copy link
Member

@mikkelhegn mikkelhegn left a comment

Choose a reason for hiding this comment

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

No approving code, just experience

@itowlson
Copy link
Contributor Author

itowlson commented Feb 1, 2023

@rajatjindal @mikkelhegn Thanks for the feedback - it now prompts by default.

@kate-goldenring
Copy link
Contributor

However, update is established in spin templates install --update (easy to alias), and is misleading if a user is moving back to an earlier version of Spin.

So are we keeping install --update for backwards compat? My vote would be to change it to install --upgrade if we are choosing to distinguish update and upgrade. Better to eliminate the confusion now than it become bothersome post 1.0

Signed-off-by: itowlson <ivan.towlson@fermyon.com>
@itowlson
Copy link
Contributor Author

itowlson commented Feb 2, 2023

Yes, if we agree that the command should be called upgrade, we should change the flag to --upgrade with an alias to --update for back compat. I was waiting for agreement on the new command name but there seems to be general acceptance. I've made that change.

(I have left the Rust field as being called update for now, because that term is widely used in the template installation code. It would be good to change it to stay in sync with the UI, but I don't want to clutter this PR with renaming noise.)

@fibonacci1729
Copy link
Contributor

LGTM code-wise. I am +1 for the experience discussed above.

@itowlson itowlson merged commit c973a44 into fermyon:main Feb 2, 2023
@rawkode
Copy link

rawkode commented Feb 4, 2023

Awesome upgrade! 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spin templates update should try to update all known templates
6 participants