New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 625975 link

Starred by 0 users
Status: Fixed
Owner:
█ vabr@chromium.org
hobby only
Closed: Jul 2016
Cc:
dcheng@chromium.org
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

IPC presubmit check won't allow uploading to WIP CLs

Project Member Reported by vabr@chromium.org, Jul 6 2016 
What steps will reproduce the problem?
(1) Create a patch touching IPC/Mojo files, such as components/autofill/content/public/cpp/autofill_types_struct_traits.h.
(2) Use git cl upload to upload the patch to code review site, without selecting any reviewers first.

What is the expected output?
That the upload succeeds unless the code changes contain some clear error.

What do you see instead?
The presubmit script aborts with the error: "Found changes to IPC files without a security OWNER!"


More notes:
I usually do not add reviewers to the CL until it is ready for review. The presubmit script made me to use --bypass-hooks, which I do not like, because it masks other potential and useful errors.

This would ideally be just a warning, not an error. A malicious person can bypass the hooks anyway, and everyone else can read the warning.

Comment 1 by vabr@chromium.org, Jul 6 2016

Owner: dcheng@chromium.org
Status: Assigned (was: Untriaged)
dcheng@, as the author of https://codereview.chromium.org/2070483002, could you please help triaging this report?

Thanks!
Vaclav

P.S. Apologies if this has already been discussed on issue 611905, to which I do not have access.

Comment 2 by dcheng@chromium.org, Jul 11 2016 

I suppose we can make it a warning at upload time and a hard error at submit time.

yes, you can obviously bypass it but the idea is to make it harder to bypass accidentally (i don't know if a presubmit warning on the bots will actually block the CQ, for example)

Comment 3 by vabr@chromium.org, Jul 11 2016 

Thanks, a warning at upload time and a hard error at submit time would be awesome.

I get the point with avoiding accidents, but the need for using --bypass-hooks can postpone noticing other issues until submit time. My problem with this error is that it is not actionable at the moment it fires (cannot add reviewers until I want them to review).

Comment 4 by vabr@chromium.org, Jul 11 2016

Cc: dcheng@chromium.org
Owner: vabr@chromium.org
Status: Started (was: Assigned)
https://codereview.chromium.org/2135103002 in flight to implement #2.
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 11 2016 

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

commit f5ce3bf9ba864291012a0d8998f1cf07d7508c16
Author: vabr <vabr@chromium.org>
Date: Mon Jul 11 14:52:41 2016

Make the IPC OWNERS check a warning until landing

The _CheckIpcOwners check is currently an error even at non-submit time. This
makes it hard to upload changes before the patch is ready for review.

Following the discussion on the bug, this CL makes that check a warning unless
it is happening at submit time, in which case it remains an error.

BUG= 625975 

Review-Url: https://codereview.chromium.org/2135103002
Cr-Commit-Position: refs/heads/master@{#404657}

[modify] https://crrev.com/f5ce3bf9ba864291012a0d8998f1cf07d7508c16/PRESUBMIT.py

Comment 6 by vabr@chromium.org, Jul 11 2016

Status: Fixed (was: Started)
Thanks for your support and review, dcheng! :)
►

Sign in to add a comment