New issue
Advanced search Search tips

Issue 825407 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

CQ: do not merge CLs still marked as Private

Project Member Reported by vapier@chromium.org, Mar 23 2018

Issue description

once we start to merge a CL, there's no reason for it to be marked Private, or to let it stay that way once it's been merged.  this makes it hard for people who aren't directly cc-ed to review the comments.

we should update the CQ bots to filter out is:private CLs, and possibly have it post a comment like "Refusing to pick up Private CLs" while removing the Commit-Queue+1 bit.

this bug is about the Chrome OS CQ, but i'm wondering whether the Chromium team has a policy on this already.  seems like we should be aligned.
 

Comment 1 by aga...@chromium.org, Mar 26 2018

Chromium's CQ specifically supports running trybots on Private changes: this is necessary for someone to do dry-runs and get back results while still keeping their change on the down-low.

Similarly, doing a full CQ run may fail: the change could go through a whole two-hour-plus run, fail, not get committed, and then sit around until the owner gets back and sees that it failed. This is not a good thing for security-critical changes.

There is no such thing as a Private Merged change, so we have no problem leaving the Private bit set until the moment the change is submitted. Happy to discuss further if we want to coordinate policies on this.

Comment 2 by vapier@chromium.org, Mar 26 2018

my only concern is about Merged|Private.  if we can disallow that, i'm happy to leave Private usage available in all other cases.

Comment 3 by aga...@chromium.org, Mar 26 2018

...I sincerely believed that [Merged, Private] was an impossible state. I'm clearly wrong: https://chromium-review.googlesource.com/c/v8/v8/+/981232

That's certainly exciting. Yes, I believe it makes sense for all changes to be made non-private at the time they're submitted.

Comment 4 by vapier@chromium.org, Mar 27 2018

i see about 40 or so CLs on our internal repo:
https://chrome-internal-review.googlesource.com/q/status:merged+is:private

and even more on our public GoB (100s that i have access to):
https://chromium-review.googlesource.com/q/status:merged+is:private

i'm fairly certain none of those were intentional :/

Comment 5 by vapier@chromium.org, Jun 28 2018

Summary: CQ: do not merge CLs still marked as Private (was: CQ: do not pick up CLs still marked as Private)
On the ChromeOS side, we can allow private for tryjobs (and even the PreCQ) while preventing the CQ from picking up Private CLs.

However, if a CL is marked Private while the CQ is running, it will still be possible for it to submitted.
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/f9d85d8779cf4352bb19557064dc3b3ae423fc21

commit f9d85d8779cf4352bb19557064dc3b3ae423fc21
Author: Mike Frysinger <vapier@chromium.org>
Date: Tue Jul 10 05:35:42 2018

cbuildbot: reject private CLs in the CQ

We don't want to merge CLs marked private in the CQ (pre-cq is fine),
so reject ones that are still marked as private.

BUG= chromium:825407 
TEST=precq passes

Change-Id: If09243920c08f2ff505801d43f84353ff2259184
Reviewed-on: https://chromium-review.googlesource.com/1117908
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/f9d85d8779cf4352bb19557064dc3b3ae423fc21/lib/patch_unittest.py
[modify] https://crrev.com/f9d85d8779cf4352bb19557064dc3b3ae423fc21/lib/patch.py

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 10

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

commit 25fbb27791f08bdcfb0fa50727d088ca5b5f517b
Author: Chromite Chromium Autoroll <chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Tue Jul 10 06:53:33 2018

Roll src/third_party/chromite 1956af3165b5..f9d85d8779cf (1 commits)

https://chromium.googlesource.com/chromiumos/chromite.git/+log/1956af3165b5..f9d85d8779cf


git log 1956af3165b5..f9d85d8779cf --date=short --no-merges --format='%ad %ae %s'
2018-07-10 vapier@chromium.org cbuildbot: reject private CLs in the CQ


Created with:
  gclient setdep -r src/third_party/chromite@f9d85d8779cf

The AutoRoll server is located here: https://chromite-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.



BUG= chromium:825407 
TBR=chrome-os-gardeners@chromium.org

Change-Id: I39f4073739c6e896156adbf86de9ead6d2b098f4
Reviewed-on: https://chromium-review.googlesource.com/1130682
Reviewed-by: Chromite Chromium Autoroll <chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: Chromite Chromium Autoroll <chromite-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#573636}
[modify] https://crrev.com/25fbb27791f08bdcfb0fa50727d088ca5b5f517b/DEPS

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/a3bc29bcacf392cdba0021fee8bab5bd45665975

commit a3bc29bcacf392cdba0021fee8bab5bd45665975
Author: Mike Frysinger <vapier@chromium.org>
Date: Wed Jul 11 19:13:32 2018

validation_pool: process private/draft CLs before filtering

We're now correctly filtering out private CLs, but we still aren't
notifying the user why they're being elided.  It's because the
validation pool will filter out non-ready CLs first, then only
process the ones left.  Turns out we've also been silently skipping
draft CLs too.

Update the validation pool to notify draft/private CLs before we
filter out the non-ready CLs.

BUG= chromium:825407 
TEST=precq passes

Change-Id: Ia32f634ae033fef61128d700d056fe4537cd88a1
Reviewed-on: https://chromium-review.googlesource.com/1132709
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>

[modify] https://crrev.com/a3bc29bcacf392cdba0021fee8bab5bd45665975/cbuildbot/validation_pool.py
[modify] https://crrev.com/a3bc29bcacf392cdba0021fee8bab5bd45665975/lib/patch.py

Owner: vapier@chromium.org
Status: Fixed (was: Available)
the CrOS CQ now detects & rejects CLs that are marked Private and CQ+1.  example:

> https://chromium-review.googlesource.com/1130703
> Patch Set 2:
> The Commit Queue could not apply your change because the CL is private. Please make your CL public before marking your commit as ready.

so i'm going to call this fixed from the CrOS side.  if Chromium wants to update their CQ flow, we can fork a new bug.

Sign in to add a comment