I'll let Andy finish triaging this. My thoughts:

1. Shall this be done?
Yes, but this is a FR, not a BUG.

2. What's best way to do this?
Definitely not for Rietveld.
For Gerrit, the quickest is adding yet another value to CQ label like "CQ when LGTM", and then CQ would only pick it up if Gerrit reports that change can be submitted.
However, this label is not user-friendly. A better one would be to move away CQ labels completely into CQ AE app itself, so that CQ isn't tied to Gerrit itself. And then we can implement nice UI for Gerrit as a plugin.

This is wholly dependent on Gerrit and the CQ themselves having support for autosubmit. Right now, git-cl could set the CQ+2 flag at upload time, but the CQ would immediately say "no LGTM" and unset it.

Putting this in Afterglow. When we get there, it should probably end up blocked on some other bugs around supporting this workflow in the web UI.

Removing Milestone-Afterglow, as it has ceased to have meaning. More refined milestones may be added back in the near future.

Making this a gerrit-only bug, as any changes to git-cl are wholly incidental to the much more major work required to have this functional on the server side.

 Issue 162819  has been merged into this issue.

 Issue 690092  has been merged into this issue.

 Issue 724577  has been merged into this issue.

I had tried and failed to add something like this in Rietveld 2 years ago: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pRCdTg41_TM/BDMjY72fTBEJ


I plan to try again in Gerrit. I believe it will be easier and cleaner to mimic the critique flow in Gerrit.


My plan:

1) Add a new AutoSubmit label (+0..+1) with user friendly descriptive text explaining what the 2 options mean.

2) If the label is set and there is a CR+1 (with no CQ+2) then display a prompt: "The AutoSubmit label is enabled. The change will be automatically sent to the CQ. Proceed?".
  If "Yes" then automatically add CQ+2.
  If "No" then remove the AutoSubmit +1 vote.


The logic in (2) will be done by the commitqueue.js plugin.

This will only work for projects that have the AutoSubmit label enabled so it will be opt-in instead of forcing it on everyone.

What about integrating this directly into Gerrit?

I'm not sure what that would look like.
Any integration into Gerrit would probably have AutoSubmit as part of the core default labels and then directly Submit after approval. For Chromium we would have to disable the directly Submit part and would still need the work in commitqueue.js to interact with the CQ labels. Sounds like much more work vs #c11.

I would rather add this into commitqueue.js and then see if the Gerrit team wants to adopt some of the behavior.

Ah good point. :)

If you do it in pure JS, non-WebUI lgtm giving will not obey "submit on lgtm", which will preclude tools from relying on this feature, although users will probably be fine as most lgtms are given by Web UI.

I'd rather have this feature done as Java side plugin, storing autosubmit state inside CL metadata and reliably running on all CR label changes. You may not even need the new label. The upside is that it'll be easier to integrate into Gerrit core eventually :) The downside is a lot slower deployment cycle.

orthogonal: agable@ has been working on refactoring commitqueue.js into something more modular/maintainable.

> If you do it in pure JS, non-WebUI lgtm giving will not obey "submit on lgtm", which will preclude tools from relying on this feature, although users will probably be fine as most lgtms are given by Web UI.

Right. Bots should give their own CR+1 and CQ+2 and should not rely on this feature (Skia has a bunch of bots that does this right now). If there is an LGTM giving tool then it would need to learn about the AutoSubmit label. The only target of this feature should be users on the Web UI.

> I'd rather have this feature done as Java side plugin, storing autosubmit state inside CL metadata and reliably running on all CR label changes. You may not even need the new label. The upside is that it'll be easier to integrate into Gerrit core eventually :) The downside is a lot slower deployment cycle.

I would much rather add it to the existing commitqueue.js/chromium-behavior.html plugins that deal with similarish behavior. Not sure if we ever plan for those to be eventually changed to Java plugins.

> orthogonal: agable@ has been working on refactoring commitqueue.js into something more modular/maintainable.

Not completely orthogonal, see my next comment.

Chatted about this with agable@ before the long weekend. Highlights:

* Add changes in chromium-behavior.html, not in commitqueue.js.

* There are two main flows that will have to be accounted for- CR+1 via the reply dialog and CR+1 from the button on the main page.

* Reply dialog interaction should probably be like this:
  When users click CR+1 a msg should popup inside the reply dialog that says "This change has Autosubmit+1, so we'll also set CQ+2. Unselect that if you don't want to trigger it."
  We want to do this because do not want more popups and more clicks than absolutely required.
  This will likely need additions to the reply box API to watch for label changes (like we do for keystrokes right now).

* CR+1 button on the main page interaction should probably be like this:
  Replace the CR+1 button with an identical one that calls a new custom fuction (similar to the "Submit to CQ" button). The custom function would display a popup asking if CQ+2 should be set because Autosubmit+1 is set.


Aaron, please add/correct anything I missed.

Yep, I concur with all statements made in comment #17 :)

+1 to #17.

There is one other thing we should talk about:

Andrii, how close do you think we are to a reality of label-less CQ? How much engineering work will that take, on both the CQ and Gerrit/plugin sides? How many design docs need to be written and circulated?

I don't want to be in a situation where we're working on adding an Autosubmit label and removing the CQ label at the same time. If Andrii thinks we could get rid of the CQ label quickly, we should do that, and *then* add autosubmit via the same method. If Andrii thinks it is going to be 2+ quarters, then we should move forward with autosubmit via label now.

From my side- I'm probably a week away from getting this done (depending on review time).
The reply dialog interaction was not too hard. I need to finish and add tests for the PG specific changes.
The CQ+1 button on the main page is difficult. Chatted with viktard@ about this and he pointed me in the right direction to try tomorrow.

Andrii, we should also talk about the per-branch CQ configs again. That should be higher priority than label-less CQ.

Label-less CQ is at least 3 weeks, with biggest challenges:
 (0) agreeing on (1)..(3) below.
 (1) getting new UI w/o labels working on Gerrit side. (hard for me, a lot easier for you)
 (2) adding new API to today's CQ Go AppEngine app. Fairly easy now.
 (3) modifying Python codebase to talk to (b) instead of polling Gerrit with the right config option. Easy, except initial authentication stuff.
 (4) migrate all projects :(

given my team OKRs, at best this is April. At worst, dunno. If Autosubmit is important for you by itself, don't block on CQ w/o labels.

Now, per-branch CQ, indeed higher priority, but I haven't finished first design doc due to rotations & courses, but should have it tomorrow.

Regarding CQ w/o labels: I have lots of thoughts/opinions about this. Please include me on the design doc (whenever there is one). I do not think this should be high priority at all but that's just my opinion. About "migrate all projects", as you probably expect, this is going to be a lot of work and will take a while. For example, Skia has lots of services/tools that would need to be updated.

Regarding per-branch CQ: That sounds great, thanks! As mentioned earlier, I will help with this effort.

Regarding what this bug is about: I am proceeding as normal. Whether the code is in chromium-behavior plugin (preferable) or some other plugin, I intend to get autosubmit working. Not because I consider it high priority but because I don't want to lose the autosubmit related stuff in my head and start over again later.

Updates-

* Submitted https://gerrit-review.googlesource.com/c/gerrit/+/161872/: Add ability to hide quick approve button to GrChangeActionsInterface.

* Sent out for review https://gerrit-review.googlesource.com/c/gerrit/+/162110/: Add more helper methods to GrChangeReplyInterface.
  viktard@ suggested a better approach with https://gerrit-review.googlesource.com/c/gerrit/+/163250: Suggest label score plugin sample.
  Added a few fixes to the above change today. Hopefully we can get it in before viktard@ is OOO for the next few weeks.

* The actual plugin code is under active review in https://chromium-review.googlesource.com/c/infra/gerrit-plugins/chromium-behavior/+/931626/: Add AutoSubmit support to chromium-behavior.html.

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/gerrit-plugins/chromium-behavior/+/b8825c348727d84eb3bd047ee3971bf89dec1b67

commit b8825c348727d84eb3bd047ee3971bf89dec1b67
Author: Ravi Mistry <rmistry@google.com>
Date: Mon Mar 05 14:59:25 2018

Add AutoSubmit support to chromium-behavior.html

There are two main flows that need to handle the AS+1 vote:
1. CR+1 in reply dialog should automatically set CQ+2 label without
   submitting. An info msg should be displayed to the user in the
   dialog.
2. CR+1 button on the main page should display a popup asking if CQ+2
   should be set since AS+1 is set.
   For this the CR+1 (quick approve) button needs to be replaced with a
   button with a custom handler. As part of this I also added a fix for
   https://bugs.chromium.org/p/gerrit/issues/detail?id=8006
   ('Want CR+1 button as reviewer also if somebody already reviewed').


Bug:  631551 , gerrit:8006
Change-Id: I643a79abde5b117281a446184455556fe6cea5be
[modify] https://crrev.com/b8825c348727d84eb3bd047ee3971bf89dec1b67/src/main/resources/static/chromium-behavior.html

Update:

Both required changes in PolyGerrit (https://gerrit-review.googlesource.com/c/gerrit/+/161872/ and https://gerrit-review.googlesource.com/c/gerrit/+/163250) have landed.

The required changes to the chromium-behavior.html plugin have also landed: https://chromium-review.googlesource.com/c/infra/gerrit-plugins/chromium-behavior/+/931626


My plan now is this:

* Wait till changes to chromium-behavior.html are in production.
* Create an Auto-Submit label for https://skia.googlesource.com/buildbot/+/master. Make sure it works.
* Create an Auto-Submit label for https://skia.googlesource.com/skia/+/master for the whole Skia team to dogfood. Address feedback.
* Create Auto-Submit label for other Chromium projects (when to do this will be up to agable@).

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/gerrit-plugins/chromium-behavior/+/34fd736befbcad87288cc8f9aa1aba7366b37ab5

commit 34fd736befbcad87288cc8f9aa1aba7366b37ab5
Author: Ravi Mistry <rmistry@google.com>
Date: Thu Mar 08 15:38:17 2018

[AutoSubmit] Check if all other reviewers have approved and display msg if not

Caveat: Approvals of reviewers is only calculated during page load.

Bug:  chromium:631551 
Change-Id: Ib903c0c19361fb9cb5fb3fc02352f1c6a9c7f677

[modify] https://crrev.com/34fd736befbcad87288cc8f9aa1aba7366b37ab5/src/main/resources/static/chromium-behavior.html

Update:

* Still waiting for the plugin change to be live before I can turn it on for https://skia.googlesource.com/buildbot/+/master and test.

* Added reviewer approval checks and a msg saying "Note: Not all reviewers have approved this change!" with https://chromium-review.googlesource.com/c/infra/gerrit-plugins/chromium-behavior/+/955775.

* Found a minor bug (https://bugs.chromium.org/p/chromium/issues/detail?id=820117) where the quick approve button goes away when there are uncommitted changes in the reply dialog.

Plugin changes are finally live.

I set ACLs for Auto-Submit label and set it with 0 default Value in Skia's All-Projects:
https://skia.googlesource.com/All-Projects/+/e17ec5ad6f950c8d81dc3026dcb491067c907a4c

Then turned it on for the buildbot project:
https://skia.googlesource.com/buildbot/+/190f9664806328d1a9167a0d5eab553aa15eec23/project.config


Uploaded a Test change here: https://skia-review.googlesource.com/c/buildbot/+/114408

Aaron, can you start testing it in https://skia.googlesource.com/buildbot
I think you are a committer, but if not let me know.

AWESOME!  Will this be added to git cl upload as well?

Yes, probably right after the larger Skia team starts dogfooding it.

The following revision refers to this bug:
  https://skia.googlesource.com/buildbot/+/d6c091fdfda407f66542e5579ae5ef011035a567

commit d6c091fdfda407f66542e5579ae5ef011035a567
Author: Aaron Gable <agable@chromium.org>
Date: Tue Mar 20 19:56:40 2018

Remove redundant whitespace file.

Bug:  chromium:631551 
Change-Id: I36158063c369e20954be7bf1296f4e3cb07c9c26
Reviewed-on: https://skia-review.googlesource.com/115440
Auto-Submit: Aaron Gable <agable@chromium.org>
Reviewed-by: Ravi Mistry <rmistry@google.com>
Commit-Queue: Ravi Mistry <rmistry@google.com>

[delete] https://crrev.com/db5958d187674dc44f5c609abc2075add0891a52/whitespace2.txt

rmistry asked me to put my FR here for review by agable and others.

When auto-submit is enabled, if there are unresolved comments (i.e. comments where the "Resolved" box isn't checked), when I click Code-Review +1, rather than auto-selecting Commit-Queue +2 (with the standard message about auto-submit), leave the Commit-Queue as 0 and instead display a message saying something like, "Auto-Submit suppressed due to unresolved comments. Choose CQ+2 (or mark comments as resolved) to trigger submit."

My reasoning:
- If I add comments to the CL, I very likely want the author to consider making edits. I probably don't want to trigger CQ+2 in that case.
  - If I only added an "FYI" comment and didn't intend for the author to make changes, I should have marked the comment as Resolved. I can also override and choose CQ+2 if I want.
  - I still want to mark the CL as Code-Review +1 to indicate that the CL is fine as-is if the author decides not to make changes based on my comments.
- Maybe it's just because I'm used to the workflow without auto-submit, but I very nearly hit "Send" on a CL where I made a comment before noticing that it was marked CQ+2 due to auto-submit.

I can file this separately if you like.

Yep, that's a really good idea, and it makes sense to me. Let's track it in a separate bug.

Sent out for review https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/986286 : Support setting Auto-Submit during "git cl upload"

Also working on fixes for https://bugs.chromium.org/p/chromium/issues/detail?id=820117 ('Auto-Submit behavior loses quick approve button with uncommitted changes to reply dialog	') and https://bugs.chromium.org/p/chromium/issues/detail?id=824459 ('FR: suppress Auto-Submit when unresolved comments').

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/31e7d56c28f1e98b7b4954469233e231e9505467

commit 31e7d56c28f1e98b7b4954469233e231e9505467
Author: Ravi Mistry <rmistry@google.com>
Date: Mon Apr 02 18:55:14 2018

Support setting Auto-Submit during "git cl upload"

Bug:  chromium:631551 
Change-Id: I1f0c397d13d601a8ecac8328b6d7bf3c117c8297
Reviewed-on: https://chromium-review.googlesource.com/986286
Commit-Queue: Ravi Mistry <rmistry@chromium.org>
Reviewed-by: Aaron Gable <agable@chromium.org>

[modify] https://crrev.com/31e7d56c28f1e98b7b4954469233e231e9505467/git_cl.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/996e6be69cced6f76b88cdb85273ad464c7e1d01

commit 996e6be69cced6f76b88cdb85273ad464c7e1d01
Author: depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Mon Apr 02 22:33:42 2018

Roll src/third_party/depot_tools/ 0d466d22d..31e7d56c2 (2 commits)

https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/0d466d22d8e2..31e7d56c28f1

$ git log 0d466d22d..31e7d56c2 --date=short --no-merges --format='%ad %ae %s'
2018-04-02 rmistry Support setting Auto-Submit during "git cl upload"
2018-04-02 ehmaldonado gclient setdep: Add support to create new variables.

Created with:
  roll-dep src/third_party/depot_tools
BUG= chromium:631551 , chromium:760633 


The AutoRoll server is located here: https://depot-tools-chromium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


TBR=agable@chromium.org

Change-Id: Ia87a7c017f69d30bc5cd22d7f3df08e3e5bc8a8a
Reviewed-on: https://chromium-review.googlesource.com/990715
Commit-Queue: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Reviewed-by: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#547540}
[modify] https://crrev.com/996e6be69cced6f76b88cdb85273ad464c7e1d01/DEPS

Update:

* Added support to Auto-Submit during "git cl upload" in https://chromium-review.googlesource.com/986286

* Fixed https://bugs.chromium.org/p/chromium/issues/detail?id=820117 : Auto-Submit behavior loses quick approve button with uncommitted changes to reply dialog.

* Fixed https://bugs.chromium.org/p/chromium/issues/detail?id=824459 : FR: suppress Auto-Submit when unresolved comments.


I plan to turn on Auto-Submit for Skia this week.

Update:

Skia is now dogfooding Auto-Submit in PolyGerrit.

Announcement email is here (for those who have access): https://groups.google.com/a/google.com/d/msg/skia-team/q6R0PeeIqms/PMSj9kyOAAAJ

Usage so far: https://skia-review.googlesource.com/q/label:Auto-Submit

One week later: No complaints yet.

Up to agable@ if we want to turn this on for any more repos right now.

tandrii@ asked if we can turn this on for chromium/infra repo.
We should do that, it has been used about 1000+ times in the Skia repo so far: https://skia-review.googlesource.com/q/label:Auto-Submit+status:merged


This is what should be added to project.config:

[label "Auto-Submit"]                                                           
        function = NoBlock                                                      
        defaultValue = 0                                                        
        copyMinScore = true                                                     
        copyMaxScore = true                                                     
        copyAllScoresOnTrivialRebase = true                                     
        copyAllScoresIfNoCodeChange = true                                      
        value = 0 Do not send CL to CQ automatically after approval             
        value = +1 Send CL to CQ automatically after approval


[access "refs/heads/*"]
....
  label-Auto-Submit = +0..+1 group Change Owner


Add this line to groups (if it does not already exist):
global:Change-Owner       Change Owner

Awesome. Filed  issue 882506  to track enabling this in public infra (Chrome Operations) repos.

As someone that commits often to both chromium infra repos and the chromium/src repo, I find myself wishing the auto-submit feature was present in both. It appears the rollout is easy enough (one change to refs/meta/config/project.config). Given that the feature has IMO been sufficiently vetted by skia and chromium-infra repos, I think it's time we roll it out to chromium/src.

rmistry/tandrii/ajp: Any objections? If not, I can drive sending coms and turning it on for chromium/src.

I've no objections. I think it's time, would be useful, and just haven't prioritized driving it myself. I would love the help. 

A few days ago I added to my OKRs to "Launch Auto-Submit on all chromium repos" because it has been successfully used many times and is definitely ready for wider use:
* 1000+ usage in Skia
* 500+ usage in chromium-review
* 200+ usage in chrome-internal-review

Let me know how I can help.

I had started on a draft PSA, which was a combination of the PSA I sent to Skia and tandrii@'s PSA to ChOps. It includes the bonus feature section (which was not working during the PSA to ChOps).

I put the draft here: https://docs.google.com/document/d/1LGBYxyZdqeBMPzWUVJSffPyXmjeGmK-g6Yxk_YGGOzE/edit (Google access only)
I added chrome-infra@ as editors. Feel free to edit however you see fit. I will not be offended if it is rewritten from scratch.

Thanks for putting that together!

One small suggestion: why don't we send the PSA first before landing it? And reword it to say something like "In 1-2 days we will enable the Auto-Submit feature ...." & link the pending CL that enables it. That way we give everyone a fair bit of heads-up that the tool will change, and any questions/concerns that come up have a chance to be resolved before flipping the switch. (Not that I expect any, but better safe than sorry.)

Makes sense to me, done. Feel free to edit if you need to make any other changes.

LGTM! I'll let you send it out since you did all the heavy lifting :)

Plan is to announce this to chromium-dev tomorrow (1/15): https://docs.google.com/document/d/1LGBYxyZdqeBMPzWUVJSffPyXmjeGmK-g6Yxk_YGGOzE/edit

If there are no objections, on Thursday (1/17) I will need somebody to make a change similar to https://bugs.chromium.org/p/chromium/issues/detail?id=631551#c46 to chromium/src to turn on Auto-Submit.

Perhaps file a git-admin ticket now (or as soon as you're confident) and work with the git admin to make the necessary changes on 1/17. I believe that's flyboy@ for this week.

Unless tandrii@ or someone else wants to volunteer to do this.

The NextAction date has arrived: 2019-01-15

Announced to chromium-dev: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/zMunKnH7Ick/Uzdvel40EwAJ


I have the project.config CL ready here: https://chromium-review.googlesource.com/c/chromium/src/+/1412272
Sent it to flyboy@ and tandrii@ for review.

The NextAction date has arrived: 2019-01-16

Plan is to submit https://chromium-review.googlesource.com/c/chromium/src/+/1412272 early (8amEST ?) tomorrow and then test.

The NextAction date has arrived: 2019-01-17

Submitted https://chromium-review.googlesource.com/c/chromium/src/+/1412272 at 8:04am EST.

Tested by uploading https://chromium-review.googlesource.com/c/chromium/src/+/1417091 with Auto-Submit+1 and asked robertphillips@ to CR+1 for the different flows. Appears to be working.

Will watch uploaded chromium changes for a bit to make sure nothing has regressed.

Also, went ahead and added rmistry@ to the auto CC for the "Infra>Codereview>Gerrit" component.

Usage can be tracked here: https://chromium-review.googlesource.com/q/label:Auto-Submit+repo:chromium/src

\o/

Thanks for driving that forward into chromium. I look forward to using it :)

Has been used 150+ times so far on chromium/src: https://chromium-review.googlesource.com/q/label:Auto-Submit+repo:chromium/src+status:merged

Marking this as fixed!

Thanks for implementing this!

I've noticed that this UI is a bit confusing when there are multiple reviewers for a CL, especially when everyone's review is necessary for OWNERS coverage. On such CLs, CQ+2 seems to be set for *each* reviewer, rather than for the final reviewer. Example CL: https://chromium-review.googlesource.com/c/chromium/src/+/1425562

In contrast, Critique's auto-submit feature checks that all necessary approvals are granted before trying to trigger the commit. Should I file a follow-up bug for similar behavior in Chromium?

tl;dr yes, file a feature request.

Critique has first hand knowledge of OWNERS files, so it knows who's approval is required.
OTH, Gerrit doesn't know about OWNERS (but there are plugins), so from Gerrit PoV, a single CR+1 vote is sufficient for submission. CQ also doesn't know OWNERS, OWNERS are checked in regular tryjob called "chromium_presubmit". End result should be gerrit understands OWNERS + presubmit no longer checks owners.

I was typing a bunch of text but it was basically what tandrii@ said so..