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

fix: place updater in same directory as update.sh #3983

Merged
merged 6 commits into from
May 25, 2022

Conversation

algolucky
Copy link
Contributor

@algolucky algolucky commented May 13, 2022

Summary

Fixes #2581

While downloading and installing the updater, the commands were not specific about which directories to download and unarchive. Because of this, if update.sh was in $PATH and was executed from a different directory, the updater archive would be installed in the $PWD causing the script to not function as expected (download remaining tools), and would not error out.

This solves that by creating a temporary directory and modifying the commands around installing the updater more explicit to make sure that updater is in the same directory as update.sh.

This also adds a -verify command line flag (defaults to false). If set, update.sh will check if GPG and checksum validation is possible on linux systems, if so, the signature, public key, and checksum file will be downloaded and used to verify the updater archive.

Test Plan

Can use this test script or the one referenced in #2581

@algolucky algolucky self-assigned this May 13, 2022
@CLAassistant
Copy link

CLAassistant commented May 13, 2022

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented May 13, 2022

Codecov Report

Merging #3983 (3563b5d) into master (aa82b9a) will increase coverage by 4.71%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3983      +/-   ##
==========================================
+ Coverage   49.76%   54.48%   +4.71%     
==========================================
  Files         409      390      -19     
  Lines       69157    48500   -20657     
==========================================
- Hits        34419    26423    -7996     
+ Misses      31014    19858   -11156     
+ Partials     3724     2219    -1505     
Impacted Files Coverage Δ
ledger/roundlru.go 90.56% <0.00%> (-5.67%) ⬇️
cmd/algoh/blockWatcher.go 77.77% <0.00%> (-3.18%) ⬇️
catchup/service.go 68.64% <0.00%> (-0.25%) ⬇️
network/wsNetwork.go 62.79% <0.00%> (ø)
crypto/merklearray/msgp_gen.go
data/hashable/msgp_gen.go
agreement/msgp_gen.go
data/account/msgp_gen.go
crypto/compactcert/msgp_gen.go
compactcert/msgp_gen.go
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa82b9a...3563b5d. Read the comment docs.

@algolucky algolucky marked this pull request as ready for review May 13, 2022 21:36
@algolucky algolucky requested a review from a team May 13, 2022 21:43
@algobarb algobarb requested a review from winder May 15, 2022 12:49
@algolucky algolucky force-pushed the fix-updater-in-path branch 3 times, most recently from 5a84ae0 to 5d4bfdf Compare May 16, 2022 20:44
onetechnical
onetechnical previously approved these changes May 17, 2022
winder
winder previously approved these changes May 17, 2022
@algolucky algolucky dismissed stale reviews from winder and onetechnical via e4638ef May 18, 2022 15:44
algobarb
algobarb previously approved these changes May 18, 2022
Copy link
Contributor

@algobarb algobarb left a comment

Choose a reason for hiding this comment

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

Thanks for adding the comment

onetechnical
onetechnical previously approved these changes May 18, 2022
cmd/updater/update.sh Outdated Show resolved Hide resolved
@algolucky algolucky dismissed stale reviews from onetechnical and algobarb via 46c45cd May 19, 2022 09:20
algobarb
algobarb previously approved these changes May 19, 2022
@algolucky algolucky requested a review from cce May 19, 2022 15:30
onetechnical
onetechnical previously approved these changes May 20, 2022
Copy link
Contributor

@onetechnical onetechnical left a comment

Choose a reason for hiding this comment

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

I didn't run this, but it looks good on the surface. We should augment the test cases with something like:

  • Run on darwin
  • Run on linux, with gpg and sha256sum
  • Run on linux, with gpg and without sha256sum
  • Run on linux, without gpg and with sha256sum
  • Run on linux without gpg and without sha256sum

algobarb
algobarb previously approved these changes May 20, 2022
@algolucky algolucky dismissed stale reviews from algobarb and onetechnical via 3563b5d May 20, 2022 20:26
@algolucky algolucky changed the title fix: use temporary directory when downloading updater fix: place updater in same directory as update.sh May 20, 2022
@algolucky
Copy link
Contributor Author

I've added the -verify flag, and set it to false by default. This way a user would have to explicitly acknowledge this change instead of it opening up the possibility of unattended side-effects on systems that the script is unaware of.

Copy link
Contributor

@onetechnical onetechnical left a comment

Choose a reason for hiding this comment

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

Thanks for the update; preserving behavior is nice. Thinking about how we want to communicate this is a good idea too.

Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

LGTM. I updated Sandbox to use one of the earlier drafts, I didn't realize the checksum/verify additions were coming.

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

Successfully merging this pull request may close these issues.

Calling 'update.sh' from a different directory doesn't work.
6 participants