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

Port origin PR labels to backport PR #87

Closed
mweinelt opened this issue Jul 5, 2021 · 10 comments · Fixed by #327
Closed

Port origin PR labels to backport PR #87

mweinelt opened this issue Jul 5, 2021 · 10 comments · Fixed by #327
Labels
enhancement New feature or request

Comments

@mweinelt
Copy link

mweinelt commented Jul 5, 2021

Allow a set of labels to be automatically applied onto the backport PR.

Use case for us (NixOS/nixpkgs) would be labels like Severity: Security, which also applies to the backport PR and right now needs to be remembered to be applied when the PR gets created.

@korthout
Copy link
Owner

korthout commented Jul 6, 2021

Hi @mweinelt. Thanks for the great idea. I can definitely see your and other use cases for this.

There's some scoping to be done here though. For example, we could define a set of labels that are always set on backport pull requests, but we can also copy labels from the original pull request. Copying also comes with some interesting edges when considering which labels to copy, e.g. simply all labels, or all labels that are not a backport label (so it doesn't trigger the action again), all matching some configurable regex pattern, or all from a configurable set.

I'd be curious to hear from you what would be most fitting to your case, so I can keep the initial change small but also consider how to expand it in the future.

@mweinelt
Copy link
Author

mweinelt commented Jul 6, 2021

Thanks for getting back to me so fast.

Flexibility wise I think regular expressions are the way to go. To keep complexity in check maybe allow a list of multiple simpler patterns that would each be sufficient for a label to be backported.

Does that sound sensible to you?

@korthout korthout added the enhancement New feature or request label Jul 7, 2021
@korthout
Copy link
Owner

korthout commented Jul 7, 2021

allow a list of multiple simpler patterns

I'm not sure about this. Regular expressions already support matching to multiple patterns using the |, e.g. abc|def. Do you think another listing syntax would be better suitable? And if so, what should that look like? It's definitely more complex to build, but if there's good reason for it then I wouldn't mind it.

How about naming of the input? What do you think about copy_labels_pattern? It's focused, so we could separate it from a add_labels, which could just be a comma-separated set of labels.

@Atemu
Copy link

Atemu commented Jul 7, 2021

I think what @mweinelt meant was something like pre-made regex patterns for simple cases that you could use instead of having to spin your own (possibly faulty) regex.

"Every label but backport labels" is probably a property a large amount of users would want, so this common case could be built in as a convenience option. Otherwise, all those users would have to declare a regex for that themselves.

It's a way to have maximum configurability while retaining ease of use which is something we quite like in the functional world ;)

@lheckemann
Copy link
Contributor

Hi, I think I'm here for the same reason as mweinelt :)

copy_labels_pattern would be great to have for nixpkgs's use case. Would you accept a PR implementing it?

@korthout
Copy link
Owner

korthout commented May 11, 2022

@lheckemann I would definitely welcome a PR for this, but let's agree first on the design.

We might be able to simplify the feature by copying all non-backport labels. For example, a boolean input copy_labels. When set to true, it copies all labels from the original PR except those that match the label_pattern input.

What do you think?

EDIT: on second thought, that's exactly what @Atemu described 💡 "Every label but backport labels"

@mohe2015
Copy link

Isn't an implementation based on a regex more flexible and similar easy to implement? Because I think there may be some labels we don't want to backport e.g. all our rebuild-xxx labels. I don't think we necessarily need to optimize for ease of use. Flexibility may be more important in my opinion if you want to keep the implementation simple.

@korthout
Copy link
Owner

@mohe2015 That actually makes sense. Flexibility first, ease of use second.

@lheckemann Let's go for the copy_labels_pattern. To make sure we don't change existing behavior, please use an empty string to mean "don't copy labels using a pattern".

@lheckemann
Copy link
Contributor

Fixed in https://github.com/DeterminateSystems/backport-action/commit/eec52aeb846db9a3395467064b78155a4b5b2ddf

@korthout
Copy link
Owner

korthout commented Dec 10, 2022

@lheckemann Please note that your code is based on changes that are currently under battle-testing in v1-rc1. They should improve the performance for nixpkgs significantly, but I wanted to ensure they behave correctly before releasing it as v1 (planned for the end of this month). The latest stable release is v0.0.9.

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
5 participants