-
Notifications
You must be signed in to change notification settings - Fork 207
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
style(scripts): Align gen-upgrade-proposal.sh with community forum release posts #9271
Conversation
Deploying agoric-sdk with Cloudflare Pages
|
scripts/gen-upgrade-proposal.sh
Outdated
|
||
info="{\"binaries\":{\"any\":\"$ZIPURL//agoric-sdk-$COMMIT_ID?checksum=$checksum\"},\"source\":\"$ZIPURL?checksum=$checksum\"}" | ||
UPGRADE_INFO="{\"binaries\":{\"any\":\"$ZIP_URL//agoric-sdk-${COMMIT_ID}?checksum=$CHECKSUM\"},\"source\":\"$ZIP_URL?checksum=$CHECKSUM\"}" |
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.
This aligns with recent forum posts, but we could also quote all interpolated variables (both here and in future posts):
UPGRADE_INFO="{\"binaries\":{\"any\":\"$ZIP_URL//agoric-sdk-${COMMIT_ID}?checksum=$CHECKSUM\"},\"source\":\"$ZIP_URL?checksum=$CHECKSUM\"}" | |
UPGRADE_INFO="{\"binaries\":{\"any\":\"${ZIP_URL}//agoric-sdk-${COMMIT_ID}?checksum=${CHECKSUM}\"},\"source\":\"${ZIP_URL}?checksum=${CHECKSUM}\"}" |
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.
As you wish. It may help readability, though is semantically identical, AFAIK.
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 is. Such a change would only be for consistency and readability.
scripts/gen-upgrade-proposal.sh
Outdated
echo "Verifying archive is at $ZIPURL..." 1>&2 | ||
echo "Verifying archive is at $ZIP_URL..." 1>&2 | ||
zipfile=$(mktemp) | ||
trap 'rm -f "$zipfile"' EXIT | ||
curl -L "$ZIPURL" -o "$zipfile" | ||
curl -L "$ZIP_URL" -o "$zipfile" | ||
|
||
echo "Generating SHA-256 checksum..." 1>&2 | ||
checksum=sha256:$(shasum -a 256 "$zipfile" | cut -d' ' -f1) | ||
CHECKSUM=sha256:$(shasum -a 256 "$zipfile" | cut -d' ' -f1) |
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.
The code in forum posts is not assumed to be running inside a script, and so doesn't have this download-with-deferred-cleanup behavior. But unfortunately, that means it also doesn't have verification that the archive exists. We could get that by adding a HEAD request both here and in future posts, e.g.
echo "Verifying archive is at $ZIP_URL..." 1>&2
curl -fLI --no-progress-meter "$ZIP_URL" -o- > /dev/null
echo "Generating SHA-256 checksum..." 1>&2
CHECKSUM=sha256:$(curl -fL "$ZIP_URL" -o- | shasum -a 256 | cut -d' ' -f1)
Or just keeping the data (currently ~10 MB) in a variable:
ZIP_DATA=$(curl -fL "$ZIP_URL" -o-)
CHECKSUM=sha256:$(printf '%s' "$ZIP_DATA" | shasum -a 256 | cut -d' ' -f1)
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.
I'd prefer using the -f
flag to have curl
fail fast:
echo "Verifying ${ZIP_URL} presence and generating SHA-256 checksum..." 1>&2
CHECKSUM=sha256:$(curl -fL "$ZIP_URL" -o- | shasum -a 256 | cut -d' ' -f1)
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.
As noted above, the code in forum posts is not assumed to be running inside a script—so pipefail
may not be active or even available (our instructions don't require bash). For example, here's what I see when running in a clean shell:
$ CHECKSUM=sha256:$(curl -L 'https://example.invalid/' -o- | shasum -a 256 | cut -d' ' -f1)
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
curl: (6) Could not resolve host: example.invalid
$ echo "CHECKSUM: $CHECKSUM"
CHECKSUM: sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
But I do like the idea of using -f
, and have updated accordingly.
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.
Looks fine to me. I pushed a white space fix to pass lint.
scripts/gen-upgrade-proposal.sh
Outdated
echo "Verifying archive is at $ZIPURL..." 1>&2 | ||
echo "Verifying archive is at $ZIP_URL..." 1>&2 | ||
zipfile=$(mktemp) | ||
trap 'rm -f "$zipfile"' EXIT | ||
curl -L "$ZIPURL" -o "$zipfile" | ||
curl -L "$ZIP_URL" -o "$zipfile" | ||
|
||
echo "Generating SHA-256 checksum..." 1>&2 | ||
checksum=sha256:$(shasum -a 256 "$zipfile" | cut -d' ' -f1) | ||
CHECKSUM=sha256:$(shasum -a 256 "$zipfile" | cut -d' ' -f1) |
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.
I'd prefer using the -f
flag to have curl
fail fast:
echo "Verifying ${ZIP_URL} presence and generating SHA-256 checksum..." 1>&2
CHECKSUM=sha256:$(curl -fL "$ZIP_URL" -o- | shasum -a 256 | cut -d' ' -f1)
scripts/gen-upgrade-proposal.sh
Outdated
|
||
info="{\"binaries\":{\"any\":\"$ZIPURL//agoric-sdk-$COMMIT_ID?checksum=$checksum\"},\"source\":\"$ZIPURL?checksum=$checksum\"}" | ||
UPGRADE_INFO="{\"binaries\":{\"any\":\"$ZIP_URL//agoric-sdk-${COMMIT_ID}?checksum=$CHECKSUM\"},\"source\":\"$ZIP_URL?checksum=$CHECKSUM\"}" |
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.
As you wish. It may help readability, though is semantically identical, AFAIK.
… an HTTP HEAD request This makes its contents copyable for forum posts, which don't presume to be running inside a script and don't require bash pipefail.
cea17a2
to
0bea601
Compare
Fixes #9270
Description
Adopt variable names and interpolation patterns as seen in community forum posts such as cf. https://community.agoric.com/t/proposal-71-agoric-upgrade-14-interchain-stack-smart-wallet-upgrades/633#creating-an-emerynet-upgrade-proposal-for-rc1-3 . Also provides slightly more guidance regarding expected transaction options and where to look for past proposals.
Security Considerations
n/a
Scaling Considerations
n/a
Documentation Considerations
We might want to direct operators to this script, rather than duplicating its contents.
Testing Considerations
This script is well-exercised.
Upgrade Considerations
Script functionality must not change.