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

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
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

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

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
@toothrot toothrot added this to In Progress in Go Release Team Sep 7, 2021
@cagedmantis cagedmantis moved this from In Progress to Planned in Go Release Team Nov 9, 2021
@dmitshur dmitshur moved this from Planned to In Progress in Go Release Team Nov 12, 2021
@cagedmantis cagedmantis moved this from In Progress to Planned in Go Release Team Jan 4, 2022
gopherbot pushed a commit to golang/build that referenced this issue Jan 21, 2022
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 <roland@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@dmitshur dmitshur moved this from Planned to In Progress in Go Release Team Feb 1, 2022
@gopherbot
Copy link

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
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 <heschi@google.com>
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@dmitshur
Copy link
Contributor

dmitshur commented Feb 4, 2022

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

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
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 <dmitshur@golang.org>
Trust: Roland Shoemaker <roland@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

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
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 <dmitshur@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@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

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
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 <dmitshur@golang.org>
Trust: Roland Shoemaker <roland@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@dmitshur dmitshur removed this from In Progress in Go Release Team Mar 29, 2022
@gopherbot
Copy link

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

@gopherbot
Copy link

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
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 <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
@gopherbot
Copy link

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
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 <heschi@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Heschi Kreinick <heschi@google.com>
@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

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

gopherbot pushed a commit to golang/build that referenced this issue Nov 30, 2022
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 <roland@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
@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.
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
Archived in project
Development

No branches or pull requests

5 participants