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

Add option to sign release artifacts with verify_release #1979

Merged
merged 4 commits into from
Apr 27, 2022

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Apr 27, 2022

Quickfix 2 for #1966 (I suggest to leave the issue open or create a new one for a long-term fix)

Description of the changes being introduced by the pull request:

  • Add option to sign locally built release artifacts with gpg, if they match the downloaded artifacts (from GitHub, PyPI).
  • Update RELEASE.md to describe usage of new option
  • [Unrelated] Fix success message. (Happy to create a separate PR, if desired)

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

Prior to theupdateframework#1946 the verify_release script was successful if both PyPI
and GitHub release artifacts matched the local build.

Now, if the `--skip-pypi` option is provided, the script can also
be successful if only the GitHub release artifacts match the local
build.

This commit splits the final success message in two separate
success messages, one for PyPI and one for GitHub.

Signed-off-by: Lukas Puehringer <[email protected]>
@coveralls
Copy link

coveralls commented Apr 27, 2022

Pull Request Test Coverage Report for Build 2232657631

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 98.327%

Totals Coverage Status
Change from base Build 2226143137: 0.001%
Covered Lines: 1188
Relevant Lines: 1204

💛 - Coveralls

verify_release Outdated
if True:
key_id = None
if args.sign is not True:
key_id = args.sign
Copy link
Member Author

Choose a reason for hiding this comment

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

Apologies if this is becoming too arrowheady.

Add option to sign locally built release artifacts with gpg,
if they match the downloaded artifacts from GitHub, PyPI.

Signed-off-by: Lukas Puehringer <[email protected]>
Mention how to use verify_release with the recently added --sign
option to create signatures for a verified release.

Signed-off-by: Lukas Puehringer <[email protected]>
@lukpueh
Copy link
Member Author

lukpueh commented Apr 27, 2022

I can see how this PR stretches the scope of verify_release quite a bit. A separate tool, e.g. sign_release, might be a better fit. I suppose I mostly wanted to reuse the build function in verify_release, and not over-engineer it given that this a quick and hopefully temporary fix.

Copy link
Member

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

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

Very cool. This LGTM as is, but I made some minor suggestions inline which you may feel free to accept or ignore.

docs/RELEASE.md Outdated Show resolved Hide resolved
verify_release Outdated Show resolved Hide resolved
verify_release Outdated Show resolved Hide resolved
verify_release Outdated Show resolved Hide resolved
verify_release Outdated Show resolved Hide resolved
verify_release Outdated
progress("Signing built release with gpg")
if success:
key_id = None
if args.sign is not True:
Copy link
Member

@jku jku Apr 27, 2022

Choose a reason for hiding this comment

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

when could this be non-true?

Copy link
Member

Choose a reason for hiding this comment

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

oh, it's True, False or keyid...

Copy link
Member Author

@lukpueh lukpueh Apr 27, 2022

Choose a reason for hiding this comment

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

./verify_release --sign 8BA69B87D43BE294F23E812089A2AD3C07D962E8

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, it's True, False or keyid...

Actually, it is True, None or keyid

Co-authored-by: Joshua Lock <[email protected]>
Signed-off-by: Lukas Puehringer <[email protected]>
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

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

Looks good to me. My concerns are:

  • we're tweaking the verification code just before the release where we want to demonstrate the verification -- but the code looks safe to me so let's do it
  • the important thing WRT signatures would be to make verification easier: signing happens once, but we'd like verification to happen as much as possible -- but I suppose we have to sign before we can verify :)

@jku jku mentioned this pull request Apr 27, 2022
@jku jku merged commit 7e5b9b5 into theupdateframework:develop Apr 27, 2022
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.

4 participants