contrib: Request author review during backports

[joestringer]

When submitting backports, request the original author's review.

Suggested-by: Paul Chaignon paul@cilium.io

[joestringer]

(Note, I tried the command manually myself against some backport files I had lying around, but I have not tried out the full script after integrating the command into the script).

[aanm]

Changes look good but I think certain guardrails need to be done, I think there's a limit in the number of reviewers a PR can have. Or am I mixing it up with assignees?

[joestringer]

Yeah, there's a limit of 15 (10 for assignees):

The list is sorted / uniqued via sort -u, do you think it will be common to have >15 unique authors in a backport PR?

I'm also not sure what the impact is in the case where this limit is exceeded.

If we think this could work, we can try it, but I also don't mind dropping this if we don't think it will work.

[pchaigno]

Thanks for taking things of my TODO list! 😄

(Note, I tried the command manually myself against some backport files I had lying around, but I have not tried out the full script after integrating the command into the script).

Did you check what happens when an external contributor's review is requested? It shouldn't be possible to do that (at least not in the WebUI), but I'm guessing we'd rather the CLI ignores that reviewer or warns rather than failing hard.

If we think this could work, we can try it, but I also don't mind dropping this if we don't think it will work.

I haven't seen a lot of backport PRs with >10 reviewers. It can happen for the first few batches after a minor release is made though. I think it's worth trying.

[joestringer]

Did you check what happens when an external contributor's review is requested? It shouldn't be possible to do that (at least not in the WebUI), but I'm guessing we'd rather the CLI ignores that reviewer or warns rather than failing hard.

Nope, I have not tested the end-to-end of this at all. I just checked the format of the input args, and ran the commands against the generated summary file to assemble the list of reviewers in a format that looks right.

We could either convince one of the upcoming backporters to try out these changes during their next round, or we could merge & iterate from the tree (or revert if it's completely broken for some unintended reason).

[pchaigno]

We could either convince one of the upcoming backporters to try out these changes during their next round, or we could merge & iterate from the tree (or revert if it's completely broken for some unintended reason).

👍 to merge.

[pchaigno]

Marking ready to merge.

[joestringer]

Looks like there is a drawback to this:

$ submit-backport                                                                                                                   
Using GitHub repository joestringer/cilium (git remote: origin)                                                                                                                     
Sending PR for branch v1.9:
                                             
v1.9 backports 2021-07-15 (2)                                                             
                                                                                          
 * #16589 -- vagrant: Bump all Vagrant box versions (@pchaigno)
   * Minor conflicts since the box versions were completely different
 * #16804 -- contrib: Explicitly set remote for backport branches (@twpayne)
 * #15999 -- Improve logging when cgroupfs mount fails (@johngv2)
   * Had to rebase across file rename pkg/cgroups/cgroups{,_linux}.go
                                                                                          
Once this PR is merged, you can update the PR labels via:                                                                                                                           
````upstream-prs                                                                           
$ for pr in 16589 16804 15999; do contrib/backporting/set-labels.py $pr done 1.9; done                                                                                              

                                                                                          
Sending pull request...                                                                                                                                                             
remote:                                                                                   
remote: Create a pull request for 'pr/v1.9-backport-2021-07-15-2' on GitHub by visiting:
remote:      https://github.com/joestringer/cilium/pull/new/pr/v1.9-backport-2021-07-15-2
remote:                                      
Error requesting reviewer: Unprocessable Entity (HTTP 422)
Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the cilium/cilium repository.
                                                                                          
                                                                                          
Signal ERR caught!                                                                        
                                                                                          
Traceback (line function script):
58 main /home/joe/git/cilium/contrib/backporting/submit-backport

Immediate workaround I used was just to remove the @ sign in front of the relevant external contributor name.

Would be nice if there was a way to ignore these errors... haven't looked into this yet.

[pchaigno]

Immediate workaround I used was just to remove the @ sign in front of the relevant external contributor name.
Would be nice if there was a way to ignore these errors... haven't looked into this yet.

I couldn't find a simple workaround either. I'm a bit worried folks will forget to update labels (the last step in the script) when they hit this. Maybe we should warn instead of failing when we hit this?

[joestringer]

@pchaigno The error comes from GitHub (HTTP 422), so I don't think we have control over this.

Best ideas I have right now are either:

• Query https://github.com/orgs/cilium/teams/committers to filter review-requests
• Remove the automatic review request
• File an issue with Github to turn this into a warning

[pchaigno]

The error comes from GitHub (HTTP 422), so I don't think we have control over this.

Yep, but it still creates the PR before erroring out. So we could issue a warning when hub fails and continues with the label updates. To ensure we don't continue if the PR creation failed, we could grep on the hub error. It's a not very clean, but a lot easier than querying the GH API to filter reviewers.

[joestringer]

Ah okay, I did not realize the PR still gets created. Sounds reasonable enough 👍

[joestringer]

Something like this perhaps?

$ git diff
diff --git a/contrib/backporting/submit-backport b/contrib/backporting/submit-backport
index 3c2df45269e2..b45968d8385c 100755
--- a/contrib/backporting/submit-backport
+++ b/contrib/backporting/submit-backport
@@ -58,7 +58,14 @@ git push -q "$USER_REMOTE" "$PR_BRANCH"
 if [ -z "$AUTHORS" ]; then
   hub pull-request -b "v$BRANCH" -l kind/backports,backport/$BRANCH -F $SUMMARY
 else
-  hub pull-request -b "v$BRANCH" -l kind/backports,backport/$BRANCH -F $SUMMARY -r $AUTHORS
+  hub_out=$(mktemp)
+  if ! hub pull-request -b "v$BRANCH" -l kind/backports,backport/$BRANCH -F $SUMMARY -r $AUTHORS | tee $hub_out; then
+    if ! grep -q "Error requesting reviewer" $hub_out; then
+      rm -- "$hub_out"
+      common::exit 1 "Failed to submit Pull Request to GitHub, exiting."
+    fi
+  fi
+  rm -- "$hub_out"
 fi
 
 prs=$(grep "contrib/backporting/set-labels.py" $SUMMARY | sed -e 's/^.*for pr in \([0-9 ]\+\);.*$/\1/g')

Untested; not sure whether hub outputs errors to stderr or stdout and could be missing some basic bash syntax.