-
Notifications
You must be signed in to change notification settings - Fork 841
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
Recommend Stack upgrade when appropriate (fixes #1681) #4729
Conversation
Sorry, I forgot one detail on this, please don't review/merge until I update this... there's a FIXME if anyone's curious. |
OK, second commit addresses the FIXME, this is ready for review. |
787c5e6
to
56c2cea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is using the latest version on Hackage to check for a new Stack version. I don't believe this takes into account that we sometimes do patchlevel Hackage-only releases to add compatibility with newer versions of dependencies. These patchlevel Hackage releases don't have any changes in functionality and so never get a full release. I think we should ignore those when it comes to prompting the user to upgrade, especially since stack upgrade
won't be able to do a binary upgrade to them. I think, instead, you could compare the 3-component minor version of the current binary (which you can get with Stack.Types.Version.stackMinorVersion
) to the minor version on hackage (which you can get by passing the hackage version through Stack.Types.Version.minorVersion
). The Docker code does something similar when it needs to download a Docker-compatible stack
executable. The release process has a policy that patchlevels are only ever used for Hackage-only releases, so this should be a safe assumption to make.
Explained by @borsboom at #4729 (review)
Apologies for merging master into this branch during a review, but due to how the caching system on CI is working, the tests will fail with a migration error unless I include the schema change on master. The relevant change to address your comments is 191301a. Thanks for the review 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be possible to simplify tracking of the time of last check.
ce3d9f9
to
b85d072
Compare
This leverages a single table and upsert, so that we update records in place, and can reuse this table for additional tracking of last-performed actions in the future.
Note: Documentation fixes for https://docs.haskellstack.org/en/stable/ should target the "stable" branch, not master.
Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!