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 backporting action #124273

Merged
merged 5 commits into from
May 25, 2021
Merged

Add backporting action #124273

merged 5 commits into from
May 25, 2021

Conversation

domenkozar
Copy link
Member

@domenkozar domenkozar commented May 24, 2021

If backport <branch> label is applied to a PR,
once the PR is merged, github-actions bot will create another PR targeting
and cherry-picking commits.

example

It takes a couple of minutes to fetch full history and all branches when backporting.

Once this PR is merged I'll create backport release-21.05 label.

Where do I document this workflow for maintainers?

If "backport <branch>" label is applied to a PR,
once the PR is merged, github-actions bot will create another PR targeting
<branch> and cherry-picking commits.
@dotlambda
Copy link
Member

Where do I document this workflow for maintainers?

What about here?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/automating-pr-backports/12565/2

@FRidh
Copy link
Member

FRidh commented May 24, 2021

Note we also have the 9.needs: port to stable label already. Of course, there is the ambiguity here when we have two stable branches.

@jtojnar
Copy link
Member

jtojnar commented May 24, 2021

We could have 9.needs: port to oldstable. Or have a command if it needs to be more specific.

@jonringer
Copy link
Contributor

jonringer commented May 24, 2021

We could have 9.needs: port to oldstable. Or have a command if it needs to be more specific.

I would rather have explicit branch targets. Less likely to have issues with EOL windows (where there's two "supported stable releases); and less reliance on someone (probably a release manager) remembering to update what's referenced by old / new, and then communicating that to entire community.

EDIT: english... my only weakness.

EDIT EDIT: Also, explicit branch targets may allow for edge cases like backporting something to an EOL release (e.g. critical CVEs such as the recent bash vulnerability).

.github/workflows/backport.yml Outdated Show resolved Hide resolved
.github/workflows/backport.yml Outdated Show resolved Hide resolved
.github/workflows/backport.yml Show resolved Hide resolved
Co-authored-by: zowoq <59103226+zowoq@users.noreply.github.com>
@Mic92
Copy link
Member

Mic92 commented May 25, 2021

Great, it even does the -x thing: https://github.com/zeebe-io/backport-action/blob/9b8949dcd4295d364b0939f07d0c7593598d26cd/backport.sh#L61

@Mic92
Copy link
Member

Mic92 commented May 25, 2021

We have backport documentation in doc/contributing/submitting-changes.chapter.md where this could go in.

@domenkozar
Copy link
Member Author

Note we also have the 9.needs: port to stable label already. Of course, there is the ambiguity here when we have two stable branches.

The action doesn't yet support customizable label naming so for now we'll have to live with some duplication in labels.

@Synthetica9
Copy link
Member

I think we should wait with merging this until upstream allows for configurable branch names, personally. It seems like they are very responsive in the linked issue, so this shouldn't take too long it seems.

@FRidh
Copy link
Member

FRidh commented May 25, 2021

I think we should wait with merging this until upstream allows for configurable branch names, personally. It seems like they are very responsive in the linked issue, so this shouldn't take too long it seems.

I think this feature is so important that we should not wait. What happened now often is that backport PR's are opened before the original is merged.

Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Assuming it is tested this looks good to me.

@domenkozar
Copy link
Member Author

I also don't see why this should block us.

@FRidh
Copy link
Member

FRidh commented May 25, 2021

@domenkozar Do you want to be a codeowner for this workflow?

@dotlambda
Copy link
Member

dotlambda commented May 25, 2021

If we merge it now and change the label names later, that will create a big problem for existing PRs.
EDIT: label names, not branch names

@FRidh
Copy link
Member

FRidh commented May 25, 2021

If we merge it now and change the label names later, that will create a big problem for existing PRs.
EDIT: label names, not branch names

Why? Labels can be renamed. Also, if we have two labels for the same branch we drop one of them and replace them by the other.

Copy link
Member

@Synthetica9 Synthetica9 left a comment

Choose a reason for hiding this comment

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

Oh, I didn't know a global label rename was possible. In that case, LGTM

@domenkozar domenkozar merged commit d71078f into master May 25, 2021
@ldesgoui ldesgoui mentioned this pull request May 25, 2021
9 tasks
@gebner
Copy link
Member

gebner commented May 25, 2021

Will this action also work for PRs to the staging branch?

@Synthetica9
Copy link
Member

I don't see a reason you couldn't do backport staging-21.05

@SuperSandro2000 SuperSandro2000 deleted the backporting-action branch May 25, 2021 12:43
@mweinelt
Copy link
Member

There seems to be a permissions problem with the action.

https://github.com/NixOS/nixpkgs/actions/runs/875335611

@korthout
Copy link

korthout commented May 25, 2021

@domenkozar It seems the action does not have permissions to push to your repo

/usr/bin/git push --set-upstream origin backport-124385-to-release-21.05
remote: Permission to NixOS/nixpkgs.git denied to github-actions[bot].
fatal: unable to access 'https://github.com/NixOS/nixpkgs/': The requested URL returned error: 403

The action also does not have permissions to comment or open pull requests:

Create comment: The process '/usr/bin/git' failed with exit code 128
Resource not accessible by integration
Error: Resource not accessible by integration

@domenkozar
Copy link
Member Author

We'll have to use pull_request_target and see if that doesn't break the action. I'll prepare a fix tomorrow.

@jonringer
Copy link
Contributor

This also doesn't work for rebased (or probably squashed) commits either.

https://github.com/NixOS/nixpkgs/runs/2671807530?check_suite_focus=true

@korthout
Copy link

This also doesn't work for rebased (or probably squashed) commits either.

I've opened an issue for this topic korthout/backport-action#46

@domenkozar
Copy link
Member Author

Opened #124760 for the permissions fix!

@zowoq
Copy link
Contributor

zowoq commented May 29, 2021

We may want to automate cleanup of the backport-***-to-*** branches or see about getting the action to submit the PRs from a forked repo (bot user).

Using a bot user would also mean actions aren't bypassed in the backport PRs (eg. wait-for-ofborg), the default github actions token isn't allowed to trigger other actions.

@davidak davidak mentioned this pull request Jun 1, 2021
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.