[OWNERS tags] Check against multiple teams per component on presubmit

[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

[RobertoCN, 2017-02-17 03:00:18]

Yang, could you please review this change?

[ymzhang1, 2017-02-17 17:26:39]

On 2017/02/17 03:00:18, RobertoCN wrote:
> Yang, could you please review this change?

Sure.

[ymzhang1, 2017-02-17 21:00:02]

The code looks good to me. Add one comment.

https://codereview.chromium.org/2698813008/diff/1/tools/checkteamtags/checkte...
File tools/checkteamtags/checkteamtags.py (right):

https://codereview.chromium.org/2698813008/diff/1/tools/checkteamtags/checkte...
tools/checkteamtags/checkteamtags.py:55: current_mappings[dir_name] = (team,
component)
Maybe we could save dir->(team, component) in component_map.json instead of
doing the conversion every time we validate the mapping. What do you think?
Current version also looks fine for me.

[RobertoCN, 2017-03-01 00:57:10]

Description was changed from

==========
[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.

R=ymzhang@chromium.org,stgao@chromium.org
BUG=
==========

to

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

[RobertoCN, 2017-03-01 00:57:13]

The CQ bit was checked by robertocn@chromium.org

[commit-bot: I haz the power, 2017-03-01 00:57:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 01:08:28]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1488329833625150, "parent_rev":
"2100af86f06ded92e913c67723bbff9af08df307", "commit_rev":
"e49357eef7a06b04ce632446fc5b3a33757ce9a1"}

[commit-bot: I haz the power, 2017-03-01 01:09:51]

Description was changed from

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

to

==========
[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/+/e49357eef7a06b04ce632446fc5b...
==========

[commit-bot: I haz the power, 2017-03-01 01:09:52]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/e49357eef7a06b04ce632446fc5b...

[stgao, 2017-03-02 01:25:01]

I have two concerns on making this as presubmit.

1. It adds a dependency on an external source other than just the Chromium repo.
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?

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

[ymzhang1, 2017-03-02 17:39:56]

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.
> 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?
> 
> BTW,
>
https://luci-milo.appspot.com/buildbot/tryserver.chromium.linux/linux_chromiu...
> seems to catch one case. Can someone follow up?

cl to correct team mailing list for Blink>XML.
https://codereview.chromium.org/2726213002/

[RobertoCN, 2017-03-02 19:24:02]

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.

> 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.
> 
> BTW,
>
https://luci-milo.appspot.com/buildbot/tryserver.chromium.linux/linux_chromiu...
> seems to catch one case. Can someone follow up?

[stgao, 2017-03-02 19:53:09]

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?