Page MenuHomePhabricator

Added a script for creating Github release drafts
ClosedPublic

Authored by jasonbcox on Jul 23 2019, 01:17.

Details

Summary

This script automates the manual process of creating a Github draft release.
This process is error-prone as shown in past releases where some of the following have occurred:

  • Git tag pushed, but not commits (or vice versa)
  • Release draft missing binaries
  • Copy-paste error on release notes.
Test Plan
github-release.sh -h
github-release.sh -d                                                                # Error missing tag
github-release.sh -d --tag v0.19.11                                                 # Error missing oauth token
# Create oauth-token on Github and paste the token to a file
github-release.sh -d --tag v0.19.11 -o <path/to/oauth-token>                        # Error missing assert dir

github-release.sh -d --tag v0.19.11 -o <path/to/oauth-token> -a <path/to/binaries>

# Releasing on old tag doesn't blow up
github-release.sh -d --tag v0.17.0 -o <path/to/oauth-token> -a <path/to/binaries>

# Live test on old tag
github-release.sh --tag v0.17.0 -o <path/to/oauth-token> -a <path/to/binaries>   # Creates draft as expected

TBA live test in the next release

Diff Detail

Repository
rABC Bitcoin ABC
Branch
github-release
Lint
Lint Passed
SeverityLocationCodeMessage
Advicecontrib/devtools/github-release.sh:9SC2005ShellCheck found an issue:
Advicecontrib/devtools/github-release.sh:128SC2002ShellCheck found an issue:
Unit
No Test Coverage
Build Status
Buildable 7494
Build 13031: Bitcoin ABC Buildbot (legacy)
Build 13030: arc lint + arc unit

Event Timeline

  • Removed --tag-commit as it's not necessary
  • Minor improvements
  • Lots of linting
  • Rebase
  • Tested live with: ` ./github-release.sh -a ~/binaries/0.20.2/ -o ~/my-oauth-token -t v0.20.2 -d ./github-release.sh -a ~/binaries/0.20.2/ -o ~/my-oauth-token -t v0.20.2 `
Fabien requested changes to this revision.Sep 18 2019, 08:40
Fabien added a subscriber: Fabien.

Please fix the linter issues.

contrib/devtools/github-release.sh
7

What about "$(dirname "$0")" ? The chance that the directory doesn't exist is null, and if cd fails for any reason you get the wrong path (you can use set -e for this).
The same applies in various places in the script.

87

You can make it simpler with:
if ! git fetch "${GIT_REPO}" tag "${TAG}"; then
or:
git fetch "${GIT_REPO}" tag "${TAG}" || (echo echo "Error: Remote does not have tag '${TAG}'." && exit 20)
It has the advantage to work with set -e as well.

This is repeated several times in the script.

This revision now requires changes to proceed.Sep 18 2019, 08:40

Simplified and cleaned up script logic according to feedback.

Changed behavior: warn -> error on asset upload failure.

Fabien requested changes to this revision.Sep 24 2019, 09:59

Please fix the remaining linter issues.

This revision now requires changes to proceed.Sep 24 2019, 09:59

Fixed remaining lint issues

Fabien requested changes to this revision.Sep 25 2019, 06:31
Fabien added inline comments.
contrib/devtools/github-release.sh
89 ↗(On Diff #13127)

echo is unlikely to fail, but it is generally safer to use set -o pipefail

121 ↗(On Diff #13127)

What is the reason for adding the assets found rather than asserting they are all present ? Is there a case where you want to create a draft release with only some of the assets ?
Furthermore the asset dir is mandatory, but allowed to be empty which is confusing.

124 ↗(On Diff #13127)

Check for jq (it's not a bash built-in nor a very common command AFAIK): if ! command -v jq; then echo error; exit 42; fi

144 ↗(On Diff #13127)

See previous comment, is that an expected behavior ?

This revision now requires changes to proceed.Sep 25 2019, 06:31
contrib/devtools/github-release.sh
121 ↗(On Diff #13127)

The intent was to be robust against name changes and adding or removing support on various distros. However, you'll notice in the last iteration of this diff, I started to move toward a more strict uploading scheme. Considering the complexity of the way it's currently implemented, it's clearly not doing it's job, and continuing with the line of thinking that strictly enforcing all expected files are present is clearly the right way to go.

124 ↗(On Diff #13127)

Good call, though I think that check should be at at the start of the script.

Fixed according to feedback (see comments)

This revision is now accepted and ready to land.Sep 30 2019, 13:03