Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2698813008: [OWNERS tags] Check against multiple teams per component on presubmit (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months ago by RobertoCN
Modified:
5 months, 2 weeks ago
Reviewers:
ymzhang1, stgao
CC:
chromium-reviews, sshruthi1
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[OWNERS tags] Check against multiple teams per component on presubmit This new check uses the latest automatically generated component map https://storage.googleapis.com/chromium-owners/component_map.json to verify that the change does not introduce multiple teams for the same component. TBR=ymzhang@chromium.org,stgao@chromium.org BUG= Review-Url: https://codereview.chromium.org/2698813008 Cr-Commit-Position: refs/heads/master@{#453784} Committed: https://chromium.googlesource.com/chromium/src/+/e49357eef7a06b04ce632446fc5b3a33757ce9a1

Patch Set 1 #

Total comments: 1

Messages

Total messages: 13 (4 generated)
RobertoCN
Yang, could you please review this change?
6 months ago (2017-02-17 03:00:18 UTC) #1
ymzhang1
On 2017/02/17 03:00:18, RobertoCN wrote: > Yang, could you please review this change? Sure.
6 months ago (2017-02-17 17:26:39 UTC) #2
ymzhang1
The code looks good to me. Add one comment. https://codereview.chromium.org/2698813008/diff/1/tools/checkteamtags/checkteamtags.py File tools/checkteamtags/checkteamtags.py (right): https://codereview.chromium.org/2698813008/diff/1/tools/checkteamtags/checkteamtags.py#newcode55 tools/checkteamtags/checkteamtags.py:55: ...
6 months ago (2017-02-17 21:00:02 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2698813008/1
5 months, 2 weeks ago (2017-03-01 00:57:40 UTC) #6
commit-bot: I haz the power
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/e49357eef7a06b04ce632446fc5b3a33757ce9a1
5 months, 2 weeks ago (2017-03-01 01:09:52 UTC) #9
stgao
I have two concerns on making this as presubmit. 1. It adds a dependency on ...
5 months, 2 weeks ago (2017-03-02 01:25:01 UTC) #10
ymzhang1
On 2017/03/02 01:25:01, stgao (catching up) wrote: > I have two concerns on making this ...
5 months, 2 weeks ago (2017-03-02 17:39:56 UTC) #11
RobertoCN
On 2017/03/02 01:25:01, stgao (catching up) wrote: > I have two concerns on making this ...
5 months, 2 weeks ago (2017-03-02 19:24:02 UTC) #12
stgao
5 months, 2 weeks ago (2017-03-02 19:53:09 UTC) #13
Message was sent while issue was closed.
On 2017/03/02 19:24:02, RobertoCN wrote:
> On 2017/03/02 01:25:01, stgao (catching up) wrote:
> > I have two concerns on making this as presubmit.
> > 
> > 1. It adds a dependency on an external source other than just the Chromium
> repo.
> Yes, I had this concern as well. There's two reasons that made me opt for this
> approach.
>   A) Presubmit script only passes the list of affected files to our script,
and
> I felt that reading the rest of the repo will break that convention.
>   B) Relying on the local checkout would force the assumption that files other
> than those affected are reasonably up-to-date.
>   C) Traversing the repo and parsing all OWNERS files takes longer than
> retrieving the json file from the cloud in most cases.

I'd suggest chatting with infra and other folks whether such a dependency is OK.
I still feel that it is a bit unfortunate to have that.
But at least, we should retry downloading if there is network error.
We might want to skip checking if downloading completely fails.

> 
> > 2. Do we check that it is caused exactly by the OWNERS file changed by the
CL
> > itself? If there are already multiple teams for a component, and I make a CL
> to
> > change some unrelated OWNERS file, will I get alerted?
> The mapping retrieved from the cloud location is guaranteed not to have
multiple
> teams. If someone tried to introduce multiple teams to the mapping, the
> presubmit would fail. And if they forcibly landed it, we would still know
> because the cron job would fail and last known good mapping would remain at
the
> cloud location.

SG.

> > 
> > BTW,
> >
>
https://luci-milo.appspot.com/buildbot/tryserver.chromium.linux/linux_chromiu...
> > seems to catch one case. Can someone follow up?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b