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

feature request: use source PR labels to pick branches to backport to #157

Closed
ronkorving opened this issue Oct 8, 2019 · 5 comments · Fixed by #180
Closed

feature request: use source PR labels to pick branches to backport to #157

ronkorving opened this issue Oct 8, 2019 · 5 comments · Fixed by #180

Comments

@ronkorving
Copy link

ronkorving commented Oct 8, 2019

First off: awesome project! Thank you for your work.

I have a feature request, and would love to hear your thoughts. It would be really nice if it were possible to create some feature PR that will later need to be backported. Often you know beforehand where it should be backported to. One could use GitHub labels to mark that all PRs with label backport-v1 should later be backported to the v1 branch. If a label system like that is applied, running backport --pr 123 could read those labels and preselect all the right branches.

Being able to determine ahead of time where we backport to would really improve our workflow. I'm guessing we may not be alone in that.

@sorenlouv
Copy link
Owner

sorenlouv commented Apr 13, 2020

Hey @ronkorving!

Sorry for the slow reply. I think is a great idea. I'm still not settled on a particular implementation but I'll sketch a rough idea of what I think. LMK if you have any suggestions to make.

versionLabelRegex
I'm proposing that to enable this feature the user must update .backportrc.json with versionLabelRegex:

{
  "upstream": "elastic/kibana",
  "branches": [{ "name": "7.x", "checked": true }, "7.7", "7.6"],
  "versionLabelRegex": "^backport-(v\d+)$"
}

versionLabelRegex is a regex that'll extract the target branch from labels. In this case the regex will match backport-v1 (v1), backport-v2 (v2) and so forth.

We can also make it an array if multiple matchers are necessary:

{
  "upstream": "elastic/kibana",
  "branches": [{ "name": "7.x", "checked": true }, "7.7", "7.6"],
  "versionLabelRegexes": ["^backport-(v\d+)$"]
}

versionLabels
An alternative to versionLabelRegex is versionLabels, where labels are directly mapped to a branch:

{
  "upstream": "elastic/kibana",
  "branches": [{ "name": "7.x", "checked": true }, "7.7", "7.6"],
  "versionLabels": {
    "backport-v1": "v1",
    "backport-v2": "v2",
    "custom-label-v3": "v3",
  }
}

This approach allows for more flexibility but is also more verbose.

Do you have any preferences for versionLabelRegex or versionLabels?

Running backport
Both approaches will work intuitively when backporting individual PRs like backport --pr 123: if one or more applicable labels are found, the PR will be backported accordingly without asking the user any additional questions.

But what should happen if the user runs something like backport --multiple? They will now be prompted to select one or more commits to backport, which might not all have version labels.
The user could select a commit which does have version labels, and a commit which doesn't.
In that case should the user then be prompted to select branches to backport to? And should that selection override the version labels or only apply to the commit that doesn't have version labels?

Is it possible that someone might want to override the label logic, and backport a commit to other branches?

LMK if it makes sense and whether you have any opinion on this matter.

@ronkorving
Copy link
Author

Hey, thanks for the extensive response and proposals.

Configuration

About versionLabelRegex vs versionLabels, I think a hybrid might be superior. I'll give a real world example from a project I'm working on. My master branches are v1-master, v2-master, v2.1-master, etc. If I want a label v1, the regexp approach seems tricky, because I cannot append -master in the result. So I would have to label v1-master. This is not the end of the world of course, but now is the time to be nitpicky I guess, before things get built.

versionLabels are a solution, but they are not just verbose, they also require upkeep.

How about a hybrid (example adapted to my scenario):

{
  "versionLabels": {
    "some-special-branch": "some-special-label",
    "v([0-9]+)-master": "v$1"
  }
}

This allows both regular expression replacement, and simple mapping to co-exist.

The $1 syntax is based on how String.prototype.replace works. This does of course make the $ symbol a special character, but I imagine that's worth it. One could always allow $$ to mean a literal $ if we really need it.

Running backport

To be honest, I don't have strong opinions or ideas on this at this time, especially as I haven't yet really used --multiple yet. I think conflicts should prompt the user to act. If at that point, the labels can be displayed, and the choice can be made manually (incl. the choice to abort), I think it should be fine?

@sorenlouv
Copy link
Owner

sorenlouv commented Apr 14, 2020

How about a hybrid (example adapted to my scenario):

I like it 👍

Running backport

For the initial implementation I'll enable this behaviour when running backport --pr and backport --sha and leave it out for backport --multiple until I have a better idea of how it should behave.

@spalger I think you had a suggestion similar to this a long time ago. Would the hybrid approach work for your use case?

@sorenlouv
Copy link
Owner

@ronkorving This has been released in v5.2.0. I ended up naming the option branchLabelMapping. You can find the documentation here.

LMK if it works like it should. If you have any other suggestions we can iterate on it.

@ronkorving
Copy link
Author

@sqren Flippin' awesome! :) I will let you know if anything comes up (although tbh, it may take a while before I get my hands on it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants