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.

Sign up for GitHub

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

Jump to bottom

gerrit: implement CL auto-submit #48021

Closed
rolandshoemaker opened this issue Aug 27, 2021 · 12 comments
Closed

gerrit: implement CL auto-submit #48021

rolandshoemaker opened this issue Aug 27, 2021 · 12 comments
Assignees
@dmitshur @rolandshoemaker
Labels
Builders x/build issues (builders, bots, dashboards) FeatureRequest FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@rolandshoemaker
Copy link
Member

(similar to #12482, but this issue is intended to track the work of actually implementing and deploying this functionality)

Authors should be able to add a "Auto-Submit" label to CLs, such that once all submission requirements are met the CL is automatically submitted. The usage "Auto-Submit" label should be restricted to the existing 'approvers' Gerrit group.

The basic conditions for triggering auto-submit should be (the first two of these can be boiled down to 'submittable' according to Gerrit):

  • Trusted
  • Code-Review+2
  • TryBot-Result+1
  • No unresolved comments

The one main open question is what restrictions should be in place around 'unreviewed' changes. Currently once a reviewer has +2'd a CL, if the author is in 'approvers' they can make changes to the CL before it is submitted (this is a common pattern, as reviewers will often +2 with small comments that the author should address before submitting). If we maintain the "Auto-Submit" label across patch sets, this behavior would essentially be maintained when using auto-submit functionality (since any comments would still need to be resolved, and a new TryBot-Result+1 would be required), although reviewers may be slightly less vigilante about looking at unreviewed changes which are submitted.

Since the author would still need to re-label with "Run-TryBot" after pushing a new patch set, it seems reasonable though to explicitly require them to also re-label with "Auto-Submit". This wouldn't really provide any extra security, but would make the author explicitly say they want their new patch set auto-submitted. Another approach would be to require there be no new patch set between the last Code-Review+2 and the auto-submit being triggered. This would explicitly require the reviewer to approve any changes since their last review, but doesn't necessarily add any additional security, since the author would still be able to manually submit their patch, and seems likely to just reduce the value of this feature.

I'm in favor of not implementing any additional restrictions, and simply saying if the change has Code-Review+2 and there are no unresolved comments (and all other submittability(?) requirements are satisfied), allowing follow-up unreviewed changes to be submitted. This would maintain the status-quo, relying on the assumption that members of the 'approvers' group should be generally trusted not to submit major unreviewed changes and that reviewers should be aware of what unreviewed changes were submitted (i.e. that there weren't changes outside of what they requested in the submitted CL).

There are two major components of this change:

  • Adding the auto-submit functionality to x/build/cmd/gopherbot
  • Adding the "Auto-Submit" label to the gerrit config

cc @golang/release @golang/security
@rolandshoemaker rolandshoemaker added the Builders x/build issues (builders, bots, dashboards) label Aug 27, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/341212 mentions this issue: cmd/gopherbot: add auto-submit functionality

@FiloSottile
Copy link
Contributor

Yeah, no need for extra restrictions, however I think that an explicit opt-in is good UX, so I'd take Russ's suggestion to make Gerrit clear AutoSubmit+1 on new patch sets. You should add an -autosubmit flag to git-codereview mail to set the vote, which would make it less onerous.

This also has a limited security advantage in that Gerrit (or GitHub via gerritbot) can be misconfigured to allow others to add a PS to someone's CL, and AutoSubmit should not carry across.

@cherrymui cherrymui added FeatureRequest NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Aug 30, 2021
@cherrymui cherrymui added this to the Unreleased milestone Aug 30, 2021
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Sep 7, 2021
gopherbot pushed a commit to golang/build that referenced this issue Jan 21, 2022
@rolandshoemaker
cmd/gopherbot: add auto-submit functionality
31d859d 
If a CL has been labeled with "Auto-Submit", is submittable according
to Gerrit, has a positive TryBot-Result vote, and has no unresolved
comments then submit the change.

This requires adding a new gerrit.Client method for the CL submission
endpoint.

Updates golang/go#48021

Change-Id: I3d5dafd1ca25a3cac5a40d7e9a744ba12ab44cae
Reviewed-on: https://go-review.googlesource.com/c/build/+/341212
Trust: Roland Shoemaker <[email protected]>
Run-TryBot: Roland Shoemaker <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/383174 mentions this issue: maintner/maintnerd/maintapi: update links in comments

gopherbot pushed a commit to golang/build that referenced this issue Feb 4, 2022
@dmitshur @gopherbot
maintner/maintnerd/maintapi: update links in comments
807e87e 
Also use this CL as a test run for golang/go#48021.

Change-Id: I65cae5cab1713bb03122ac8137f3587d519375b4
Reviewed-on: https://go-review.googlesource.com/c/build/+/383174
Reviewed-by: Heschi Kreinick <[email protected]>
Trust: Dmitri Shuralyov <[email protected]>
Auto-Submit: Dmitri Shuralyov <[email protected]>
Run-TryBot: Dmitri Shuralyov <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
@dmitshur
Copy link
Contributor

dmitshur commented Feb 4, 2022 •
edited
Loading

I performed a controlled test run on https://go-review.googlesource.com/c/build/+/383174, watching it closely. There weren't any unexpected findings, everything worked as intended.

Here's a log of events with some additional logging/information about decisions gopherbot took:

1. Send a CL with Auto-Submit+1, CL should not show up during dry-run

	>>> bailing because Auto-Submit (approved by user 6005) and TryBot-Result (<nil>) aren't approved

2. Run TryBots so patchset has TryBot-Result+1, CL should not show up during dry-run

	>>> bailing because !changeInfo.Submittable

3. Add Code-Review+2 (but not both Trust+1), CL should not show up during dry-run

	>>> bailing because !changeInfo.Submittable

4. Add second Trust+1, CL should show up during dry-run

	2022/02/04 12:24:07 [dry-run] would've submitted CL https://golang.org/cl/383174 ...

5. Add an unresolved comment, CL should not show up during dry-run

	>>> bailing because thread is unresolved 27f316d4_6ad5c7c7

6. Resolve comment, CL should show up during dry-run

	2022/02/04 12:26:44 [dry-run] would've submitted CL https://golang.org/cl/383174 ...

7. Push new patchset, CL should not show up during dry-run

	this cleared both TryBot-Result and Auto-Submit votes

	>>> bailing because Auto-Submit (<nil>) and TryBot-Result (<nil>) aren't approved

8. Re-add Auto-Submit+1, CL should not show up during dry-run

	>>> bailing because Auto-Submit (approved by user 6005) and TryBot-Result (<nil>) aren't approved

9. Run TryBots so patchset has TryBot-Result+1, CL should show up during dry-run

	2022/02/04 12:39:01 [dry-run] would've submitted CL https://golang.org/cl/383174 ...

10. Run gopherbot without --dry-run, CL should be submitted

	2022/02/04 12:41:40 submitting CL https://golang.org/cl/383174 ...

(Thanks @rolandshoemaker for coming up with the sample run sequence.)

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/383274 mentions this issue: cmd/gopherbot: remove auto-submit restriction

gopherbot pushed a commit to golang/build that referenced this issue Feb 7, 2022
@rolandshoemaker
cmd/gopherbot: remove auto-submit restriction
7517b10 
Run the Auto-Submit action alongside everything else now.

Updates golang/go#48021

Change-Id: If7a109bfe0dc64e5b488713415193129e102640a
Reviewed-on: https://go-review.googlesource.com/c/build/+/383274
Reviewed-by: Dmitri Shuralyov <[email protected]>
Trust: Roland Shoemaker <[email protected]>
Run-TryBot: Roland Shoemaker <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/386294 mentions this issue: cmd/gopherbot: handle threads of 3 and more comments in Auto-Submit task

gopherbot pushed a commit to golang/build that referenced this issue Feb 18, 2022
@dmitshur @gopherbot
cmd/gopherbot: handle comment threads with 3+ depth in Auto-Submit task
904a0c7 
The current logic to find if a CL has any unresolved threads is to
iterate over all comments in chronological order, and use the resolved
state of most recent comments to update the overall resolved state of
the corresponding thread. Threads are identified by the ID of the root
(earliest) comment whose InReplyTo field is zero.

When determining the thread ID that a reply is associated with, it's
important to follow the loop of InReplyTo pointers all way to the top
of the thread. That is, if there's a thread with 3 comments in total:

	A ← B ← C

(Where A is an initial comment, B is a reply to it, and C is a reply
to B.)

Then the resolved state of comment C needs to update the resolved state
of thread A, even though comment C is a direct reply to comment B.

The existing code worked for threads with depths 1 and 2, but not 3+.
To handle threads with deeper nesting, add a 'roots' map that tracks
the root ID for each comment seen during the chronological iteration.

I thought this would apply to the equivalent logic in coordinator, and
probably should have checked sooner, as then I'd find this was already
implemented there in CL 317971. This change applies some comments and
code style decisions from coordinator's listPatchSetThreads function
so they're more in sync.

For golang/go#48021.

Change-Id: I93baff45aae511a60ccfce4e97b05d2a1b358e1f
Reviewed-on: https://go-review.googlesource.com/c/build/+/386294
Run-TryBot: Dmitri Shuralyov <[email protected]>
Reviewed-by: Roland Shoemaker <[email protected]>
Trust: Dmitri Shuralyov <[email protected]>
Auto-Submit: Dmitri Shuralyov <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
@rolandshoemaker rolandshoemaker removed the NeedsFix The path to resolution is known, but the work has not been done. label Mar 11, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/391734 mentions this issue: cmd/gopherbot: don't autosubmit CLs with wait-release

gopherbot pushed a commit to golang/build that referenced this issue Mar 11, 2022
@rolandshoemaker @gopherbot
cmd/gopherbot: don't autosubmit CLs with wait-release
aa7fa4b 
If a CL has the "wait-release" hashtag set, don't autosubmit it, since
we are presumably in the freeze, and should wait for the tree to open.

Updates golang/go#48021
Updates golang/go#24836

Change-Id: I23258ccbb71d2b8febe97ad8883b9fe9f90c761d
Reviewed-on: https://go-review.googlesource.com/c/build/+/391734
Reviewed-by: Dmitri Shuralyov <[email protected]>
Trust: Roland Shoemaker <[email protected]>
Run-TryBot: Roland Shoemaker <[email protected]>
Auto-Submit: Roland Shoemaker <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/396894 mentions this issue: cmd/gopherbot: don't autosubmit CLs with wait-release topic

@rolandshoemaker rolandshoemaker mentioned this issue May 13, 2022
Open
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/406237 mentions this issue: cmd/gopherbot: revise autosubmit behavior for stacks

gopherbot pushed a commit to golang/build that referenced this issue May 18, 2022
@rolandshoemaker
cmd/gopherbot: revise autosubmit behavior for stacks
96df98a 
When checking whether to autosubmit a change in a stack, ignore whether
the revision of merged/abandoned parents are current (base revision ==
current revision). There are multiple reasons a merged parent may not be
current (the most obvious being that the parent change was submitted,
which increases the revision number, but the child was not rebased onto
the new revision), but as long as the change is still considered
'submittable' by gerrit (i.e. there are no merge conflicts) this should
not materially affect our decision of whether or not to submit the
change (and matches what most users will do when manually submitting a
stack).

For golang/go#48021.

Change-Id: Iceff8a88ac3638671f36175d802254788d2470fd
Reviewed-on: https://go-review.googlesource.com/c/build/+/406237
Reviewed-by: Dmitri Shuralyov <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/431375 mentions this issue: git-codereview: add -autosubmit

gopherbot pushed a commit to golang/review that referenced this issue Oct 3, 2022
@heschi @gopherbot
git-codereview: add -autosubmit
a897dc9 
Add a flag to set the Auto-Submit label.

For golang/go#48021.

Change-Id: If704e8b5e9e0e2521eed78fe28af10d3c31ec3a0
Reviewed-on: https://go-review.googlesource.com/c/review/+/431375
Auto-Submit: Heschi Kreinick <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Russ Cox <[email protected]>
Run-TryBot: Heschi Kreinick <[email protected]>
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 18, 2022
@rolandshoemaker
Copy link
Member Author

We've basically implemented everything described here, any follow-up work can now have it's own issue. 🎉

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/452135 mentions this issue: cmd/gopherbot: rely on Gerrit config for TryBot and resolved comments

@dmitshur dmitshur moved this to Done in Go Release Nov 19, 2022
gopherbot pushed a commit to golang/build that referenced this issue Nov 30, 2022
@dmitshur @gopherbot
cmd/gopherbot: rely on Gerrit config for TryBot and resolved comments
0f1bb4b 
As of go.dev/issue/56031, the need to have passing TryBots and no
unresolved comment threads is enforced by a submit requirement in
Gerrit. The code in the auto-submit task is no longer load-bearing,
so remove it to simplify and to avoid creating a false impression of
it doing something important (beyond little differences, like handling
the TryBot-Bypass label as implemented by the Gerrit submit rule, etc.).

Running the "auto-submit CLs" task in dry-run mode did not report
that it would submit any CLs.

For golang/go#48021.
Updates golang/go#56031.

Change-Id: I09c6900199d26bd8e90fe7f7f681fd6227a762e8
Reviewed-on: https://go-review.googlesource.com/c/build/+/452135
Reviewed-by: Roland Shoemaker <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Auto-Submit: Dmitri Shuralyov <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Dmitri Shuralyov <[email protected]>
@golang golang locked and limited conversation to collaborators Nov 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@dmitshur dmitshur

@rolandshoemaker rolandshoemaker

Labels
Builders x/build issues (builders, bots, dashboards) FeatureRequest FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
Go Release
Archived in project
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@FiloSottile @dmitshur @rolandshoemaker @gopherbot @cherrymui