New issue
Advanced search Search tips

Issue 755753 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

In import CLs, TBR the current "ecosystem infra" sheriff

Project Member Reported by qyears...@chromium.org, Aug 15 2017

Issue description

Currently in test_importer.py, it adds TBR=qyearsley (my username is hardcoded).

In general if there is an ecosystem infra rotation, the current sheriff would be a good person to TBR.

First action here is to find out if there's an endpoint we can use to fetch the current sheriff username.
 
Owner: qyears...@chromium.org
Status: Assigned (was: Available)
Looks like one way to get the current ecosystem infra sheriff is to fetch http://chromium-build.appspot.com/p/chromium/all_rotations.js (this is how SoM appears to get current sheriff:
https://cs.chromium.org/chromium/infra/go/src/infra/appengine/sheriff-o-matic/frontend/elements/som-drawer/som-drawer.js?dr=C&l=98
Status: Started (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 23 2017

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

commit a30383079c1f1522d44c6eff31a75cd9a1fefc55
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Mon Oct 23 23:15:13 2017

wpt-import: TBR current ecosystem infra sheriff

Bug:  755753 
Change-Id: I12829ec0a83608219225a799d54609e3b1007d6c
Reviewed-on: https://chromium-review.googlesource.com/729639
Reviewed-by: Robert Ma <robertma@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510955}
[modify] https://crrev.com/a30383079c1f1522d44c6eff31a75cd9a1fefc55/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
[modify] https://crrev.com/a30383079c1f1522d44c6eff31a75cd9a1fefc55/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py

Status: Fixed (was: Started)
Should be resolved after that CL.
Status: Started (was: Fixed)
Reopening; the CL above broke WPT imports when a non-chromium.org email address (such as mine) is on rotation, see e.g. https://luci-milo.appspot.com/buildbot/chromium.infra.cron/wpt-importer/3918

Since it doesn't look like any other Tools OWNERS can review https://chromium-review.googlesource.com/c/chromium/src/+/739393 in a timely manner, I'm reverting the original CL and we can land everything once Quinten's back next Monday.
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 27 2017

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

commit cb514b4d4a30205c8cbb9c742d14a6b5820407e9
Author: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Date: Fri Oct 27 08:39:01 2017

Revert "wpt-import: TBR current ecosystem infra sheriff"

This reverts commit a30383079c1f1522d44c6eff31a75cd9a1fefc55.

Reason for revert: broke WPT imports when a non-chromium.org email is on rotation, nobody could review https://chromium-review.googlesource.com/c/chromium/src/+/739393 in a timely manner.

Original change's description:
> wpt-import: TBR current ecosystem infra sheriff
> 
> Bug:  755753 
> Change-Id: I12829ec0a83608219225a799d54609e3b1007d6c
> Reviewed-on: https://chromium-review.googlesource.com/729639
> Reviewed-by: Robert Ma <robertma@chromium.org>
> Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#510955}

TBR=qyearsley@chromium.org,foolip@chromium.org,robertma@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  755753 
Change-Id: I8edd837fb8636583f8e3095444a1e7403e2dab71
Reviewed-on: https://chromium-review.googlesource.com/741501
Reviewed-by: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/heads/master@{#512128}
[modify] https://crrev.com/cb514b4d4a30205c8cbb9c742d14a6b5820407e9/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
[modify] https://crrev.com/cb514b4d4a30205c8cbb9c742d14a6b5820407e9/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 27 2017

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

commit 78e97a5517a3064d8738b5bbd06e5df0c9f0dc27
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Fri Oct 27 12:10:28 2017

Reland "wpt-import: TBR current ecosystem infra sheriff"

This is a reland of https://chromium-review.googlesource.com/729639 combined
with the fix for non-chromium.org email addresses uploaded in
https://chromium-review.googlesource.com/739393. They are being landed
together to avoid having a small window where having the former and not the
latter can break WPT imports.

First CL's original commit message:

  wpt-import: TBR current ecosystem infra sheriff

Second CL's original commit message:

  wpt-import: Do not always append chromium.org to TBR email addresses.

  The rotation JSON file contains full email addresses for users with a
  non-chromium.org email (such as myself).

  The code was previously failing in those cases, as it generated a TBR line
  such as "raphael.kubo.da.costa@intel.com@chromium.org", which causes
  depot_tools' owners.py to bail out.

Bug:  755753 
Change-Id: I5306f137dbc740452ec9e28497da076a146a78fe
Reviewed-on: https://chromium-review.googlesource.com/741593
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/heads/master@{#512153}
[modify] https://crrev.com/78e97a5517a3064d8738b5bbd06e5df0c9f0dc27/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
[modify] https://crrev.com/78e97a5517a3064d8738b5bbd06e5df0c9f0dc27/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py

Status: Fixed (was: Started)
Fixed again; I've landed both CLs together to avoid issues. Thanks to Philip and Robert for reviewing!
Thank you Raphael! Such a silly mistake :-(

At least the rotation calendar JSON contains full email addresses for the non-chromium.org case. I wonder whether the CC field entries actually have to be full email addresses, or whether it's unnecessary to ever append "@chromium.org" to anything...
Apparently appending chromium.org is indeed not necessary. I created https://chromium-review.googlesource.com/c/chromium/src/+/743521 via `git-cl upload --tbrs qyearsley --cc robertma' and it seems to have worked fine.

I can send a CL simplifying the code if you prefer.
Status: Available (was: Fixed)
> I can send a CL simplifying the code if you prefer.

Yes let's get rid of that string concatenation then! The simpler, the merrier :)
Project Member

Comment 13 by bugdroid1@chromium.org, Oct 30 2017

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

commit a565e32a80dac82ab97112d0fa8b165a4f03980f
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Mon Oct 30 16:51:21 2017

WPT import: Stop appending @chromium.org to TBR emails altogether.

The JSON rotation file we use contains user names for people with
@chromium.org and full email addresses for all other cases.

It turns out passing '--tbrs foo' to git-cl results in foo@chromium.org
being added to the Gerrit CL, so we can simplify our code a little bit.

Bug:  755753 
Change-Id: If86524ec1937525b8cb8b3d4a937c06f4ce21295
Reviewed-on: https://chromium-review.googlesource.com/744011
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512520}
[modify] https://crrev.com/a565e32a80dac82ab97112d0fa8b165a4f03980f/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
[modify] https://crrev.com/a565e32a80dac82ab97112d0fa8b165a4f03980f/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py

Status: Fixed (was: Available)
Project Member

Comment 15 by bugdroid1@chromium.org, Nov 13 2017

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

commit 81645448b527b4953066a551a1f61ddbae7ef425
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Mon Nov 13 17:23:19 2017

wpt import: Never return an empty string in tbr_reviewer().

After commit a565e32 ("WPT import: Stop appending @chromium.org to TBR
emails altogether"), whenever _fetch_ecosystem_infra_sheriff_username()
returned an empty string we'd pass it on instead of falling back to
'qyearsley' like we did before.

Fix it by making sure we always return the fallback string both if there's
an exception as well as if there is nobody currently on rotation.

Bug:  755753 ,  784262 
Change-Id: I31b298bd3ffe53fc10fec42f8096f04d9fdb08dd
Reviewed-on: https://chromium-review.googlesource.com/765492
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Robert Ma <robertma@chromium.org>
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/heads/master@{#515971}
[modify] https://crrev.com/81645448b527b4953066a551a1f61ddbae7ef425/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
[modify] https://crrev.com/81645448b527b4953066a551a1f61ddbae7ef425/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py

Sign in to add a comment