Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2797673002:
Add global status comments for owners (Closed)
|
Created:
3 years, 8 months ago by jochen (gone - plz use gerrit) Modified:
3 years, 8 months ago CC:
agable+watch_chromium.org, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd global status comments for owners
BUG=694222
R=dpranke@chromium.org
Review-Url: https://codereview.chromium.org/2797673002
Cr-Commit-Position: refs/heads/master@{#462800}
Committed: https://chromium.googlesource.com/chromium/src/+/fb96ed753ed737db5befc119248c110e41134814
Patch Set 1 #
Total comments: 2
Patch Set 2 : updates #
Messages
Total messages: 27 (14 generated)
The CQ bit was checked by jochen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
agable@chromium.org changed reviewers: + agable@chromium.org
Storing this in build/ seems odd. The vast majority of chromium developers have never made a change to build/. It also doesn't seem discoverable -- I don't see messaging built in to git-cl-owners pointing people at this file if they want to add a status message. If I were an OWNER of various files in chromium, *and* I knew that this file existed at all, I would expect it to be at the root of the repo that contains files I own. Secondly, this seems like information that doesn't need to be versioned. Similar to Rietveld nicknames and Monorail status messages, it seems like this should be stored in a lightweight app somewhere -- or better yet, in Gerrit itself. That said, code review isn't the place for design discussion, and I assume you've talked this over with dpranke already, so LGTM.
On 2017/04/04 at 15:25:01, agable wrote: > Storing this in build/ seems odd. The vast majority of chromium developers have never made a change to build/. It also doesn't seem discoverable -- I don't see messaging built in to git-cl-owners pointing people at this file if they want to add a status message. If I were an OWNER of various files in chromium, *and* I knew that this file existed at all, I would expect it to be at the root of the repo that contains files I own. The reason for putting it into build/ is that then all projects that map build/ in will get the file automatically. It is true that if we'd pull the information from gerrit or some other server, it would also be globally available, however, the other comments are also taken directly from the OWNERS files, so I don't think it's all that strange. I also don't expect that too many reviewers will be in a situation that they wish to annotate themselves, so I plan to send a PSA and hope that this catches everybody. > > Secondly, this seems like information that doesn't need to be versioned. Similar to Rietveld nicknames and Monorail status messages, it seems like this should be stored in a lightweight app somewhere -- or better yet, in Gerrit itself. > > That said, code review isn't the place for design discussion, and I assume you've talked this over with dpranke already, so LGTM. Dirk, wdyt?
On 2017/04/04 16:55:40, jochen wrote: > On 2017/04/04 at 15:25:01, agable wrote: > > Storing this in build/ seems odd. The vast majority of chromium developers > have never made a change to build/. It also doesn't seem discoverable -- I don't > see messaging built in to git-cl-owners pointing people at this file if they > want to add a status message. If I were an OWNER of various files in chromium, > *and* I knew that this file existed at all, I would expect it to be at the root > of the repo that contains files I own. > > The reason for putting it into build/ is that then all projects that map build/ > in will get the file automatically. It is true that if we'd pull the information > from gerrit or some other server, it would also be globally available, however, > the other comments are also taken directly from the OWNERS files, so I don't > think it's all that strange. > > I also don't expect that too many reviewers will be in a situation that they > wish to annotate themselves, so I plan to send a PSA and hope that this catches > everybody. > > > > > Secondly, this seems like information that doesn't need to be versioned. > Similar to Rietveld nicknames and Monorail status messages, it seems like this > should be stored in a lightweight app somewhere -- or better yet, in Gerrit > itself. > > > > That said, code review isn't the place for design discussion, and I assume > you've talked this over with dpranke already, so LGTM. > > Dirk, wdyt? Well, we don't have that lightweight app, and everything else about owners is configured as files in the repo. I agree that the status being versioned makes less sense, though. It's not clear to me that this feature is actually going to be useful to anyone but jochen -- :) -- so I'd rather not put too much effort into it for now. We can change things if we need in the future.
https://codereview.chromium.org/2797673002/diff/1/build/OWNERS File build/OWNERS (right): https://codereview.chromium.org/2797673002/diff/1/build/OWNERS#newcode15 build/OWNERS:15: per-file OWNERS.status=* why is a per-file setting needed?
https://codereview.chromium.org/2797673002/diff/1/build/OWNERS File build/OWNERS (right): https://codereview.chromium.org/2797673002/diff/1/build/OWNERS#newcode15 build/OWNERS:15: per-file OWNERS.status=* On 2017/04/04 at 17:05:25, Dirk Pranke wrote: > why is a per-file setting needed? so setting a status doesn't require a review
On 2017/04/04 17:06:50, jochen wrote: > https://codereview.chromium.org/2797673002/diff/1/build/OWNERS > File build/OWNERS (right): > > https://codereview.chromium.org/2797673002/diff/1/build/OWNERS#newcode15 > build/OWNERS:15: per-file OWNERS.status=* > On 2017/04/04 at 17:05:25, Dirk Pranke wrote: > > why is a per-file setting needed? > > so setting a status doesn't require a review Oh, duh. Right. For some reason I thought that this was saying that some files had their own OWNERS.status file, i.e., I totally misread this :). lgtm.
still lg?
yes, still lgtm, thanks!
jochen@chromium.org changed reviewers: + brettw@chromium.org
brettw for top-level approval. ptal
The CQ bit was checked by jochen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by jochen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agable@chromium.org Link to the patchset: https://codereview.chromium.org/2797673002/#ps20001 (title: "updates")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491547953469880,
"parent_rev": "9ae4bac8c7152820dede06e35c5099cb1ae43662", "commit_rev":
"fb96ed753ed737db5befc119248c110e41134814"}
Message was sent while issue was closed.
Description was changed from ========== Add global status comments for owners BUG=694222 R=dpranke@chromium.org ========== to ========== Add global status comments for owners BUG=694222 R=dpranke@chromium.org Review-Url: https://codereview.chromium.org/2797673002 Cr-Commit-Position: refs/heads/master@{#462800} Committed: https://chromium.googlesource.com/chromium/src/+/fb96ed753ed737db5befc119248c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/fb96ed753ed737db5befc119248c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
