Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1882583003:
Gerrit git cl upload: add check for missing credentials. (Closed)
|
Created:
4 years, 8 months ago by tandrii(chromium) Modified:
4 years, 8 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@R250 Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionGerrit git cl upload: add check for missing credentials.
Checks creds before uploading and running presubmit, generalizing
the case of Rietveld. If they are missing, suggests a URL to
generate them.
R=andybons@chromium.org,phajdan.jr@chromium.org
BUG=583153
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299883
Patch Set 1 : rebase #Patch Set 2 : fianlly, with tests #Patch Set 3 : rebase #Patch Set 4 : fix #Patch Set 5 : pylint + mocks #Patch Set 6 : and fix pylint + proper mocks #
Total comments: 14
Patch Set 7 : review #Patch Set 8 : argh,fix #
Dependent Patchsets: Messages
Total messages: 35 (17 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
PTAL
lgtm
The CQ bit was checked by tandrii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882583003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882583003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Depot Tools Presubmit on tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Depot%20Tools%20Presubm...)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882583003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882583003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Depot Tools Presubmit on tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Depot%20Tools%20Presubm...)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882583003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882583003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Depot Tools Presubmit on tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Depot%20Tools%20Presubm...)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882583003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882583003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tandrii@chromium.org changed reviewers: + machenbach@chromium.org
+machenbach@ for recent changes. https://codereview.chromium.org/1882583003/diff/140001/git_cl.py File git_cl.py (left): https://codereview.chromium.org/1882583003/diff/140001/git_cl.py#oldcode1788 git_cl.py:1788: # TODO(tandrii): refactor this ugliness into _RietveldChangelistImpl. this has been done in previous Cls already.
lgtm with suggestions https://codereview.chromium.org/1882583003/diff/140001/gerrit_util.py File gerrit_util.py (right): https://codereview.chromium.org/1882583003/diff/140001/gerrit_util.py#newcode104 gerrit_util.py:104: parts[0] = parts[0] + '-review' I'm a fan of parts[0] += '-review' up to you https://codereview.chromium.org/1882583003/diff/140001/gerrit_util.py#newcode105 gerrit_util.py:105: url = 'https://%s/new-password' % ('.'.join(parts)) Maybe assert not host.startswith('http') I know it doesn't, but an assert makes the invariants clearer. https://codereview.chromium.org/1882583003/diff/140001/gerrit_util.py#newcode183 gerrit_util.py:183: NetrcAuthenticator = CookiesAuthenticator Optional: Maybe let the NetrcAuthenticator print a deprecation warning on creation? Of course even printing might break people. https://codereview.chromium.org/1882583003/diff/140001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1882583003/diff/140001/git_cl.py#newcode1988 git_cl.py:1988: git_host = self._GetGitHost() Guess calling getgithost multiple times is cheap as the git calls made are all local? https://codereview.chromium.org/1882583003/diff/140001/git_cl.py#newcode2012 git_cl.py:2012: [] if gerrit_auth else [self._gerrit_host] + Different code, not sure if more or less readable? bool(gerrit_auth) * [self._gerrit_host] + bool(git_auth) * [git_host] https://codereview.chromium.org/1882583003/diff/140001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1882583003/diff/140001/tests/git_cl_test.py#n... tests/git_cl_test.py:1283: 'press Enter to continue, Ctrl+C to abort.'],), '')) Does it need that indentation?
LGTM https://codereview.chromium.org/1882583003/diff/140001/gerrit_util.py File gerrit_util.py (right): https://codereview.chromium.org/1882583003/diff/140001/gerrit_util.py#newcode104 gerrit_util.py:104: parts[0] = parts[0] + '-review' On 2016/04/13 at 10:29:13, Michael Achenbach wrote: > I'm a fan of > parts[0] += '-review' > up to you +1 https://codereview.chromium.org/1882583003/diff/140001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1882583003/diff/140001/git_cl.py#newcode2012 git_cl.py:2012: [] if gerrit_auth else [self._gerrit_host] + On 2016/04/13 at 10:29:13, Michael Achenbach wrote: > Different code, not sure if more or less readable? > bool(gerrit_auth) * [self._gerrit_host] + bool(git_auth) * [git_host] FWIW, I prefer the version in the patch, not in the comment.
https://codereview.chromium.org/1882583003/diff/140001/gerrit_util.py File gerrit_util.py (right): https://codereview.chromium.org/1882583003/diff/140001/gerrit_util.py#newcode104 gerrit_util.py:104: parts[0] = parts[0] + '-review' On 2016/04/13 10:29:13, Michael Achenbach wrote: > I'm a fan of > parts[0] += '-review' > up to you Done. https://codereview.chromium.org/1882583003/diff/140001/gerrit_util.py#newcode183 gerrit_util.py:183: NetrcAuthenticator = CookiesAuthenticator On 2016/04/13 10:29:13, Michael Achenbach wrote: > Optional: Maybe let the NetrcAuthenticator print a deprecation warning on > creation? Of course even printing might break people. skipping because depot_tool will always be now a collection of (deprecated || broken) stuff. https://codereview.chromium.org/1882583003/diff/140001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/1882583003/diff/140001/git_cl.py#newcode1988 git_cl.py:1988: git_host = self._GetGitHost() On 2016/04/13 10:29:13, Michael Achenbach wrote: > Guess calling getgithost multiple times is cheap as the git calls made are all > local? Yes. https://codereview.chromium.org/1882583003/diff/140001/git_cl.py#newcode2012 git_cl.py:2012: [] if gerrit_auth else [self._gerrit_host] + On 2016/04/13 10:29:13, Michael Achenbach wrote: > Different code, not sure if more or less readable? > bool(gerrit_auth) * [self._gerrit_host] + bool(git_auth) * [git_host] Acknowledged. https://codereview.chromium.org/1882583003/diff/140001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/1882583003/diff/140001/tests/git_cl_test.py#n... tests/git_cl_test.py:1283: 'press Enter to continue, Ctrl+C to abort.'],), '')) On 2016/04/13 10:29:13, Michael Achenbach wrote: > Does it need that indentation? yeah, because it's not two elements of the list, but a looooooong string, which is part of the list.
The CQ bit was checked by tandrii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from andybons@chromium.org Link to the patchset: https://codereview.chromium.org/1882583003/#ps160001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882583003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882583003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Depot Tools Presubmit on tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Depot%20Tools%20Presubm...)
The CQ bit was checked by tandrii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phajdan.jr@chromium.org, andybons@chromium.org, machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/1882583003/#ps180001 (title: "argh,fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882583003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882583003/180001
Message was sent while issue was closed.
Description was changed from ========== Gerrit git cl upload: add check for missing credentials. Checks creds before uploading and running presubmit, generalizing the case of Rietveld. If they are missing, suggests a URL to generate them. R=andybons@chromium.org,phajdan.jr@chromium.org BUG=583153 ========== to ========== Gerrit git cl upload: add check for missing credentials. Checks creds before uploading and running presubmit, generalizing the case of Rietveld. If they are missing, suggests a URL to generate them. R=andybons@chromium.org,phajdan.jr@chromium.org BUG=583153 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299883 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=299883 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
