Revert "git-cl: make SetReview ReAuth capable"

This reverts commit 6da6aecbb4.

Reason for revert: Breaks official chrome release.

Original change's description:
> git-cl: make SetReview ReAuth capable
>
> This CL changes SetReview to be ReAuth capable.
>
> This is a potential breaking change if an downstream script relies on
> gerrit_util.SetReview but hasn't implemented ReAuth.
>
> If this caused breakage, please refer to https://chromium.googlesource.com/chromium/src.git/+/HEAD/docs/gerrit_reauth.md#Troubleshooting for troubleshooting instructions.
>
> If the above doesn't work, please report a bug, then add `LUCI_BYPASS_REAUTH=1` to the environment to disable ReAuth for now.
>
> Bug: 442666611
> Change-Id: I7724f15f166f9975fc88be5d8e1c93be4e1ec302
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6939308
> Reviewed-by: Allen Li <ayatane@chromium.org>
> Reviewed-by: Scott Lee <ddoman@chromium.org>
> Commit-Queue: Jiewei Qian <qjw@chromium.org>

Bug: 442666611
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Change-Id: I7b7e769b5a8011d45e976e471d0f2e43f186e0c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6955930
Commit-Queue: Alex Kravchuk <alexanderkr@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Ben Mason <benmason@chromium.org>
changes/30/6955930/2
Jacek Dobrowolski 2 months ago committed by LUCI CQ
parent 6da6aecbb4
commit c932af8f97

@ -1766,15 +1766,10 @@ def SetReview(host,
labels=None, labels=None,
notify=None, notify=None,
ready=None, ready=None,
automatic_attention_set_update: Optional[bool] = None, automatic_attention_set_update: Optional[bool] = None):
project: Optional[str] = None):
"""Sets labels and/or adds a message to a code review. """Sets labels and/or adds a message to a code review.
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#set-review https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#set-review
We need to check if this change's `project` needs to satisfy ReAuth
requirements, and attach an ReAuth credential to the request. `project`
can be optionally be provided to save one Gerrit server round-trip.
""" """
if not msg and not labels: if not msg and not labels:
return return
@ -1791,19 +1786,7 @@ def SetReview(host,
if automatic_attention_set_update is not None: if automatic_attention_set_update is not None:
body[ body[
'ignore_automatic_attention_set_rules'] = not automatic_attention_set_update 'ignore_automatic_attention_set_rules'] = not automatic_attention_set_update
conn = CreateHttpConn(host, path, reqtype='POST', body=body)
# ReAuth needed if we set labels (notably Code-Review).
reauth_context = None
if bool(labels):
reauth_context = auth.ReAuthContext(
host=host,
project=project or GetChangeDetail(host, change)["project"])
conn = CreateHttpConn(host,
path,
reqtype='POST',
body=body,
reauth_context=reauth_context)
response = ReadHttpJsonResponse(conn) response = ReadHttpJsonResponse(conn)
if labels: if labels:
for key, val in labels.items(): for key, val in labels.items():

@ -2270,8 +2270,7 @@ class Changelist(object):
gerrit_util.SetReview(self.GetGerritHost(), gerrit_util.SetReview(self.GetGerritHost(),
self._GerritChangeIdentifier(), self._GerritChangeIdentifier(),
labels=labels, labels=labels,
notify=notify, notify=notify)
project=self.GetGerritProject())
return 0 return 0
except KeyboardInterrupt: except KeyboardInterrupt:
raise raise
@ -2589,8 +2588,7 @@ class Changelist(object):
gerrit_util.SetReview(self.GetGerritHost(), gerrit_util.SetReview(self.GetGerritHost(),
self._GerritChangeIdentifier(), self._GerritChangeIdentifier(),
msg=message, msg=message,
ready=publish, ready=publish)
project=self.GetGerritProject())
def GetCommentsSummary(self, readable=True): def GetCommentsSummary(self, readable=True):
# DETAILED_ACCOUNTS is to get emails in accounts. # DETAILED_ACCOUNTS is to get emails in accounts.

@ -656,59 +656,6 @@ class GerritUtilTest(unittest.TestCase):
mockJsonResponse.return_value = {'status': {}} mockJsonResponse.return_value = {'status': {}}
self.assertTrue(gerrit_util.IsCodeOwnersEnabledOnRepo('host', 'repo')) self.assertTrue(gerrit_util.IsCodeOwnersEnabledOnRepo('host', 'repo'))
@mock.patch('gerrit_util.CreateHttpConn')
@mock.patch('gerrit_util.ReadHttpJsonResponse')
@mock.patch('gerrit_util.GetChangeDetail')
def testSetReview_ReAuth(self, mockGetChangeDetail, mockJsonResponse,
mockCreateHttpConn):
mockJsonResponse.return_value = {"labels": {"Code-Review": 1}}
mockGetChangeDetail.return_value = {
"project": "infra/infra",
}
gerrit_util.SetReview('chromium', 123456, labels={'Code-Review': 1})
# Check `project` in reauth_context is backfilled.
mockGetChangeDetail.assert_called_with('chromium', 123456)
httpConnKwargs = mockCreateHttpConn.call_args[1]
self.assertIn('reauth_context', httpConnKwargs)
self.assertEqual(
auth.ReAuthContext(host='chromium', project='infra/infra'),
httpConnKwargs['reauth_context'])
@mock.patch('gerrit_util.CreateHttpConn')
@mock.patch('gerrit_util.ReadHttpJsonResponse')
@mock.patch('gerrit_util.GetChangeDetail')
def testSetReview_ReAuth_WithProject(self, mockGetChangeDetail,
mockJsonResponse, mockCreateHttpConn):
mockJsonResponse.return_value = {"labels": {"Code-Review": 1}}
gerrit_util.SetReview('chromium',
123456,
labels={'Code-Review': 1},
project="infra/experimental")
# Check reauth_context uses the given project.
mockGetChangeDetail.assert_not_called()
httpConnKwargs = mockCreateHttpConn.call_args[1]
self.assertIn('reauth_context', httpConnKwargs)
self.assertEqual(
auth.ReAuthContext(host='chromium', project='infra/experimental'),
httpConnKwargs['reauth_context'])
@mock.patch('gerrit_util.CreateHttpConn')
@mock.patch('gerrit_util.ReadHttpJsonResponse')
@mock.patch('gerrit_util.GetChangeDetail')
def testSetReview_ReAuthNotNeeded(self, mockGetChangeDetail,
mockJsonResponse, mockCreateHttpConn):
mockJsonResponse.return_value = {"ready": True}
gerrit_util.SetReview('chromium', 123456, msg="test")
# ReAuth not needed.
mockGetChangeDetail.assert_not_called()
httpConnKwargs = mockCreateHttpConn.call_args[1]
self.assertIsNone(httpConnKwargs.get('reauth_context', None))
class SSOAuthenticatorTest(unittest.TestCase): class SSOAuthenticatorTest(unittest.TestCase):

@ -638,11 +638,9 @@ class TestGitCl(unittest.TestCase):
mock.patch('git_cl.gerrit_util.AddReviewers', mock.patch('git_cl.gerrit_util.AddReviewers',
lambda *a: self._mocked_call('AddReviewers', *a)).start() lambda *a: self._mocked_call('AddReviewers', *a)).start()
mock.patch('git_cl.gerrit_util.SetReview', mock.patch('git_cl.gerrit_util.SetReview',
lambda h, i, msg=None, labels=None, notify=None, ready=None, lambda h, i, msg=None, labels=None, notify=None, ready=None:
automatic_attention_set_update=None, project=None:
(self._mocked_call('SetReview', h, i, msg, labels, notify, (self._mocked_call('SetReview', h, i, msg, labels, notify,
ready, automatic_attention_set_update, ready))).start()
project))).start()
mock.patch('git_cl.gerrit_util.LuciContextAuthenticator.is_applicable', mock.patch('git_cl.gerrit_util.LuciContextAuthenticator.is_applicable',
return_value=False).start() return_value=False).start()
mock.patch('git_cl.gerrit_util.GceAuthenticator.is_applicable', mock.patch('git_cl.gerrit_util.GceAuthenticator.is_applicable',
@ -2553,7 +2551,7 @@ class TestGitCl(unittest.TestCase):
(('SetReview', 'chromium-review.googlesource.com', (('SetReview', 'chromium-review.googlesource.com',
'infra%2Finfra~123', None, { 'infra%2Finfra~123', None, {
'Commit-Queue': vote 'Commit-Queue': vote
}, notify, None, None, 'infra/infra'), ''), }, notify, None), ''),
] ]
@unittest.skipIf(gclient_utils.IsEnvCog(), @unittest.skipIf(gclient_utils.IsEnvCog(),
@ -3207,8 +3205,7 @@ class TestGitCl(unittest.TestCase):
'https://chromium.googlesource.com/infra/infra') 'https://chromium.googlesource.com/infra/infra')
self.calls = [ self.calls = [
(('SetReview', 'chromium-review.googlesource.com', (('SetReview', 'chromium-review.googlesource.com',
'infra%2Finfra~10', 'msg', None, None, None, None, 'infra/infra'), 'infra%2Finfra~10', 'msg', None, None, None), None),
None),
] ]
self.assertEqual(0, git_cl.main(['comment', '-i', '10', '-a', 'msg'])) self.assertEqual(0, git_cl.main(['comment', '-i', '10', '-a', 'msg']))

Loading…
Cancel
Save