-
Notifications
You must be signed in to change notification settings - Fork 2k
Automating docker-node part of the official Node Docker images release process #1646
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
Conversation
@Pehesi97 just notice this, exciting! Feel free to ping if/when you're ready for feedback. /cc @nodejs/docker |
Co-authored-by: Simen Bekkhus <[email protected]>
I'll merge this later today (CET) unless anybody says otherwise - would like to see it handle the new v17 release |
I've enabled EDIT: right
I'd love to hear those reasons 🙂 I've now enabled the auto merge feature, and we already have branch protection rules, so I think all 3 conditions in the docs are ok: https://github.com/peter-evans/enable-pull-request-automerge#conditions We also require a review, so we probably need the auto approve action as well |
Yeah, I think that will work. My main issue when trying to make auto-merge work was that the check suite isn't static (we create it dynamically using GitHub Actions) and it wasn't enough to trigger the "Require status checks to pass before merging" rule. I didn't try requiring approvals for the auto-merge to work, and all the others were already satisfied by the time I opened a test PR. About the auto-approve-action, I think https://github.com/hmarr/auto-approve-action will do the trick. Do you want me to do the changes? |
If it works, I'd prefer to use auto merge over a manual poll, yes 🙂 |
@SimenB actually, I don't think that will work. For auto-approving the PR, we would need the checks to pass. That's kinda the point of polling the GitHub API. |
Co-authored-by: Simen Bekkhus <[email protected]>
Fair enough. Merging will fail unless an approval is added though, so we need that part anyways |
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.
all of these actions can run with the default token.
However, approval must happen with a different token as it's not allowed to approve your own PR
Co-authored-by: Simen Bekkhus <[email protected]>
Co-authored-by: Simen Bekkhus <[email protected]>
Co-authored-by: Simen Bekkhus <[email protected]>
Co-authored-by: Simen Bekkhus <[email protected]>
@Pehesi97 see Pehesi97#27 (also, very sorry for all the back and forth 😬) |
It's OK. I understand. |
I'm not 100% the automerge will work, but as mentioned in Pehesi97#27 (comment), just getting the PR is a great first step. Thank you so much for working on this @Pehesi97, and thanks for your patience! |
Should this have kicked off a new build/release process for this Node.js release from yesterday? Or should I open a PR for that? |
Looks like the Alpine builds still aren't available |
Just released: PR here |
Description
Motivation and Context
#1312
Testing Details
Example Output(if appropriate)
Types of changes
Checklist