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

feat(scripts): Rewrite gen-upgrade-proposal.sh #10590

Merged
merged 8 commits into from
Jan 6, 2025

Conversation

gibson042
Copy link
Member

@gibson042 gibson042 commented Dec 1, 2024

Fixes #8784

Description

  • script arguments for target release/tag/commit and Cosmos upgrade name, plus pass-through of arbitrary agd tx arguments
  • automatic selection of upgrade name from gen-github-release.sh release contents, with interactive disambiguation (where possible) of multiple candidates
  • reasonable defaults for title and description from gen-github-release.sh release contents
  • script option --send to invoke agd tx rather than just printing it out (but successfully invocation additionally requires --from etc.)
  • readable output of the agd tx command and data affecting it, whether or not it is to be invoked directly
  • argument checking with readable error messages and usage details
  • use of GitHub requests and https://main.agoric.net/network-config, overridable by environment variables

Sample use:

$ scripts/gen-upgrade-proposal.sh --send agoric-upgrade-18-rc2
Checking https://api.github.com/repos/Agoric/agoric-sdk for agoric-upgrade-18-rc2...
Verifying archive at https://github.com/Agoric/agoric-sdk/archive/431b36a49f8574ce49c29d152bf0ace03eb5a348.zip...
Generating SHA-256 checksum...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100 10.1M    0 10.1M    0     0  4421k      0 --:--:--  0:00:02 --:--:-- 5292k

Found upgrade names:
* agoric-upgrade-18-basic-2
* agoric-upgrade-18-devnet
* agoric-upgrade-18-mainnet

upgrade name (agoric-upgrade-18-mainnet): typo
use unknown upgrade name typo (y/n)? n
upgrade name (agoric-upgrade-18-mainnet): 

agd tx gov submit-proposal software-upgrade agoric-upgrade-18-mainnet \
  --upgrade-info '{"binaries":{"any":"https://github.com/Agoric/agoric-sdk/archive/431b36a49f8574ce49c29d152bf0ace03eb5a348.zip//agoric-sdk-431b36a49f8574ce49c29d152bf0ace03eb5a348?checksum=sha256:6d8d7b444e28c52dc15731bfc42955d3bb78d135bc6569c290261f278a587046"},"source":"https://github.com/Agoric/agoric-sdk/archive/431b36a49f8574ce49c29d152bf0ace03eb5a348.zip?checksum=sha256:6d8d7b444e28c52dc15731bfc42955d3bb78d135bc6569c290261f278a587046"}' \
  --title 'Upgrade to agoric-upgrade-18-rc2' \
  --description https://github.com/Agoric/agoric-sdk/releases/tag/agoric-upgrade-18-rc2 \
  --chain-id "$(curl -sSL https://main.agoric.net/network-config | jq -r .chainName)" \
  --from '<WALLET>' \
  --upgrade-height "$(agoric-estimator -date '<DATE>' -rpc https://main.rpc.agoric.net:443 | tee /dev/stderr | sed -n '$s/.* //p')" \
  --help

Executing...
scripts/gen-upgrade-proposal.sh: line 286: agoric-estimator: command not found
Error: invalid argument "" for "--upgrade-height" flag: strconv.ParseInt: parsing "": invalid syntax
Usage:
  agd tx gov submit-proposal software-upgrade [name] (--upgrade-height [height]) (--upgrade-info [info]) [flags]

Security Considerations

This script is not directly on a security-critical path, but it is important for it to remain transparent for operators.

Scaling Considerations

n/a

Documentation Considerations

Includes embedded usage information, and also remains backwards-compatible in a zero-argument form that uses git HEAD as the target ref (although we should consider instead making target ref a required argument).

I'd also like to incorporate https://github.com/Agoric/estimator into agd itself, for something better than --upgrade-height "$(agoric-estimator -date '<DATE>' -rpc https://main.rpc.agoric.net:443 | tee /dev/stderr | sed -n '$s/.* //p')", but haven't done so in this PR.

Testing Considerations

Tested manually, although I'm open to ideas.

Upgrade Considerations

None in particular.

Fixes #8784

* script arguments for target release/tag/commit and Cosmos upgrade
  name, plus pass-through of arbitrary `agd tx` arguments
* automatic selection of upgrade name from gen-github-release.sh release
  contents, with interactive disambiguation (where possible) of multiple
  candidates
* reasonable defaults for title and description from
  gen-github-release.sh release contents
* script option `--send` to invoke `agd tx` rather than just printing it
  out (but successfully invocation additionally requires `--from` etc.)
* readable output of the `agd tx` command and data affecting it, whether
  or not it is to be invoked directly
* argument checking with readable error messages and usage details
* use of GitHub requests and https://main.agoric.net/network-config,
  overridable by environment variables
@gibson042 gibson042 requested a review from a team as a code owner December 1, 2024 20:33
Copy link

cloudflare-workers-and-pages bot commented Dec 1, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: f54f821
Status: ✅  Deploy successful!
Preview URL: https://8fb28d46.agoric-sdk.pages.dev
Branch Preview URL: https://gibson-8784-scriptable-gen-u.agoric-sdk.pages.dev

View logs

@gibson042 gibson042 requested a review from mujahidkay December 18, 2024 23:14
@mujahidkay
Copy link
Member

I am getting this error when trying to run the script

agoric@toradora agoric-sdk % scripts/gen-upgrade-proposal.sh agoric-upgrade-18-rc5
Checking https://api.github.com/repos/Agoric/agoric-sdk for agoric-upgrade-18-rc5...
Verifying archive at https://github.com/Agoric/agoric-sdk/archive/ff46c4aacd7b01ce8ea1fa14b77c187efa9a3b7f.zip...
Generating SHA-256 checksum...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100 10.1M    0 10.1M    0     0  2496k      0 --:--:--  0:00:04 --:--:-- 4090k

Found upgrade names:
* agoric-upgrade-18-emerynet-rc3
* agoric-upgrade-18-devnet
* agoric-upgrade-18-mainnet
scripts/gen-upgrade-proposal.sh: line 201: UPGRADE_NAMES: bad array subscript

@gibson042
Copy link
Member Author

gibson042 commented Dec 24, 2024

Hmm, that line is UPGRADE_NAME="${UPGRADE_NAMES[-1]}" # default to the last upgrade name... which is perfectly acceptable bash:

Any element of an array may be referenced using ${name[subscript]}… If the subscript used to reference an element of an indexed array evaluates to a number less than zero, it is interpreted as relative to one greater than the maximum index of the array, so negative indices count back from the end of the array, and an index of -1 refers to the last element.

@mujahidkay what do you see from bash --version? And relatedly, from bash -c 'arr=(foo bar baz); echo "${arr[-1]}";'? I can change this to use a length-minus-one expression, but that shouldn't be necessary so I'd like to better understand your environment.

@mujahidkay
Copy link
Member

Hmm, that line is UPGRADE_NAME="${UPGRADE_NAMES[-1]}" # default to the last upgrade name... which is perfectly acceptable bash:

Any element of an array may be referenced using ${name[subscript]}… If the subscript used to reference an element of an indexed array evaluates to a number less than zero, it is interpreted as relative to one greater than the maximum index of the array, so negative indices count back from the end of the array, and an index of -1 refers to the last element.

@mujahidkay what do you see from bash --version? And relatedly, from bash -c 'arr=(foo bar baz); echo "${arr[-1]}";'? I can change this to use a length-minus-one expression, but that shouldn't be necessary so I'd like to better understand your environment.

@gibson042 it indeed turned out to be a version issue. The script doesn't work with GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin24) and grep (BSD grep, GNU compatible) 2.6.0-FreeBSD. But worked after I updated them to GNU bash, version 5.2.37(1)-release (x86_64-apple-darwin23.4.0) and grep (GNU grep) 3.11. Ideally, I would like to mention these version requirements in the MAINTAINERS doc somewhere (I'll file an issue for it and make the changes) because I also noticed we need jq 1.7 minimum when running scripts/npm-dist-tag.sh and my machine had jq 1.6 out of the box.

@gibson042
Copy link
Member Author

The script doesn't work with GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin24) and grep (BSD grep, GNU compatible) 2.6.0-FreeBSD. But worked after I updated them to GNU bash, version 5.2.37(1)-release (x86_64-apple-darwin23.4.0) and grep (GNU grep) 3.11.

@mujahidkay Thanks for checking. I decided to refactor the script so it should work with older bash and grep, because I'd rather have the compatibility than add otherwise unnecessary friction.

I also noticed we need jq 1.7 minimum when running scripts/npm-dist-tag.sh and my machine had jq 1.6 out of the box.

Similarly fixed in #10785.

mergify bot pushed a commit that referenced this pull request Dec 30, 2024
#10590 (comment)
> I also noticed we need `jq 1.7` minimum when running `scripts/npm-dist-tag.sh` and my machine had jq 1.6 out of the box.

## Description
Replace [`pick(...)`](https://jqlang.github.io/jq/manual/v1.7/#pick) with an equivalent expression that doesn't require version 1.7 (cf. [jq/NEWS.md](https://github.com/jqlang/jq/blob/jq-1.7/NEWS.md#language-changes:~:text=Adds%20new%20builtin%20pick(stream)) and jqlang/jq#2656 ).

### Security Considerations
None.

### Scaling Considerations
n/a

### Documentation Considerations
Deferred.

### Testing Considerations
Not covered.

### Upgrade Considerations
n/a
Copy link
Member

@mujahidkay mujahidkay left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. This looks good. I have tested basic usage and it works fine. Thanks for this improvement.

@gibson042 gibson042 added the automerge:squash Automatically squash merge label Jan 6, 2025
@mergify mergify bot merged commit f4df38c into master Jan 6, 2025
81 checks passed
@mergify mergify bot deleted the gibson-8784-scriptable-gen-upgrade-proposal branch January 6, 2025 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gen-upgrade-proposal.sh should be scriptable
2 participants