commit 3008dc1af73b215a8015b76fe356f84becafa6d7
author Daniel Cheng <dcheng@chromium.org> Fri May 13 04:02:11 2022
committer Chromium LUCI CQ <chromium-scoped@luci-project-accounts.iam.gserviceaccount.com> Fri May 13 04:02:11 2022
tree 14569b77fe0020acf7470f8d8c0a427691a150d5
parent af0fc4d4c776e151850661a6bb0cd8812457aa00

Don't require approval during CQ+1 for security owners check.

CQ+1 runs the presubmit commit hooks, but it is not useful for the
missing security reviewers check to require approval, since there may be
reviewers that have yet to respond. Only CQ+2 should generate an error
for a missing approval.

Bug: 801315
Change-Id: I76ed35d515987a1efb9a8a2344210bc601a22849
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3646825
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Giovanni Ortuno Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1002990}

PRESUBMIT.py

diff --git a/PRESUBMIT.py b/PRESUBMIT.py
index 9a47b40..be65042 100644
--- a/PRESUBMIT.py
+++ b/PRESUBMIT.py
@@ -2246,6 +2246,8 @@
                     '--tbr was specified, skipping OWNERS check for DEPS additions'
                 )
             ]
+        # TODO(dcheng): Make this generate an error on dry runs if the reviewer
+        # is not added, to prevent review serialization.
         if input_api.dry_run:
             return [
                 output_api.PresubmitNotifyResult(
@@ -2768,7 +2770,9 @@
     """
     owner_email, reviewers = (
         input_api.canned_checks.GetCodereviewOwnerAndReviewers(
-            input_api, None, approval_needed=input_api.is_committing))
+            input_api,
+            None,
+            approval_needed=input_api.is_committing and not input_api.dry_run))
 
     security_owners = input_api.owners_client.ListOwners(owners_file)
     return any(owner in reviewers for owner in security_owners)
@@ -3010,10 +3014,6 @@
         output_api.AppendCC('ipc-security-reviews@chromium.org')
 
     results = []
-    if input_api.is_committing:
-        make_presubmit_message = output_api.PresubmitError
-    else:
-        make_presubmit_message = output_api.PresubmitPromptWarning
 
     # Ensure that a security reviewer is included as a CL reviewer. This is a
     # hack, but is needed because the OWNERS check (by design) ignores new
@@ -3024,6 +3024,12 @@
     missing_reviewer_errors.extend(fuchsia_results.missing_reviewer_errors)
 
     if missing_reviewer_errors:
+        # Missing reviewers are only a warning at upload time; otherwise, it'd
+        # be impossible to upload a change.
+        if input_api.is_committing:
+            make_presubmit_message = output_api.PresubmitError
+        else:
+            make_presubmit_message = output_api.PresubmitPromptWarning
         results.append(
             make_presubmit_message(
                 'Found missing security reviewers:',
@@ -3034,8 +3040,13 @@
     owners_file_errors.extend(fuchsia_results.owners_file_errors)
 
     if owners_file_errors:
+        # Missing per-file rules are always an error. While swarming and caching
+        # means that uploading a patchset with updated OWNERS files and sending
+        # it to the CQ again should not have a large incremental cost, it is
+        # still frustrating to discover the error only after the change has
+        # already been uploaded.
         results.append(
-            make_presubmit_message(
+            output_api.PresubmitError(
                 'Found OWNERS files with missing per-file rules for '
                 'security-sensitive files.\nPlease update the OWNERS files '
                 'below to add the missing rules:',