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

Add cli update command #740

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

blackheaven
Copy link

Description

The aim is to smooth user experience as it comes to updating.

Fixes #616

Type of change

Please select the option(s) that is more relevant.

  • Code cleanup
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

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

@blackheaven thanks for putting the effort into this PR!

I believe there are couple additional features we would need to make this feature complete and usable:

  1. We would want to check that there is already an installation of Wasp on the disk, and that it was done by Wasp installer script. To ensure we are not e.g. installing Wasp while there is some other kind of installation already there, e.g. via package manager or smth. We could edit installer script to have --dry-run option that just says where it would write the files, and then we can check if those paths exist. That would confirm previous installation.
    [Ok, this is a bit advanced actually and we can even leave it out of this PR].
  2. We should let them know which version are they going to upgrade to, and warn them if the version is a breaking change compared to their current version. How to do this? Well, ideally we would modify installer script so we can do --dry-run on it that just tells us the version to which it would update wasp. Keep in mind, installer script is in separate repo: https://github.com/wasp-lang/get-wasp/blob/master/installer.sh . Then, we could use that information to obtain the new version, and we already can obtain the current version from Haskell code, and then we can use the SemVer module in our codebase to do the comparison of the versions and produce appropriate messages / asks / dialogue options.
    [This feature is really important for usability].

Easy way to get the latest version of Wasp:

curl -LIs -o /dev/null -w %{url_effective} https://github.com/wasp-lang/wasp/releases/latest | awk -F/ '{print $NF}'

update :: Command ()
update = do
cliSendMessageC $ Msg.Start "Updating wasp..."
status <- liftIO $ system "curl -sSL https://get.wasp-lang.dev/installer.sh | sh"
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, I don't think this will work, since wasp installer will refuse to write over existing files, and if we are doing updating, it will almost certainly write over existing files.

It will then let you know you need to run it with:

curl -sSL https://get.wasp-lang.dev/installer.sh | sh -s -- --force

So we can just go immediately with that, to make sure it actually performs the update?

update :: Command ()
update = do
cliSendMessageC $ Msg.Start "Updating wasp..."
status <- liftIO $ system "curl -sSL https://get.wasp-lang.dev/installer.sh | sh"
Copy link
Member

Choose a reason for hiding this comment

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

This call to system, will it forward STDIO/STDERR to the STDIO/STDERR of our haskell program? It would be cool if it would! That way they can see progress, and possibly any errors.

Copy link
Author

Choose a reason for hiding this comment

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

Yes it does :)

@blackheaven
Copy link
Author

  • We would want to check that there is already an installation of Wasp on the disk, and that it was done by Wasp installer script. To ensure we are not e.g. installing Wasp while there is some other kind of installation already there, e.g. via package manager or smth. We could edit installer script to have --dry-run option that just says where it would write the files, and then we can check if those paths exist. That would confirm previous installation.
    [Ok, this is a bit advanced actually and we can even leave it out of this PR].

Actually I thought doing it the way round:

  1. Add an update command to the installer which:
  • Check if wasp is installed
  • Compare versions
  • Check whether it's "user"-installed or system-installed (even if we could workaround with $PATH
  • Perform the update

My aim is to avoid logic duplication since users are able to call the script anyway, while they are only able to call wasp update when it's already installed.

I may not be clear, let me know what you think.

  • We should let them know which version are they going to upgrade to, and warn them if the version is a breaking change compared to their current version. How to do this? Well, ideally we would modify installer script so we can do --dry-run on it that just tells us the version to which it would update wasp. Keep in mind, installer script is in separate repo: https://github.com/wasp-lang/get-wasp/blob/master/installer.sh . Then, we could use that information to obtain the new version, and we already can obtain the current version from Haskell code, and then we can use the SemVer module in our codebase to do the comparison of the versions and produce appropriate messages / asks / dialogue options.
    [This feature is really important for usability].

I agree

@Martinsos
Copy link
Member

Interesting proposal that we move logic to the installer script!
One bad thing about installer script is that it is bash, which is harder to maintain than Haskell.
You mentioned that they can call wasp update only when wasp is installed -> but that is fine, right? Since you can't really update Wasp if it is not installed!

So I would certainly go with wasp update, since it is very easy to discover and use, but the question is how much it does on its own, and how much does it use installer script. I would do it on its own as much as I can, and use installer script only when we risk serious duplication of logic.

@blackheaven
Copy link
Author

I've made a new iteration integration update logic, tell me if we're going in the right direction and/or if there are things to improve.

@Martinsos
Copy link
Member

@blackheaven sorry for a bit late response , we have been on company retreat so it took me a bit to get to this!
I think this is a good direction. The main thing I imagined a bit differently was that we would call installer script from Haskell to do the job for us, not download the binary on our own. But, this is also interesting approach. Let's reason about it a bit:

Installer script has a bit more knowledge about where it installs binary, and any additional steps it does, for example right now it also creates wasp bash script in bin/wasp, which then points to wasp-bin. It also checks if wasp is available in the path, produces warning if not.
So if we just call it while telling it to overwrite existing installation, that might be enough. Anyway it has the option right now to overwrite existing installation. And we get these extra benefits, that it checks other stuff also.

On the other hand, if we do the updating via Haskell CLI as you did, then we can be a bit more "surgical", as you were, by updating only a specific thing, not going through the whole installation process.

Ha, I am not sure what is better! My intuition is still pushing me a bit toward calling install script from Haskell at the moment, I think it will be easier to maintain in the short term.

In that case you would figure out the version you want to install from Haskell, you would ask user if they are ok with that, and then, once all is confirmed and ready, you would call installer script. I described that above in the previous comments.

@blackheaven
Copy link
Author

I'm not sure to understand the difference between my first attempt (calling the installer script, which I would modify latter) and your proposal of creating a dedicated script for update.

IIUC, we'll have to create a dedicated script (I'm not sure where to put it), which will be called to do the task.

It feels quite confusing to me as, as you mentioned, bash scripts are harder to maintain, plus it will looks like a lot the installer script.

Maybe I'm wrong but extending current installer script to precise/refine the update/-f case would be cheaper to develop and easier to maintain.

@Martinsos
Copy link
Member

I'm not sure to understand the difference between my first attempt (calling the installer script, which I would modify latter) and your proposal of creating a dedicated script for update.

IIUC, we'll have to create a dedicated script (I'm not sure where to put it), which will be called to do the task.

It feels quite confusing to me as, as you mentioned, bash scripts are harder to maintain, plus it will looks like a lot the installer script.

Maybe I'm wrong but extending current installer script to precise/refine the update/-f case would be cheaper to develop and easier to maintain.

Well that is exactly what I am recommending! I suggested that from Haskell code, we call installer script (which already exists) to do the basic work for us. We would need to modify installer script a bit to possibly support that dry run we mentioned, or smth else, not 100% sure yet.
But right now, you are not calling installer script, instead you are directly downloading the release from github.

Let me know if there is anything else confusing!

@blackheaven
Copy link
Author

Clearer, thanks.

I have modified the code here according to your comments, and modified the install script accordingly: wasp-lang/get-wasp#2

@Martinsos
Copy link
Member

I commented on the other PR on the details of how installer script could work.

Here, one thing that is missing is to check if update is a breaking change and warn them, and ideally offer to update to a non-breaking version instead. That is somewhat more work though so we can possibly postpone that for the future PR.

@blackheaven
Copy link
Author

@Martinsos to move forward on it, should I add parameters to pass-through get-wasp?

@Martinsos
Copy link
Member

@blackheaven pls check my comment on the PR, and also this comment on relevant issue: #616 (comment) .

1 similar comment
@Martinsos
Copy link
Member

@blackheaven pls check my comment on the PR, and also this comment on relevant issue: #616 (comment) .

@weedertree
Copy link

@blackheaven could you assign me this issue

@blackheaven
Copy link
Author

@weedertree I cannot, I am not a maintainer

@Martinsos
Copy link
Member

@weedertree if you want to take this one on, I can assign you!
It was a bit complex one though, mostly on the side of just figuring out exactly what and how we want to do here. I will have to remind myself of the details, but I remember that much.
What is your take on this one? How are you with Haskell, which will likely be needed for this one? How would you approach it?

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

Successfully merging this pull request may close these issues.

Make process of updating Wasp nicer
3 participants