From a657331e90e23e289e85a92af49b64829151f403 Mon Sep 17 00:00:00 2001 From: Takuto Ikuta Date: Thu, 20 Jan 2022 00:24:57 +0000 Subject: [PATCH] ninjalog_uploader: use goma_auth to detect googler Bug: 1288639 Change-Id: I447e2f66603ffb8d68599dcf22023fd7857dc4fd Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3400398 Reviewed-by: Fumitoshi Ukai Reviewed-by: Josip Sokcevic Commit-Queue: Takuto Ikuta --- PRESUBMIT.py | 31 ++++++++++++++++++++++--------- ninjalog.README.md | 4 +--- ninjalog_uploader.py | 20 +++++++++++--------- ninjalog_uploader_wrapper.py | 7 ++++--- tests/ninjalog_uploader_test.py | 18 ++++++++++++++++++ 5 files changed, 56 insertions(+), 24 deletions(-) diff --git a/PRESUBMIT.py b/PRESUBMIT.py index 58ef96cd2..ef1c98781 100644 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py @@ -112,22 +112,36 @@ def CheckUnitTestsOnCommit(input_api, output_api): 'recipes_test.py', ] + py3_only_tests = ['ninjalog_uploader_test.py'] + tests = input_api.canned_checks.GetUnitTestsInDirectory( input_api, output_api, 'tests', files_to_check=test_to_run_list, - files_to_skip=tests_to_skip_list + py2_only_tests, + files_to_skip=tests_to_skip_list + py2_only_tests + py3_only_tests, run_on_python3=True) # TODO: once py3 compatbile, remove those tests - tests.extend(input_api.canned_checks.GetUnitTestsInDirectory( - input_api, - output_api, - 'tests', - files_to_check=py2_only_tests, - files_to_skip=tests_to_skip_list, - run_on_python3=False)) + tests.extend( + input_api.canned_checks.GetUnitTestsInDirectory( + input_api, + output_api, + 'tests', + files_to_check=py2_only_tests, + files_to_skip=tests_to_skip_list, + run_on_python3=False)) + + # TODO: use this for all tests when py2 support is dropped. + tests.extend( + input_api.canned_checks.GetUnitTestsInDirectory( + input_api, + output_api, + 'tests', + files_to_check=py3_only_tests, + files_to_skip=tests_to_skip_list, + run_on_python3=True, + run_on_python2=False)) return input_api.RunTests(tests) @@ -177,4 +191,3 @@ def CheckOwnersOnUpload(input_api, output_api): def CheckDoNotSubmitOnCommit(input_api, output_api): return input_api.canned_checks.CheckDoNotSubmit(input_api, output_api) - diff --git a/ninjalog.README.md b/ninjalog.README.md index b1e5284c6..ddfa9fc39 100644 --- a/ninjalog.README.md +++ b/ninjalog.README.md @@ -14,9 +14,7 @@ $ autoninja -C out/Release chrome autoninja uploads ninja's build log for Google employees but we don't collect logs from external contributors. -We use [this page](https://chromium-build-stats-staging.appspot.com/should-upload) -to decide whether an autoninja user is a Googler or not. Only people with access -to Google's internal network can see 'Success'. +We use `goma_auth info` to decide whether an autoninja user is a Googler or not. Before uploading logs, autoninja shows a message 10 times to warn users that we will collect build logs. diff --git a/ninjalog_uploader.py b/ninjalog_uploader.py index 3aa0239e6..18ca64d3e 100755 --- a/ninjalog_uploader.py +++ b/ninjalog_uploader.py @@ -26,8 +26,6 @@ import subprocess import sys import time -from third_party.six.moves import http_client -from third_party.six.moves.urllib import error from third_party.six.moves.urllib import request # These build configs affect build performance. @@ -39,13 +37,17 @@ ALLOWLISTED_CONFIGS = ('symbol_level', 'use_goma', 'is_debug', 'use_errorprone_java_compiler', 'incremental_install') -def IsGoogler(server): - """Check whether this script run inside corp network.""" - try: - resp = request.urlopen('https://' + server + '/should-upload') - return resp.read() == b'Success' - except (error.URLError, http_client.RemoteDisconnected): +def IsGoogler(): + """Check whether this user is Googler or not.""" + p = subprocess.run('goma_auth info', + capture_output=True, + text=True, + shell=True) + if p.returncode != 0: return False + l = p.stdout.splitlines()[0] + # |l| will be like 'Login as @google.com' for googler using goma. + return l.startswith('Login as ') and l.endswith('@google.com') def ParseGNArgs(gn_args): @@ -190,7 +192,7 @@ def main(): # Disable logging. logging.disable(logging.CRITICAL) - if not IsGoogler(args.server): + if not IsGoogler(): return 0 ninjalog = args.ninjalog or GetNinjalog(args.cmdline) diff --git a/ninjalog_uploader_wrapper.py b/ninjalog_uploader_wrapper.py index fbfca3f29..6ce7dba27 100755 --- a/ninjalog_uploader_wrapper.py +++ b/ninjalog_uploader_wrapper.py @@ -17,7 +17,7 @@ import subprocess2 THIS_DIR = os.path.dirname(__file__) UPLOADER = os.path.join(THIS_DIR, 'ninjalog_uploader.py') CONFIG = os.path.join(THIS_DIR, 'ninjalog.cfg') -VERSION = 2 +VERSION = 3 def LoadConfig(): @@ -29,8 +29,7 @@ def LoadConfig(): return config return { - 'is-googler': - ninjalog_uploader.IsGoogler('chromium-build-stats.appspot.com'), + 'is-googler': ninjalog_uploader.IsGoogler(), 'countdown': 10, 'version': VERSION, } @@ -69,6 +68,8 @@ If you have questions about this, please send mail to infra-dev@chromium.org You can find a more detailed explanation in %s +or +https://chromium.googlesource.com/chromium/tools/depot_tools/+/main/ninjalog.README.md """ % (whitelisted, countdown, __file__, __file__, os.path.abspath(os.path.join(THIS_DIR, "ninjalog.README.md")))) diff --git a/tests/ninjalog_uploader_test.py b/tests/ninjalog_uploader_test.py index 5b22f2688..facc20dfc 100755 --- a/tests/ninjalog_uploader_test.py +++ b/tests/ninjalog_uploader_test.py @@ -7,6 +7,7 @@ import json import os import sys import unittest +import unittest.mock ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) sys.path.insert(0, ROOT_DIR) @@ -15,6 +16,23 @@ import ninjalog_uploader class NinjalogUploaderTest(unittest.TestCase): + def test_IsGoogler(self): + with unittest.mock.patch('subprocess.run') as run_mock: + run_mock.return_value.returncode = 0 + run_mock.return_value.stdout = ('Login as foo@google.com\n' + 'goma is ready to use') + self.assertTrue(ninjalog_uploader.IsGoogler()) + + with unittest.mock.patch('subprocess.run') as run_mock: + run_mock.return_value.returncode = 1 + run_mock.return_value.stdout = 'Not logged in\n' + self.assertFalse(ninjalog_uploader.IsGoogler()) + + with unittest.mock.patch('subprocess.run') as run_mock: + run_mock.return_value.returncode = 0 + run_mock.return_value.stdout = 'Login as foo@example.com\n' + self.assertFalse(ninjalog_uploader.IsGoogler()) + def test_parse_gn_args(self): self.assertEqual(ninjalog_uploader.ParseGNArgs(json.dumps([])), {})