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

Create backport PR automatically #148

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Create backport PR automatically #148

wants to merge 1 commit into from

Conversation

r0mant
Copy link
Contributor

@r0mant r0mant commented Aug 3, 2023

2 changes to the backport bot:

  • Attempt to create the backport PR automatically. Fallback to the current "quick PR" link if it fails.
  • Transfer all labels (except for "backport/" ones) to the opened backport PR.

@r0mant r0mant requested review from a team August 3, 2023 22:49
@r0mant
Copy link
Contributor Author

r0mant commented Aug 7, 2023

@jentfoo @reedloden A side effect of this change would be that the original PR creator could technically approve their own backport PR because it will be opened by the bot account. We would still require the 2nd approval.

Just want to make sure it's fine from the compliance perspective like SOC2, etc.

@jentfoo
Copy link
Contributor

jentfoo commented Aug 7, 2023

@r0mant, brainstorming, what if we protected the automatic backport branches so that custom changes can't be pushed to them? If that's reasonable then I don't think there is any concerns, since we have control over what content can be in the PR.

@r0mant
Copy link
Contributor Author

r0mant commented Aug 7, 2023

@jentfoo Yeah, we can create a branch protection rule that will require a pull request to merge to backport/xxx branches. I'll send a PR to github-terraform.

@r0mant
Copy link
Contributor Author

r0mant commented Aug 7, 2023

@jentfoo Before I do it though: do we have to do it? I.e. my question was, will we actually violate any compliance provisions if we don't protect backport branches? Just trying to not introduce any extra barriers for the team unless really necessary.

@reedloden
Copy link
Contributor

reedloden commented Aug 7, 2023

From a purely compliance standpoint, as long as we have at least one peer review (as in, review from an authorized non-bot approver who is not the code author), we're fine. The two separate reviews is a goal, but not a hard requirement.

From a security standpoint, adding branch protections is the right call. Lack of branch protections has bitten us in the past, and I'm concerned what would happen if a non-bot user pushed branches that matched the same pattern. How does the bot handle it if a branch already exists? What if somebody changes the underlying branch contents (at any time during the process)?

As such, I'd prefer we add branch protections, as these are special branches and should be treated as such.

@r0mant
Copy link
Contributor Author

r0mant commented Aug 9, 2023

@reedloden If non-bot user pushes backport/ branch, nothing would change compared to now. Since it's a user that pushes it, it will require 2 approvals from others to merge to master/release branches just like now.

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.

3 participants