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

feat(cherry-pick): add 'backport' as a recongized cherry-pick command #17

Closed
wants to merge 3 commits into from

Conversation

kevinawoo
Copy link
Member

Adds back in cherry-pick and adds an alias backport, which reverts spinnaker/spinnaker.github.io#1969.

Soo.... @maggieneterval, @ezimanyi it looks like @spinnakerbot is back doing cherry-picks!

@plumpy and I were talking about how there wasn't any cherry-picks/backports found using when using the runbook's cherry-pick open PRs query. For example, I had to search for backport directly and look thought them to find any missed ones, like spinnaker/igor#762.

I thought spinnaker/spinnaker.github.io#1952 would work, but then it occurred to me to try out a non Spinnaker org member. They couldn't mention @ spinnaker/release-managers, and I didn't get a notification. Example test case: spinnaker/spinnaker.github.io#1969.

The github docs seem to suggest no, but it's not super explicit that public members can't.

@maggieneterval
Copy link
Contributor

Hey @kevinawoo, thanks for the explanation! To make sure I understand, is the root problem you're trying to solve here discoverability by the release manager of PRs intended for backporting? Making the backport command runnable by all community members does solve that on a certain level, in that now any community member will be able to have Mergify create a backport PR, and that will be discoverable using the existing query for all PRs against a non-master branch.

However, given the new stricter backport criteria, I'd imagine that contributors would want a way to communicate their intent to backport a given PR as soon as possible, so the release manager is providing feedback on the PR against master, and not later once they've already created a backport PR. Does this seem right based on your experience so far? To keep the feedback loop as tight as possible, perhaps we could create a new label like backport-candidate that contributors (or other community members interested in the fix) could apply to open (or, I suppose, merged) PRs against master. Release managers could then query for PRs with that label and either approve or reject them for backporting. If public members can't add labels to PRs, then we could document using the spinnakerbot command to add this label, but either way, I'd rather have release managers providing feedback on backport candidates as soon as possible, and therefore would prefer we make it easier to for release managers to discover these candidates rather than make it easier for contributors to move forward with a PR against a release branch without the release manager ever looking at the original PR against master.

One drawback to this alternate solution is it adds yet another step to the backport process, which based on this release cycle, we've already made sufficiently burdensome as to greatly reduce the number of proposed backports. That said, I think one more step up-front is probably preferable than a likely unhappy path I foresee happening if we open up the backport command to the entire community, which is that the release manager then needs to audit each original PR and investigate whether it meets the backport criteria after it's already been merged (the Mergify commands only work post-merge), and possibly request changes that will then need to be addressed in a separate PR.

@kevinawoo
Copy link
Member Author

To make sure I understand, is the root problem you're trying to solve here discoverability by the release manager of PRs intended for backporting?

Yep. I have a hard time finding those PRs that the community wants backported.


However, given the new stricter backport criteria, I'd imagine that contributors would want a way to communicate their intent to backport a given PR as soon as possible, so the release manager is providing feedback on the PR against master, and not later once they've already created a backport PR. Does this seem right based on your experience so far?

🤔 Hmm... I see, interesting. Deck PR is an example where I think it worked out well where a user was trying to backport, but then a release-manager steps in to accept or not. The user was showing intent to backport to 1.20, but there was no an intent for 1.19 or 1.21 at the time. On the same PR against master, we were able to see 1.20 backport closed with a comment. I believe a label backport-candidate/1.19 would work well in this situation.


To keep the feedback loop as tight as possible

💯, absolutely 👍


... create a new label like backport-candidate that contributors (or other community members interested in the fix) could apply to open (or, I suppose, merged) PRs against master.

Non Spinnaker org members cannot add labels 😞, we'd have to have @spinnakerbot do it.


As for accepting the backport, I already have to audit each PR to see if it is acceptable for the criteria, no matter the method of notification. I've been making sure to write down why I've accepted the backport and why it follows the policy.... juuust incase! Especially since I'm forgetful.

igor example

image

orca example

image


If we were to have @spinnakerbot do it, how would we indicate which version we'd want?

  • @spinnakerbot backport 1.19
  • spinnakerbot adds labels: backport-candidate/1.19
  • release-managers can now query for backport-candiate/1.19, backport-candiate/1.20, and backport-candiate/1.21 ? (note, there would be 3 separate queries as Github doesn't support boolean OR)
  • after a PR is merged someone (@spinnakerbot or release-manager) needs to remove the tag. I think it adds confusion, if it wasn't backported and the tag is still on there. I would probably opt to swap out the backport-canidate/1.19 for backported/1.19 tag on the master PR.

@maggieneterval
Copy link
Contributor

It sounds like your primary concerns with labels indicating backport candidacy are:
(1) We would need three separate queries to find backport candidates.
(2) We would need to remove the backport candidacy label if the PR was not backported to reduce confusion.

(1) I would actually recommend we create only one label, backport-candidate, rather than one per supported release, and update the backport candidate instructions to tell contributors that they must indicate which release branches they want the PR to be backported to as part of the PR message. (We can also document that they must run spinnakerbot add-label backport-candidate in order to add the label, since as you noted, non-org members cannot manually add labels.)

(2) I'd recommend we add to the release manager instructions that upon rejecting a backport candidate, they both leave an explanation as a comment (as it looks like you've been doing a 💯 job of) and removing the backport-candidate label from the PR.

I'm happy to update both the release manager and backport criteria documentation to reflect this if we feel comfortable moving forward with this solution.

@kevinawoo
Copy link
Member Author

awesome feedback 👍 💯

🎉 LETS DO IT! 🎉

I'll close this PR then. It looks like all we'll need to do is create that label and spinnakerbot adds can handle it automagically doesn't have any white/black listing.

@kevinawoo kevinawoo closed this Jul 25, 2020
@kevinawoo kevinawoo deleted the feature/backport branch July 25, 2020 00:25
@maggieneterval
Copy link
Contributor

Just testing that this works...

@spinnakerbot add-label backport-candidate

@maggieneterval
Copy link
Contributor

Oh, I guess we have to add new labels to each repository's issue's list of labels, not just spinnaker/spinnaker...

maggieneterval added a commit to maggieneterval/spinnaker.github.io that referenced this pull request Jul 27, 2020
Per [discussion](spinnaker/spinnakerbot#17) with current release manager @kevinawoo, let's improve the user story for both release managers evaluating backport candidates and contributors proposing backport candidates. Updates documentation to reflect the new agreed-upon workflow.
@maggieneterval
Copy link
Contributor

Conclusion: updated the docs per this conversation in this PR, and added the new label to each repo that cares about OSS release branches. Don't worry, I made it the same yellow hex in every repo.

mergify bot added a commit to spinnaker/spinnaker.github.io that referenced this pull request Jul 28, 2020
* feat(docs): update backport documentation

Per [discussion](spinnaker/spinnakerbot#17) with current release manager @kevinawoo, let's improve the user story for both release managers evaluating backport candidates and contributors proposing backport candidates. Updates documentation to reflect the new agreed-upon workflow.

* fix(docs): clarify that backports are only accepted to supported release branches

* Update community/contributing/releasing/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

* Update community/releases/release-manager-runbook/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

* Update community/releases/release-manager-runbook/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

* Update community/contributing/releasing/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

* Update community/releases/release-manager-runbook/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

* Update community/contributing/releasing/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

* Update community/releases/release-manager-runbook/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

* Update community/releases/release-manager-runbook/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

* Update community/contributing/releasing/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

Co-authored-by: Dave Dorbin <ddorbin@google.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Kaixiang-AWS pushed a commit to Kaixiang-AWS/spinnaker.github.io that referenced this pull request Aug 21, 2020
* feat(docs): update backport documentation

Per [discussion](spinnaker/spinnakerbot#17) with current release manager @kevinawoo, let's improve the user story for both release managers evaluating backport candidates and contributors proposing backport candidates. Updates documentation to reflect the new agreed-upon workflow.

* fix(docs): clarify that backports are only accepted to supported release branches

* Update community/contributing/releasing/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

* Update community/releases/release-manager-runbook/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

* Update community/releases/release-manager-runbook/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

* Update community/contributing/releasing/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

* Update community/releases/release-manager-runbook/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

* Update community/contributing/releasing/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

* Update community/releases/release-manager-runbook/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

* Update community/releases/release-manager-runbook/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

* Update community/contributing/releasing/index.md

Co-authored-by: Dave Dorbin <ddorbin@google.com>

Co-authored-by: Dave Dorbin <ddorbin@google.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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.

2 participants