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

allow install script to print error on failed binary download #11335

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

tommatime
Copy link
Contributor

Proposed Changes

Currently, when the install script encounters an error downloading a file (either the binary signature or the binary itself), the script either exits with no error message or continues running until the missing file causes another error. This is confusing to the user because it is unclear where or why the script failed. This change fixes the bug and allows the error message to print before exiting.

Types of Changes

Bugfix

Verification

For the positive case, the script should continue with no changes to current functionality.

For the negative case, run the script with a condition that will cause the hash or binary download to fail. For example, I set the INSTALL_K3S_VERSION variable to something that does not exist:

$ INSTALL_K3S_VERSION=fake-version sh install.sh

Which should now log something like this, whereas the error message would not show previously:

[INFO]  Using fake-version as release
[INFO]  Downloading hash https://github.com/k3s-io/k3s/releases/download/fake-version/sha256sum-amd64.txt
[ERROR]  Download failed <-- This is the log that was not showing

Testing

No, if there are integration tests for install.sh please point me in that direction so I can make the update.

Linked Issues

Fixes #1774

User-Facing Change

NONE

Further Comments

When logging $? after the case statement, the result was blank rather than holding the return code for the previous command, which I assume bash interpreted as a 0/success code. This change introduces a $status variable that explicitly stores the exit code, and also cleans up some of the -e and +e logic/adds comments to make sure it is clear why they are being used.

This is my first contribution here, and one of my first ever, so please let me know of any additional changes that may need to be made. Thanks!

@tommatime tommatime marked this pull request as ready for review November 18, 2024 05:28
@tommatime tommatime requested a review from a team as a code owner November 18, 2024 05:28
Copy link
Member

@dereknola dereknola left a comment

Choose a reason for hiding this comment

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

LGTM, for future reference, we do have install script tests, but no change is required for your PR. We just run a bunch of different OS VMs and validate that the install script successfully executes on them.

Copy link

codecov bot commented Nov 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 42.60%. Comparing base (cd4dded) to head (295bc8b).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11335      +/-   ##
==========================================
- Coverage   47.18%   42.60%   -4.59%     
==========================================
  Files         179      179              
  Lines       18600    18600              
==========================================
- Hits         8776     7924     -852     
- Misses       8469     9472    +1003     
+ Partials     1355     1204     -151     
Flag Coverage Δ
e2etests 34.30% <ø> (-7.91%) ⬇️
inttests 34.66% <ø> (-0.02%) ⬇️
unittests 13.82% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@dereknola dereknola merged commit b83f803 into k3s-io:master Nov 21, 2024
37 checks passed
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.

Request: Tweak install script so error is shown in terminal when fail to download release
3 participants