Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1437683006:
Give tools/ a real OWNERS file. (Closed)
|
Created:
5 years, 1 month ago by Nico Modified:
4 years, 8 months ago CC:
chromium-reviews, glider+watch_chromium.org, telemetry-reviews_chromium.org, asvitkine+watch_chromium.org, bruening+watch_chromium.org, alokp, borenet2, dtu, Evan Stade, iannucci, kjellander_chromium, M-A Ruel, mnaganov (inactive), Paweł Hajdan Jr., rmcilroy, satorux1, shatch, Torne, Vadim Sh., Yaron Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGive tools/ a real OWNERS file.
See "[chromium-dev] Adding a real OWNERS file for tools/"
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qYrZ2_Y-2dI/S0TsAnTDEQAJ
This allows removing several `set noparent`s and makes it unnecessary
to add more of those in other places (e.g. tools/grit, tools/clang, ...).
The idea is that tools is still a good place to put your one-off scripts
that you want to share with others and this shouldn't add any friction
for adding More Stuff, so it's ok to TBR for new tools/ subfolders.
But existing tools/ should be reviewed by the folks who wrote them
(or you can say OWNERS=* in your subfolder).
I added per-file OWNERS for most files that live directly in tools/,
and an OWNERS file for grit now that it lives in src.
Some tools/ subfolders currently lack OWNERS files, please add those
as required.
BUG=none
R=dpranke@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/f911237bfc4f8b4e1cf3c772c8e1fa9bd1c3c417
Patch Set 1 #Patch Set 2 : Give tools/ a real OWNERS file. #Patch Set 3 : Give tools/ a real OWNERS file. #Patch Set 4 : Give tools/ a real OWNERS file. #
Total comments: 5
Messages
Total messages: 33 (8 generated)
Description was changed from ========== Give tools/ a real OWNERS file. BUG=none ========== to ========== Give tools/ a real OWNERS file. See "[chromium-dev] Adding a real OWNERS file for tools/" https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qYrZ2_Y-2dI/S0TsA... This allows removing several `set noparent`s and makes it unnecessary to add more of those in other places (e.g. tools/grit, tools/clang, ...). The idea is that tools is still a good place to put your one-off scripts that you want to share with others and this shouldn't add any friction for adding More Stuff, so it's ok to TBR for new tools/ subfolders. But existing tools/ should be reviewed by the folks who wrote them (or you can say OWNERS=* in your subfolder). I added per-file OWNERS for most files that live directly in tools/, and an OWNERS file for grit now that it lives in src. Some tools/ subfolders currently lack OWNERS files, please add those as required. BUG=none ==========
thakis@chromium.org changed reviewers: + brettw@chromium.org
yay / nay?
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
lgtm
cc'd people: fyi, I'm adding you as a per-file owner to tools/OWNERS for a scripts where I would've picked you as reviewer for changes to that script, from looking through each script's commit history. Please take a peek, and add more owners as appropriate in a follow-up.
The CQ bit was checked by thakis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1437683006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1437683006/60001
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f911237bfc4f8b4e1cf3c772c8e1fa9bd1c3c417 Cr-Commit-Position: refs/heads/master@{#359384}
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as f911237bfc4f8b4e1cf3c772c8e1fa9bd1c3c417 (presubmit successful).
Message was sent while issue was closed.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1437683006/diff/60001/tools/metrics/OWNERS File tools/metrics/OWNERS (left): https://codereview.chromium.org/1437683006/diff/60001/tools/metrics/OWNERS#ol... tools/metrics/OWNERS:1: set noparent I think removing this is dangerous on tools/metrics. Now, if you have an LGTM from a src/OWNERS (e.g. because you needed a stamp for chrome/ or something), you might end up submitting without a metrics reviewer looking at your histograms.xml changes. You can do this accidentally (i.e. because the tools told you had complete OWNERS coverage). I don't think that's a good practice. Can we still have setnoparent at least for the XML files? Otherwise, I feel it's too easy to accidentally land those changes without a proper metrics review.
Message was sent while issue was closed.
asvitkine@chromium.org changed reviewers: + isherman@chromium.org
Message was sent while issue was closed.
+isherman for his thoughts too
Message was sent while issue was closed.
https://codereview.chromium.org/1437683006/diff/60001/tools/metrics/OWNERS File tools/metrics/OWNERS (left): https://codereview.chromium.org/1437683006/diff/60001/tools/metrics/OWNERS#ol... tools/metrics/OWNERS:1: set noparent On 2015/11/12 21:10:26, Alexei Svitkine (slow) wrote: > I think removing this is dangerous on tools/metrics. > > Now, if you have an LGTM from a src/OWNERS (e.g. because you needed a stamp for > chrome/ or something), you might end up submitting without a metrics reviewer > looking at your histograms.xml changes. You can do this accidentally (i.e. > because the tools told you had complete OWNERS coverage). This is true for all OWNERS files though, right? You have to trust that people in OWNERS files above you don't abuse their powers. > I don't think that's a good practice. Can we still have setnoparent at least for > the XML files? Otherwise, I feel it's too easy to accidentally land those > changes without a proper metrics review. Having `noparent` is bad practice too, in general as it makes global refactorings harder and whatnot. But if per-file noparent is possible, having it for the xml files in this directory would be fine with me (they do seem delicate, and per-file noparent sets a slightly worse precedent than a full noparent). But maybe we could wait a week or two and see if this a real problem first?
Message was sent while issue was closed.
Just to be clear, I don't mean that src/OWNERS would abuse their powers. My concerns is that someone getting one of those people to review their CL (for a different reason - e.g. maybe they want jam@ to review content/ changes) would not realise they're missing a reviewer for histograms.xml and no one would notice and the CL would be landed. People already forget they need an owner review for these files and often find out from the cq presubmit check that they forgot to add an owner for histograms.xml. This happens on a regular basis - I have a watchlist so I can find you an example from my inbox if you'd like. This change would make it easier to make this mistake and not have it caught. (I guess it depends how often src/OWNERS review CLs - but I suspect this still happens fairly often). On Thu, Nov 12, 2015 at 4:19 PM, <thakis@chromium.org> wrote: > > https://codereview.chromium.org/1437683006/diff/60001/tools/metrics/OWNERS > File tools/metrics/OWNERS (left): > > > https://codereview.chromium.org/1437683006/diff/60001/tools/metrics/OWNERS#ol... > tools/metrics/OWNERS:1: set noparent > On 2015/11/12 21:10:26, Alexei Svitkine (slow) wrote: > >> I think removing this is dangerous on tools/metrics. >> > > Now, if you have an LGTM from a src/OWNERS (e.g. because you needed a >> > stamp for > >> chrome/ or something), you might end up submitting without a metrics >> > reviewer > >> looking at your histograms.xml changes. You can do this accidentally >> > (i.e. > >> because the tools told you had complete OWNERS coverage). >> > > This is true for all OWNERS files though, right? You have to trust that > people in OWNERS files above you don't abuse their powers. > > I don't think that's a good practice. Can we still have setnoparent at >> > least for > >> the XML files? Otherwise, I feel it's too easy to accidentally land >> > those > >> changes without a proper metrics review. >> > > Having `noparent` is bad practice too, in general as it makes global > refactorings harder and whatnot. But if per-file noparent is possible, > having it for the xml files in this directory would be fine with me > (they do seem delicate, and per-file noparent sets a slightly worse > precedent than a full noparent). But maybe we could wait a week or two > and see if this a real problem first? > > https://codereview.chromium.org/1437683006/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On Thu, Nov 12, 2015 at 1:26 PM, Alexei Svitkine <asvitkine@chromium.org> wrote: > Just to be clear, I don't mean that src/OWNERS would abuse their powers. > My concerns is that someone getting one of those people to review their CL > (for a different reason - e.g. maybe they want jam@ to review content/ > changes) would not realise they're missing a reviewer for histograms.xml > and no one would notice and the CL would be landed. > The list of toplevel OWNERS actively reviewing stuff is pretty small though (3?), maybe we could just tell them to be mindful about this going forward. Not sure. > People already forget they need an owner review for these files and often > find out from the cq presubmit check that they forgot to add an owner for > histograms.xml. This happens on a regular basis - I have a watchlist so I > can find you an example from my inbox if you'd like. This change would make > it easier to make this mistake and not have it caught. (I guess it depends > how often src/OWNERS review CLs - but I suspect this still happens fairly > often). > If you have an example where a toplevel owner accidentally lg'd a histograms change, then that'd convince me :-) > On Thu, Nov 12, 2015 at 4:19 PM, <thakis@chromium.org> wrote: > >> >> https://codereview.chromium.org/1437683006/diff/60001/tools/metrics/OWNERS >> File tools/metrics/OWNERS (left): >> >> >> https://codereview.chromium.org/1437683006/diff/60001/tools/metrics/OWNERS#ol... >> tools/metrics/OWNERS:1: set noparent >> On 2015/11/12 21:10:26, Alexei Svitkine (slow) wrote: >> >>> I think removing this is dangerous on tools/metrics. >>> >> >> Now, if you have an LGTM from a src/OWNERS (e.g. because you needed a >>> >> stamp for >> >>> chrome/ or something), you might end up submitting without a metrics >>> >> reviewer >> >>> looking at your histograms.xml changes. You can do this accidentally >>> >> (i.e. >> >>> because the tools told you had complete OWNERS coverage). >>> >> >> This is true for all OWNERS files though, right? You have to trust that >> people in OWNERS files above you don't abuse their powers. >> >> I don't think that's a good practice. Can we still have setnoparent at >>> >> least for >> >>> the XML files? Otherwise, I feel it's too easy to accidentally land >>> >> those >> >>> changes without a proper metrics review. >>> >> >> Having `noparent` is bad practice too, in general as it makes global >> refactorings harder and whatnot. But if per-file noparent is possible, >> having it for the xml files in this directory would be fine with me >> (they do seem delicate, and per-file noparent sets a slightly worse >> precedent than a full noparent). But maybe we could wait a week or two >> and see if this a real problem first? >> >> https://codereview.chromium.org/1437683006/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Often the histograms.xml is part of a larger CL that changes code, refactors, adds classes, etc. They wouldn't have lg'd the histogram changes, but maybe something else in the CL. I can look to see if I can find such an example. On Thu, Nov 12, 2015 at 4:37 PM, Nico Weber <thakis@chromium.org> wrote: > On Thu, Nov 12, 2015 at 1:26 PM, Alexei Svitkine <asvitkine@chromium.org> > wrote: > >> Just to be clear, I don't mean that src/OWNERS would abuse their powers. >> My concerns is that someone getting one of those people to review their CL >> (for a different reason - e.g. maybe they want jam@ to review content/ >> changes) would not realise they're missing a reviewer for histograms.xml >> and no one would notice and the CL would be landed. >> > > The list of toplevel OWNERS actively reviewing stuff is pretty small > though (3?), maybe we could just tell them to be mindful about this going > forward. Not sure. > > >> People already forget they need an owner review for these files and often >> find out from the cq presubmit check that they forgot to add an owner for >> histograms.xml. This happens on a regular basis - I have a watchlist so I >> can find you an example from my inbox if you'd like. This change would make >> it easier to make this mistake and not have it caught. (I guess it depends >> how often src/OWNERS review CLs - but I suspect this still happens fairly >> often). >> > > If you have an example where a toplevel owner accidentally lg'd a > histograms change, then that'd convince me :-) > > >> On Thu, Nov 12, 2015 at 4:19 PM, <thakis@chromium.org> wrote: >> >>> >>> >>> https://codereview.chromium.org/1437683006/diff/60001/tools/metrics/OWNERS >>> File tools/metrics/OWNERS (left): >>> >>> >>> https://codereview.chromium.org/1437683006/diff/60001/tools/metrics/OWNERS#ol... >>> tools/metrics/OWNERS:1: set noparent >>> On 2015/11/12 21:10:26, Alexei Svitkine (slow) wrote: >>> >>>> I think removing this is dangerous on tools/metrics. >>>> >>> >>> Now, if you have an LGTM from a src/OWNERS (e.g. because you needed a >>>> >>> stamp for >>> >>>> chrome/ or something), you might end up submitting without a metrics >>>> >>> reviewer >>> >>>> looking at your histograms.xml changes. You can do this accidentally >>>> >>> (i.e. >>> >>>> because the tools told you had complete OWNERS coverage). >>>> >>> >>> This is true for all OWNERS files though, right? You have to trust that >>> people in OWNERS files above you don't abuse their powers. >>> >>> I don't think that's a good practice. Can we still have setnoparent at >>>> >>> least for >>> >>>> the XML files? Otherwise, I feel it's too easy to accidentally land >>>> >>> those >>> >>>> changes without a proper metrics review. >>>> >>> >>> Having `noparent` is bad practice too, in general as it makes global >>> refactorings harder and whatnot. But if per-file noparent is possible, >>> having it for the xml files in this directory would be fine with me >>> (they do seem delicate, and per-file noparent sets a slightly worse >>> precedent than a full noparent). But maybe we could wait a week or two >>> and see if this a real problem first? >>> >>> https://codereview.chromium.org/1437683006/ >>> >> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
e.g. here's a CL that jam lg'd that included a histogram change. In this case, the reviewer was conscious enough to put a histograms.xml owner as a reviewer, but I'm not convinced we can rely on that to happen in general - especially for newer project members that may not realize they need an explicit review that. https://codereview.chromium.org/1407883005/ On Thu, Nov 12, 2015 at 5:32 PM, Alexei Svitkine <asvitkine@chromium.org> wrote: > Often the histograms.xml is part of a larger CL that changes code, > refactors, adds classes, etc. They wouldn't have lg'd the histogram > changes, but maybe something else in the CL. I can look to see if I can > find such an example. > > On Thu, Nov 12, 2015 at 4:37 PM, Nico Weber <thakis@chromium.org> wrote: > >> On Thu, Nov 12, 2015 at 1:26 PM, Alexei Svitkine <asvitkine@chromium.org> >> wrote: >> >>> Just to be clear, I don't mean that src/OWNERS would abuse their powers. >>> My concerns is that someone getting one of those people to review their CL >>> (for a different reason - e.g. maybe they want jam@ to review content/ >>> changes) would not realise they're missing a reviewer for histograms.xml >>> and no one would notice and the CL would be landed. >>> >> >> The list of toplevel OWNERS actively reviewing stuff is pretty small >> though (3?), maybe we could just tell them to be mindful about this going >> forward. Not sure. >> >> >>> People already forget they need an owner review for these files and >>> often find out from the cq presubmit check that they forgot to add an owner >>> for histograms.xml. This happens on a regular basis - I have a watchlist so >>> I can find you an example from my inbox if you'd like. This change would >>> make it easier to make this mistake and not have it caught. (I guess it >>> depends how often src/OWNERS review CLs - but I suspect this still happens >>> fairly often). >>> >> >> If you have an example where a toplevel owner accidentally lg'd a >> histograms change, then that'd convince me :-) >> >> >>> On Thu, Nov 12, 2015 at 4:19 PM, <thakis@chromium.org> wrote: >>> >>>> >>>> >>>> https://codereview.chromium.org/1437683006/diff/60001/tools/metrics/OWNERS >>>> File tools/metrics/OWNERS (left): >>>> >>>> >>>> https://codereview.chromium.org/1437683006/diff/60001/tools/metrics/OWNERS#ol... >>>> tools/metrics/OWNERS:1: set noparent >>>> On 2015/11/12 21:10:26, Alexei Svitkine (slow) wrote: >>>> >>>>> I think removing this is dangerous on tools/metrics. >>>>> >>>> >>>> Now, if you have an LGTM from a src/OWNERS (e.g. because you needed a >>>>> >>>> stamp for >>>> >>>>> chrome/ or something), you might end up submitting without a metrics >>>>> >>>> reviewer >>>> >>>>> looking at your histograms.xml changes. You can do this accidentally >>>>> >>>> (i.e. >>>> >>>>> because the tools told you had complete OWNERS coverage). >>>>> >>>> >>>> This is true for all OWNERS files though, right? You have to trust that >>>> people in OWNERS files above you don't abuse their powers. >>>> >>>> I don't think that's a good practice. Can we still have setnoparent at >>>>> >>>> least for >>>> >>>>> the XML files? Otherwise, I feel it's too easy to accidentally land >>>>> >>>> those >>>> >>>>> changes without a proper metrics review. >>>>> >>>> >>>> Having `noparent` is bad practice too, in general as it makes global >>>> refactorings harder and whatnot. But if per-file noparent is possible, >>>> having it for the xml files in this directory would be fine with me >>>> (they do seem delicate, and per-file noparent sets a slightly worse >>>> precedent than a full noparent). But maybe we could wait a week or two >>>> and see if this a real problem first? >>>> >>>> https://codereview.chromium.org/1437683006/ >>>> >>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Well, we wouldn't rely on new people getting this right, but on jam getting this right. But ok, let's add a per-file noparent entry then. On Thu, Nov 12, 2015 at 2:35 PM, Alexei Svitkine <asvitkine@chromium.org> wrote: > e.g. here's a CL that jam lg'd that included a histogram change. In this > case, the reviewer was conscious enough to put a histograms.xml owner as a > reviewer, but I'm not convinced we can rely on that to happen in general - > especially for newer project members that may not realize they need an > explicit review that. > > https://codereview.chromium.org/1407883005/ > > On Thu, Nov 12, 2015 at 5:32 PM, Alexei Svitkine <asvitkine@chromium.org> > wrote: > >> Often the histograms.xml is part of a larger CL that changes code, >> refactors, adds classes, etc. They wouldn't have lg'd the histogram >> changes, but maybe something else in the CL. I can look to see if I can >> find such an example. >> >> On Thu, Nov 12, 2015 at 4:37 PM, Nico Weber <thakis@chromium.org> wrote: >> >>> On Thu, Nov 12, 2015 at 1:26 PM, Alexei Svitkine <asvitkine@chromium.org >>> > wrote: >>> >>>> Just to be clear, I don't mean that src/OWNERS would abuse their >>>> powers. My concerns is that someone getting one of those people to review >>>> their CL (for a different reason - e.g. maybe they want jam@ to review >>>> content/ changes) would not realise they're missing a reviewer for >>>> histograms.xml and no one would notice and the CL would be landed. >>>> >>> >>> The list of toplevel OWNERS actively reviewing stuff is pretty small >>> though (3?), maybe we could just tell them to be mindful about this going >>> forward. Not sure. >>> >>> >>>> People already forget they need an owner review for these files and >>>> often find out from the cq presubmit check that they forgot to add an owner >>>> for histograms.xml. This happens on a regular basis - I have a watchlist so >>>> I can find you an example from my inbox if you'd like. This change would >>>> make it easier to make this mistake and not have it caught. (I guess it >>>> depends how often src/OWNERS review CLs - but I suspect this still happens >>>> fairly often). >>>> >>> >>> If you have an example where a toplevel owner accidentally lg'd a >>> histograms change, then that'd convince me :-) >>> >>> >>>> On Thu, Nov 12, 2015 at 4:19 PM, <thakis@chromium.org> wrote: >>>> >>>>> >>>>> >>>>> https://codereview.chromium.org/1437683006/diff/60001/tools/metrics/OWNERS >>>>> File tools/metrics/OWNERS (left): >>>>> >>>>> >>>>> https://codereview.chromium.org/1437683006/diff/60001/tools/metrics/OWNERS#ol... >>>>> tools/metrics/OWNERS:1: set noparent >>>>> On 2015/11/12 21:10:26, Alexei Svitkine (slow) wrote: >>>>> >>>>>> I think removing this is dangerous on tools/metrics. >>>>>> >>>>> >>>>> Now, if you have an LGTM from a src/OWNERS (e.g. because you needed a >>>>>> >>>>> stamp for >>>>> >>>>>> chrome/ or something), you might end up submitting without a metrics >>>>>> >>>>> reviewer >>>>> >>>>>> looking at your histograms.xml changes. You can do this accidentally >>>>>> >>>>> (i.e. >>>>> >>>>>> because the tools told you had complete OWNERS coverage). >>>>>> >>>>> >>>>> This is true for all OWNERS files though, right? You have to trust that >>>>> people in OWNERS files above you don't abuse their powers. >>>>> >>>>> I don't think that's a good practice. Can we still have setnoparent at >>>>>> >>>>> least for >>>>> >>>>>> the XML files? Otherwise, I feel it's too easy to accidentally land >>>>>> >>>>> those >>>>> >>>>>> changes without a proper metrics review. >>>>>> >>>>> >>>>> Having `noparent` is bad practice too, in general as it makes global >>>>> refactorings harder and whatnot. But if per-file noparent is possible, >>>>> having it for the xml files in this directory would be fine with me >>>>> (they do seem delicate, and per-file noparent sets a slightly worse >>>>> precedent than a full noparent). But maybe we could wait a week or two >>>>> and see if this a real problem first? >>>>> >>>>> https://codereview.chromium.org/1437683006/ >>>>> >>>> >>>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
SG, will do. On Thu, Nov 12, 2015 at 5:39 PM, Nico Weber <thakis@chromium.org> wrote: > Well, we wouldn't rely on new people getting this right, but on jam > getting this right. > > But ok, let's add a per-file noparent entry then. > > On Thu, Nov 12, 2015 at 2:35 PM, Alexei Svitkine <asvitkine@chromium.org> > wrote: > >> e.g. here's a CL that jam lg'd that included a histogram change. In this >> case, the reviewer was conscious enough to put a histograms.xml owner as a >> reviewer, but I'm not convinced we can rely on that to happen in general - >> especially for newer project members that may not realize they need an >> explicit review that. >> >> https://codereview.chromium.org/1407883005/ >> >> On Thu, Nov 12, 2015 at 5:32 PM, Alexei Svitkine <asvitkine@chromium.org> >> wrote: >> >>> Often the histograms.xml is part of a larger CL that changes code, >>> refactors, adds classes, etc. They wouldn't have lg'd the histogram >>> changes, but maybe something else in the CL. I can look to see if I can >>> find such an example. >>> >>> On Thu, Nov 12, 2015 at 4:37 PM, Nico Weber <thakis@chromium.org> wrote: >>> >>>> On Thu, Nov 12, 2015 at 1:26 PM, Alexei Svitkine < >>>> asvitkine@chromium.org> wrote: >>>> >>>>> Just to be clear, I don't mean that src/OWNERS would abuse their >>>>> powers. My concerns is that someone getting one of those people to review >>>>> their CL (for a different reason - e.g. maybe they want jam@ to >>>>> review content/ changes) would not realise they're missing a reviewer for >>>>> histograms.xml and no one would notice and the CL would be landed. >>>>> >>>> >>>> The list of toplevel OWNERS actively reviewing stuff is pretty small >>>> though (3?), maybe we could just tell them to be mindful about this going >>>> forward. Not sure. >>>> >>>> >>>>> People already forget they need an owner review for these files and >>>>> often find out from the cq presubmit check that they forgot to add an owner >>>>> for histograms.xml. This happens on a regular basis - I have a watchlist so >>>>> I can find you an example from my inbox if you'd like. This change would >>>>> make it easier to make this mistake and not have it caught. (I guess it >>>>> depends how often src/OWNERS review CLs - but I suspect this still happens >>>>> fairly often). >>>>> >>>> >>>> If you have an example where a toplevel owner accidentally lg'd a >>>> histograms change, then that'd convince me :-) >>>> >>>> >>>>> On Thu, Nov 12, 2015 at 4:19 PM, <thakis@chromium.org> wrote: >>>>> >>>>>> >>>>>> >>>>>> https://codereview.chromium.org/1437683006/diff/60001/tools/metrics/OWNERS >>>>>> File tools/metrics/OWNERS (left): >>>>>> >>>>>> >>>>>> https://codereview.chromium.org/1437683006/diff/60001/tools/metrics/OWNERS#ol... >>>>>> tools/metrics/OWNERS:1: set noparent >>>>>> On 2015/11/12 21:10:26, Alexei Svitkine (slow) wrote: >>>>>> >>>>>>> I think removing this is dangerous on tools/metrics. >>>>>>> >>>>>> >>>>>> Now, if you have an LGTM from a src/OWNERS (e.g. because you needed a >>>>>>> >>>>>> stamp for >>>>>> >>>>>>> chrome/ or something), you might end up submitting without a metrics >>>>>>> >>>>>> reviewer >>>>>> >>>>>>> looking at your histograms.xml changes. You can do this accidentally >>>>>>> >>>>>> (i.e. >>>>>> >>>>>>> because the tools told you had complete OWNERS coverage). >>>>>>> >>>>>> >>>>>> This is true for all OWNERS files though, right? You have to trust >>>>>> that >>>>>> people in OWNERS files above you don't abuse their powers. >>>>>> >>>>>> I don't think that's a good practice. Can we still have setnoparent at >>>>>>> >>>>>> least for >>>>>> >>>>>>> the XML files? Otherwise, I feel it's too easy to accidentally land >>>>>>> >>>>>> those >>>>>> >>>>>>> changes without a proper metrics review. >>>>>>> >>>>>> >>>>>> Having `noparent` is bad practice too, in general as it makes global >>>>>> refactorings harder and whatnot. But if per-file noparent is possible, >>>>>> having it for the xml files in this directory would be fine with me >>>>>> (they do seem delicate, and per-file noparent sets a slightly worse >>>>>> precedent than a full noparent). But maybe we could wait a week or two >>>>>> and see if this a real problem first? >>>>>> >>>>>> https://codereview.chromium.org/1437683006/ >>>>>> >>>>> >>>>> >>>> >>> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1437683006/diff/60001/tools/OWNERS File tools/OWNERS (right): https://codereview.chromium.org/1437683006/diff/60001/tools/OWNERS#newcode12 tools/OWNERS:12: per-file bisect*.py=rsesek@chromium.org I actually don't want to OWN this file (https://crrev.com/0a817adf7ea9ced8b91c008f1e405f9792d63dd0) https://codereview.chromium.org/1437683006/diff/60001/tools/OWNERS#newcode18 tools/OWNERS:18: per-file boilerplate.py=rmcilroy@chromium.org I did author this one, though.
Message was sent while issue was closed.
On 2015/11/12 22:39:28, Nico wrote: > Well, we wouldn't rely on new people getting this right, but on jam getting > this right. FWIW, it's hard for jam to "get this right". He can say "content/ lgtm, didn't look at anything else", but our tools just see "... lgtm ...". So, if the CL author isn't being super careful, zhi could just miss that nobody has reviewed their metrics changes yet. jam could wait to write "lgtm" until every file that he's not reviewing has been reviewed, but that's makes his workflow kind of a pain in the ass. I see this problem come up a lot in practice, when a //chrome OWNER stamps a CL. The //chrome OWNERS are themselves very careful to say "such and such files lgtm", but reviewers aren't always diligent about figuring out exactly what other files needed reviews, and our presubmit scripts are happy. I don't have an example CL in mind, unfortunately.
Message was sent while issue was closed.
I think no-parent for the XML file is the right trade off here
Message was sent while issue was closed.
satorux@chromium.org changed reviewers: + satorux@chromium.org
Message was sent while issue was closed.
lgtm for three lines with my name
Message was sent while issue was closed.
lgtm for three lines with my name
Message was sent while issue was closed.
On 2015/11/13 01:07:55, jochen (slow - traveling) wrote: > I think no-parent for the XML file is the right trade off here Yeah, it seems like a reasonable tradeoff. But I thought the general issue was worth raising too: It's hard for high-level OWNERS to approve only part of a CL, and sometimes this leads to OWNERS review being skipped for some files.
Message was sent while issue was closed.
Ping on this: On 2015/11/13 00:35:34, Robert Sesek wrote: > https://codereview.chromium.org/1437683006/diff/60001/tools/OWNERS > File tools/OWNERS (right): > > https://codereview.chromium.org/1437683006/diff/60001/tools/OWNERS#newcode12 > tools/OWNERS:12: per-file mailto:bisect*.py=rsesek@chromium.org > I actually don't want to OWN this file > (https://crrev.com/0a817adf7ea9ced8b91c008f1e405f9792d63dd0) > > https://codereview.chromium.org/1437683006/diff/60001/tools/OWNERS#newcode18 > tools/OWNERS:18: per-file mailto:boilerplate.py=rmcilroy@chromium.org > I did author this one, though.
Message was sent while issue was closed.
https://codereview.chromium.org/1437683006/diff/60001/tools/OWNERS File tools/OWNERS (right): https://codereview.chromium.org/1437683006/diff/60001/tools/OWNERS#newcode12 tools/OWNERS:12: per-file bisect*.py=rsesek@chromium.org On 2015/11/13 00:35:34, Robert Sesek wrote: > I actually don't want to OWN this file > (https://crrev.com/0a817adf7ea9ced8b91c008f1e405f9792d63dd0) Ah sorry, missed this. Who else besides rmcilroy should own it then?
Message was sent while issue was closed.
On 2015/12/01 21:09:19, Nico wrote: > https://codereview.chromium.org/1437683006/diff/60001/tools/OWNERS > File tools/OWNERS (right): > > https://codereview.chromium.org/1437683006/diff/60001/tools/OWNERS#newcode12 > tools/OWNERS:12: per-file mailto:bisect*.py=rsesek@chromium.org > On 2015/11/13 00:35:34, Robert Sesek wrote: > > I actually don't want to OWN this file > > (https://crrev.com/0a817adf7ea9ced8b91c008f1e405f9792d63dd0) > > Ah sorry, missed this. Who else besides rmcilroy should own it then? Maybe someone from TE? (Anyone but me ;-)
Message was sent while issue was closed.
On 2015/12/01 21:14:54, Robert Sesek wrote: > On 2015/12/01 21:09:19, Nico wrote: > > https://codereview.chromium.org/1437683006/diff/60001/tools/OWNERS > > File tools/OWNERS (right): > > > > https://codereview.chromium.org/1437683006/diff/60001/tools/OWNERS#newcode12 > > tools/OWNERS:12: per-file mailto:bisect*.py=rsesek@chromium.org > > On 2015/11/13 00:35:34, Robert Sesek wrote: > > > I actually don't want to OWN this file > > > (https://crrev.com/0a817adf7ea9ced8b91c008f1e405f9792d63dd0) > > > > Ah sorry, missed this. Who else besides rmcilroy should own it then? > > Maybe someone from TE? (Anyone but me ;-) Please add anantha@ as the owner. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
