Skip to content

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented May 17, 2021

These can be triggered with comments like /backport to release/5.0

@dotnet/aspnet-build any thoughts on a good place to document this feature?

@wtgodbe wtgodbe requested a review from a team May 17, 2021 17:02
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

I have a lot of questions but, we can iterate if we decide we want to change the rules slightly.

// extract the target branch name from the trigger phrase containing these characters: a-z, A-Z, digits, forward slash, dot, hyphen, underscore
const regex = /\/backport to ([a-zA-Z\d\/\.\-\_]+)/;
target_branch = regex.exec(context.payload.comment.body);
if (target_branch == null) throw "Error: No backport branch found in the trigger phrase.";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest enforcing /\/backport to release\/\d\.\d(-preview\d)?/ to avoid garbage \backport to freddy PRs.

Nit: could just ignore poorly-formed comments i.e. anything that doesn't match the full regular expression.

pr_description_template: |
Backport of #%source_pr_number% to %target_branch%

/cc %cc_users%
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I correct %cc_users% are the person who issued the /backport comment and the author of the containing PR (if distinct)❔ If possible, would be good to at least /cc the PR approvers on the backport PR. Of course, the original author must approve because the should understand whether a fix in 'main' is sufficient without additional work in (say) 'release/2.1'.

Separately, this action doesn't check group membership until fairly late. What is the visible failure if someone w/o rights to create branches in dotnet/aspnetcore adds a /backport to george comment❔


jobs:
backport:
if: github.event.issue.pull_request != '' && contains(github.event.comment.body, '/backport to')
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we confirm the PR has been merged before allowing backports❔

pr_description_template:
description: 'The template used for the PR description. Special placeholder tokens that will be replaced with a value: %target_branch%, %source_pr_title%, %source_pr_number%, %cc_users%.'
default: |
Backport of #%source_pr_number% to %target_branch%
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, would appreciate the final commit hash in this comment


if (error.postToGitHub === undefined || error.postToGitHub == true) {
// post failure to GitHub comment
const unknown_error_body = `@${comment_user} an error occurred while backporting to ${target_branch}, please check the run log for details!\n\n${error.message}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where would users find the "run log"❔

// download and apply patch
await exec.exec(`curl -sSL "${github.context.payload.issue.pull_request.patch_url}" --output changes.patch`);

const git_am_command = "git am --3way --ignore-whitespace --keep-non-patch changes.patch";
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand using the patch allows for back-porting an in-progress PR but

  1. It's not clear to me we want to support in-progress PRs in our repos. Would appreciate others' thoughts on this…
  2. It loses any separation the original author chooses to leave when merging e.g. they might rebase 2 or 3 commits rather than squashing them.

Do the available APIs support determining the final commit or commits when a PR is merged❔

@dougbu
Copy link
Contributor

dougbu commented May 17, 2021

/cc @dotnet/aspnet-build and @dotnet/aspdoi

@dougbu
Copy link
Contributor

dougbu commented May 17, 2021

Other thoughts:

  • If possible, would be good to add backport PR to the Servicing Status project
  • If possible, would be good to make the original PR author an approver of the backport PR
  • If possible, would be good to assign the backport PR to @dotnet/aspnet-build

@wtgodbe
Copy link
Member Author

wtgodbe commented May 18, 2021

deferring most of these questions to @akoeplinger who wrote this code - we're considering moving this to arcade so it may make more sense to work on improvements as we do that. Though Doug does bring up some good points about extensibility, like the ability for repos to specify aliases to tag on these PRs & add them to repo projects.

@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label May 18, 2021
@dougbu dougbu merged commit 70e29d6 into main May 20, 2021
@dougbu dougbu deleted the wtgodbe/action branch May 20, 2021 05:55
@ghost ghost added this to the 6.0-preview5 milestone May 20, 2021
@dougbu
Copy link
Contributor

dougbu commented May 20, 2021

Got this in because I wanna use it 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants