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

Use a PR for Release Commits #521

Closed
wants to merge 98 commits into from

Conversation

blink1073
Copy link
Contributor

@blink1073 blink1073 commented Aug 5, 2023

Fixes #505

  • Remove the need for ADMIN_GITHUB_TOKEN
  • Create a PR and merge it to make the actual changes in populate release
  • Create and push the tag as part of finalize release
  • Steps:
    • Test this using https://github.com/blink1073/test-python-project
    • Get tests to pass
    • Add notes on how to test changes - use the fork and branch as the @
    • Update examples and docs - must always use environment for security - should use trusted publishers
    • Publish this as the v3 tag
    • Update docs and example workflows for the new fine-grained token access

@blink1073 blink1073 added the enhancement New feature or request label Aug 5, 2023
@blink1073 blink1073 marked this pull request as draft August 14, 2023 00:17
@fcollonval
Copy link
Member

Thanks a lot @blink1073 this is a nice improvement; the new token required will indeed be far better.

@jtpio
Copy link
Member

jtpio commented Aug 16, 2023

The one downside I see is that the PRs themselves are in the name of whoever created the token, but the commits will still be in the name of whichever admin runs the deployment.

Hmm in that case maybe we should keep using the bots, so the PRs are opened from the bot account, so it's more "neutral"? Otherwise it would be a bit strange if the GitHub account used for opening the PR does not correspond to the person doing the release.

@blink1073
Copy link
Contributor Author

We would need to give the bots admin access to the org itself:

image

@jtpio
Copy link
Member

jtpio commented Aug 17, 2023

Otherwise a maintainer who wants to make a new release would have to go to the repository settings and put their token. Then after the release remove the secret. The next maintainer would also have to use their token to make a release.

The downside is that this adds a couple more clicks and burden for making a release, but should help associate the PR with the maintainer the release.

@jtpio
Copy link
Member

jtpio commented Aug 17, 2023

Or maybe the workflow dispatch could take the user token as input as a mandatory field. Although not sure this can be done securely without potentially leaking the token.

@blink1073
Copy link
Contributor Author

That could be up to the project. Personally, I don't think it would be worth it, since the commits are still in the workflow runner's username.

@fcollonval
Copy link
Member

Personally, I don't think it would be worth it, since the commits are still in the workflow runner's username.

👍

@blink1073
Copy link
Contributor Author

This commit has the docs and example updates: 022cba1 (#521)

@blink1073 blink1073 marked this pull request as ready for review September 9, 2023 14:50
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @blink1073

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

@blink1073
Copy link
Contributor Author

@fcollonval @jtpio I have an idea: instead of using the personal access token, we can add an option to jupyterlab-probot to close and reopen a PR meeting certain criteria. It already subscribes to Pull Requests and it has access to open/close them. This would trigger the workflows to run, and would require no ongoing maintenance. We can still make the PAT available for folks who don't want to use jupyterlab-probot but still have merge requirements.

@jtpio
Copy link
Member

jtpio commented Sep 25, 2023

we can add an option to jupyterlab-probot to close and reopen a PR meeting certain criteria

This would also be useful to trigger CI after Playwright snapshots updates: jupyterlab/jupyterlab#13505

@blink1073
Copy link
Contributor Author

Yeah I like that idea. The alternative would be to add a PAT to the workflow, but I don't think we want to start doing that in general, because it would be easy to introduce a security hole.

@blink1073
Copy link
Contributor Author

Now that that the PR bot can close/reopen, the next step is to add that as an option, and make the comment if it is given, and update the instructions accordingly.

@blink1073
Copy link
Contributor Author

I think this actually conflicts with the ability to have silent releases, since we'd have to have a PR instead of a push.

New idea: use the user's GITHUB_TOKEN, and recommend the use of an environment. Technically our check for the github actor's permissions would still work, but environment is still better.

To handle forwardport and silent changelog PRs, we add an option to make the PR comment to toggle the PR.

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

Successfully merging this pull request may close these issues.

Replace usage of ADMIN_GITHUB_TOKEN
3 participants