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

Update err message to handle failed download #48

Merged
merged 2 commits into from
May 31, 2022

Conversation

eightfilms
Copy link
Contributor

@eightfilms eightfilms commented May 30, 2022

Closes #44

  • Add _fuelup_version to the info printed, so that users can check versioning at the release page
  • Clearer err message so that users know what went wrong and where to check instead of just 'unsupported arch' -> actually, in the case of 'unsupported arch', that check should've failed at get_architecture already
  • also removed the $ matching in grep - i'm not sure why matching $ didn't work on my machine, but matching 404 should be sufficient either way since there would be nowhere else in the message that should have a 404

@eightfilms eightfilms self-assigned this May 30, 2022
mitchmindtree
mitchmindtree previously approved these changes May 30, 2022
Copy link
Contributor

@mitchmindtree mitchmindtree left a comment

Choose a reason for hiding this comment

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

Lgtm, just an optional indentation fix

fuelup-init.sh Outdated
if echo "$_err" | grep -q 404$; then
err "installer for platform '$3' not found, this may be unsupported"
if echo "$_err" | grep -q 404; then
err "fuelup was not found - either the release is not ready yet or the tag is invalid. You can check if the release is available here: https://github.com/FuelLabs/fuelup/releases"
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks it like was erroneously changed from spaces to tabs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are spaces preferred here? I ran it all under a shell formatter one of the last PRs. Can we disregard this indentation issue this PR and I will open another PR running shfmt? Would probably be worth including in #31 as well

fuelup-init.sh Outdated
if echo "$_err" | grep -q 404$; then
err "installer for platform '$3' not found, this may be unsupported"
if echo "$_err" | grep -q 404; then
err "fuelup was not found - either the release is not ready yet or the tag is invalid. You can check if the release is available here: https://github.com/FuelLabs/fuelup/releases"
Copy link
Contributor

Choose a reason for hiding this comment

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

Link to the actual release that's being tried

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I linked to the releases page as a conscious decision since linking to a version that doesn't exist results simply in a Github 404 page, which isn't very informative (imo). Linking to the releases page allows the user to look at the latest release from the top anyway, and to manually check if the version the script was trying to download exists.

With that said, will update it to link to the actual release though - if the above logic makes sense I can change it back in future

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless something goes very wrong, the only time this error should be hit under normal operation is when there's a release but not binaries uploaded yet. So not a 404. If it's a 404 then that means something went really wrong, for which linking to the exact page is probably more informative.

Copy link
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

utACK, but will defer to @mitchmindtree for final review.

@eightfilms eightfilms merged commit 94787d2 into master May 31, 2022
@eightfilms eightfilms deleted the binggh/gracefully-failing-fuelup-init branch May 31, 2022 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

fuelup-init.sh should fail gracefully if binary can't be found
3 participants