Enable `spago init` and `spago upgrade-set` to use user-specified package-set tag rather than latest release #680

JordanMartinez · 2020-08-14T19:57:28Z

Description of the change

Fixes #370 by implementing the feature but doesn't have Spago check whether the user-specified tag is valid.

Checklist:

Added the change to the "Unreleased" section of the changelog
Added some example of the new feature to the README
Added a test for the contribution (if applicable)

P.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 😊

src/Spago/CLI.hs

CHANGELOG.md

src/Spago/CLI.hs

src/Spago/PackageSet.hs

src/Spago/Packages.hs

Note: tag is not verified to actually exist in this PR

- modifyPackageSetVersion -> updatePackageSetVersion - updatePackageSet -> useLatestRelease

JordanMartinez · 2020-08-16T14:54:06Z

I've rebased the code on master and accounted for your feedback.

When renaming modifyPackageSetVersion to updatePackageSetVersion, I also renamed updatePackageSet to useLatestRelease. This would be a breaking change if anyone is using spago as a library. I can revert if you'd like, but I thought the name better describes what the code is doing.

JordanMartinez · 2020-08-16T19:20:25Z

A few thoughts on this. In your comment in #370, you list the following things that would make this done:

add a --tag flag to the init and upgrade-set commands
Spago patches the packages.dhall file with the specified tag
before patching, Spago verifies that a package set with that tag exists

1 and 2 have been done. How should number 3 be done, and what should my involvement with that be?

f-f · 2020-08-17T11:17:02Z

@JordanMartinez in the issue you link I proposed that we check the GitHub releases of the repo before patching, to make sure that the tag exists. And the reason why I proposed such solution is only so that we're able to give a decent error message to the user, and prevent silent failure.
I'm not attached to any particular solution as long as we take care of this concern, so that we can introduce this feature while keeping a good UX.

Currently this are the error messages that the user gets when they input a tag that doesn't exist (this is for upgrade-set, but the message for init is similar):

$ spago upgrade-set --tag psc-nonexistent
[info] Changing the package set version to "psc-nonexistent"
[info] Package-set upgraded to latest tag "psc-nonexistent"
Fetching the new one and generating hashes.. (this might take some time)
spago: 
Error: Remote file not found

HTTP status code: 404

URL: https://github.com/purescript/package-sets/releases/download/psc-nonexistent/packages.dhall

Message:

1│ 
2│ 
3│ 
4│ 
5│ <!DOCTYPE html>
6│ <html lang="en">
7│   <head>
…│     <meta charset="utf-8">

It's good that we fail immediately and do not edit the file with the wrong tag, but the message could probably be better?

I think we should merge this as is, and we can take care of error message improvements in another pull request (it could be you or someone else, and ideally it would happen before the next release)

About strategies for improving the error message:

the above message comes from Dhall getting a 404 for the import. We might be able to pattern match on this specific error and return an error message with a suggestion to "please check that the tag you passed in is correct" (though I'm not sure myself which function call actually throws this error)
if it turns out we cannot catch the HTTP error from Dhall, we can just try to fetch the file ourselves before Dhall does that, and handle the 404 there

f-f · 2020-08-17T11:20:16Z

@JordanMartinez re breaking changes: we don't support at all people using Spago as a library, since we don't even publish to Hackage. The current versioning system only cares about breaking changes from the CLI perspective - that's the only API that we guarantee at the moment, so it's fine to change internal things

JordanMartinez · 2020-08-17T13:28:35Z

in the issue you link I proposed that we check the GitHub releases of the repo before patching, to make sure that the tag exists. And the reason why I proposed such solution is only so that we're able to give a decent error message to the user, and prevent silent failure.

Sorry, let me be clearer. I understand (and agree!) that we want a better error message and to verify that the package set actually exists before we patch packages.dhall. However, you described three ways we could do that: check it via GitHub, store the latest list via a release, or modify the Curator. Out of all of them, the Curator approach sounded like the approach with the best tradeoffs. However, the Curator is a separate project, so I was wondering whether I should be involved in that or whether you would work on that and then I could update this PR once that work is done.

If this PR was merged, I would want to update the message above to say "unsafely patching packages.dhall to the specified tag regardless of whether that tag actually exists or not" beforehand."

f-f · 2020-08-17T19:56:57Z

@JordanMartinez

..we want a better error message and to verify that the package set actually exists before we patch packages.dhall

If this PR was merged, I would want to update the message above to say "unsafely patching packages.dhall to the specified tag regardless of whether that tag actually exists or not" beforehand."

Note that we are already throwing an error before we patch the file, which is the behaviour we want here.
I.e. this is a perfectly safe operation because if the package set file does not exists then the packages.dhall will not be patched.

However, you described three ways we could do that: check it via GitHub, store the latest list via a release, or modify the Curator. Out of all of them, the Curator approach sounded like the approach with the best tradeoffs. However, the Curator is a separate project, so I was wondering whether I should be involved in that or whether you would work on that and then I could update this PR once that work is done.

I apologize for not explaining myself clearly in my previous message: I think that all the three ways I described in the past and that you quote here are overly complicated and should not be implemented, and we can get by with implementing one of the two solutions I mentioned above (i.e. catching the error that Dhall gives us or verifying ourseves that we get a 404 when fetching a non-existent package set - I can detail more if you'd like), which are dramatically simpler to implement.

JordanMartinez · 2020-08-18T19:52:39Z

I've done some digging and here's what I've found.

First, in Dhall.purs's writeRawExpr, the above error is thrown via Dhall.Import.load expr, which ultimately calls loadWith

Second, the error message we get is an (HTTPExceptionRequest _ ConnectionFailure) exception based on the error message it produces.

Since I'm not familiar with Haskell, can these exceptions be caught?

Note: Spago.Config uses `Dhall.writeRawExpr` and `PackageSet.useLatestRelease` doesn't need to verify the package set exists. Thus, I duplicated `writeRawExpr` as `writeRawExprPostVerify` so that the package set verification can be done specifically in `PackageSet.useSpecificRelease`

JordanMartinez · 2020-08-19T04:18:03Z

Current error message:

spago init --tag psc-nope
[info] Initializing a sample project or migrating an existing one..
[info] Current tag is different from specified tag "psc-nope"
Fetching the new one and generating hashes.. (this might take some time)
[error] Package-set tag "psc-nope" in the repo "purescript/package-sets" does not exist.
Reverting change back to "psc-0.13.8-20200724

The init command should still work, but right now the program exits. I'll fix that tomorrow.

f-f

Thank you for digging into this! I left some refactoring comments, but otherwise looks great 👍

src/Spago/Messages.hs

src/Spago/PackageSet.hs

Reduces code duplication

Reduce code duplication

JordanMartinez · 2020-08-19T13:29:26Z

I updated the code a bit before I read your messages. I only print the 'package set does not exist' message when we get a 404 specifically. Otherwise, someone might be mislead by a 5xx error.

Current output:

$ spago init --tag psc-none
[info] Initializing a sample project or migrating an existing one..
[info] Updating package-set tag to "psc-none"
Fetching the new one and generating hashes.. (this might take some time)
[warn] Package-set tag "psc-none" in the repo "purescript/package-sets" does not exist.
Will ignore user-specified tag and continue using current tag: "psc-0.13.8-20200724"
[info] Set up a local Spago project.
[info] Try running `spago build`

$ spago upgrade-set --tag psc-please
[info] Updating package-set tag to "psc-please"
Fetching the new one and generating hashes.. (this might take some time)
[warn] Package-set tag "psc-please" in the repo "purescript/package-sets" does not exist.
Will ignore user-specified tag and continue using current tag: "psc-0.13.8-20200724"

JordanMartinez · 2020-08-19T13:30:18Z

...might help if I pushed the commits I just made...

f-f

I left only some tiny suggestions, but otherwise this looks amazing, great work! 👏

Would you like to put together some tests or should we merge now and take care of the tests later?

src/Spago/PackageSet.hs

CHANGELOG.md

JordanMartinez · 2020-08-23T17:38:54Z

I looked at the tests for a bit and wasn't quite sure how to write it. I'll likely need more guidance. We should likely add the tests here so the entire things is self-contained, but it's up to you.

JordanMartinez · 2020-08-24T04:34:23Z

Ah, nevermind. I think I got the hang of if now.

JordanMartinez · 2020-08-24T19:57:46Z

Besides coming to an agreement as to how certain things should be rendered, I think this PR is done!

f-f

It is done indeed! Thank you for doing this 🙂

We squash-merge as a rule, so I added you as a collaborator to the repo and you can reshuffle the commit message and merge yourself.

f-f reviewed Aug 16, 2020

View reviewed changes

src/Spago/CLI.hs Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

src/Spago/CLI.hs Outdated Show resolved Hide resolved

src/Spago/PackageSet.hs Outdated Show resolved Hide resolved

src/Spago/Packages.hs Outdated Show resolved Hide resolved

JordanMartinez added 7 commits August 16, 2020 07:21

Enable user to specify a tag when initializing project

c50cdd1

Note: tag is not verified to actually exist in this PR

Add --tag option to upgrade-set command

596d98a

Update changelog: add --tag as new feature

4a5d703

Refactor: Text.Text -> Text

a2f1505

Remove -t short-hand flag for --tag

c643adc

Refactor code: rename two functions

c65a34e

- modifyPackageSetVersion -> updatePackageSetVersion - updatePackageSet -> useLatestRelease

Refactor: Text.Text -> Text

0aa3afd

JordanMartinez force-pushed the addTag branch from 97af4eb to 0aa3afd Compare August 16, 2020 14:52

JordanMartinez added 3 commits August 18, 2020 18:16

Figure out the necessary path to produce correct error message

c8a80d7

Further clean up error messages

cd9d385

f-f reviewed Aug 19, 2020

View reviewed changes

JordanMartinez added 7 commits August 19, 2020 05:43

Only notify user that package set doesn't exist if get 404

485129a

Stop exporting useSpecificReleaes and useLatestRelease

201f772

Unify upgradingPackageSet and changingTo... messages into one message

27bba90

Prepare updateTags unification; remove writeRawExprPostVerify

f3f0c15

Move useLatestRelease to where clause in updatePackageSetVersion

2b389a7

Reduces code duplication

Inline common code into updatePackageSetVersion

b8e160e

Reduce code duplication

Remove 'revert' phrase and provide clearer message when set not exist

8765fd3

JordanMartinez changed the title ~~Add tag~~ Enable spago init and spago upgrade-set to use user-specified package-set tag rather than latest release Aug 19, 2020

f-f approved these changes Aug 23, 2020

View reviewed changes

src/Spago/PackageSet.hs Outdated Show resolved Hide resolved

src/Spago/PackageSet.hs Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

JordanMartinez added 2 commits August 23, 2020 10:36

Rename mbTag to maybeTag

14ee82f

Change header to 'breaking changes' and update note

4207ade

JordanMartinez added 2 commits August 24, 2020 12:33

Implement tests for --tag parameter

c3f806b

Update readme to document --tag argument

0cae4f9

f-f approved these changes Aug 25, 2020

View reviewed changes

JordanMartinez merged commit 3b6967b into purescript:master Aug 25, 2020

JordanMartinez deleted the addTag branch August 25, 2020 13:23

f-f mentioned this pull request Oct 30, 2020

How can I depend on a lower version purs? #666

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable `spago init` and `spago upgrade-set` to use user-specified package-set tag rather than latest release #680

Enable `spago init` and `spago upgrade-set` to use user-specified package-set tag rather than latest release #680

JordanMartinez commented Aug 14, 2020 •

edited

JordanMartinez commented Aug 16, 2020

JordanMartinez commented Aug 16, 2020

f-f commented Aug 17, 2020

f-f commented Aug 17, 2020

JordanMartinez commented Aug 17, 2020

f-f commented Aug 17, 2020

JordanMartinez commented Aug 18, 2020

JordanMartinez commented Aug 19, 2020

f-f left a comment

JordanMartinez commented Aug 19, 2020

JordanMartinez commented Aug 19, 2020

f-f left a comment

JordanMartinez commented Aug 23, 2020

JordanMartinez commented Aug 24, 2020

JordanMartinez commented Aug 24, 2020

f-f left a comment •

edited

Enable spago init and spago upgrade-set to use user-specified package-set tag rather than latest release #680

Enable spago init and spago upgrade-set to use user-specified package-set tag rather than latest release #680

Conversation

JordanMartinez commented Aug 14, 2020 • edited

Description of the change

Checklist:

JordanMartinez commented Aug 16, 2020

JordanMartinez commented Aug 16, 2020

f-f commented Aug 17, 2020

f-f commented Aug 17, 2020

JordanMartinez commented Aug 17, 2020

f-f commented Aug 17, 2020

JordanMartinez commented Aug 18, 2020

JordanMartinez commented Aug 19, 2020

f-f left a comment

Choose a reason for hiding this comment

JordanMartinez commented Aug 19, 2020

JordanMartinez commented Aug 19, 2020

f-f left a comment

Choose a reason for hiding this comment

JordanMartinez commented Aug 23, 2020

JordanMartinez commented Aug 24, 2020

JordanMartinez commented Aug 24, 2020

f-f left a comment • edited

Choose a reason for hiding this comment

Enable `spago init` and `spago upgrade-set` to use user-specified package-set tag rather than latest release #680

Enable `spago init` and `spago upgrade-set` to use user-specified package-set tag rather than latest release #680

JordanMartinez commented Aug 14, 2020 •

edited

f-f left a comment •

edited