The code to load .netrc file copies it to a temporary file
named $TEMP/XXXXXXXXXgerrit_util/netrc before loading it
to workaround crbug.com/664664.
However, using a netrc file is deprecated (in favor of a
gitcookies file instead) so most of the time, the file is
empty. Instead of creating an empty file, load data from
/dev/null (named os.devnull).
This avoid leaving an empty file lying around (since the
temporary file is not deleted), and avoid race-conditions
on Windows when multiple processes/threads try to create
the same file.
Bug: none
Change-Id: I873b72402038ca3f0d32387353dc89b731aaa927
Reviewed-on: https://chromium-review.googlesource.com/1132998
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Ryan Tseng <hinoka@chromium.org>
Unfortunately, w/o rewrite of gerrit_util, one can't take advantage
of existing auth.Authenticator (which also doesn't know about ~/.netrc
or .gitcookies, but that's fixable). This rewrite, however, will likely
cause issues for nasty scripts outside of depot_tools which use
gerrit_util as a Python library. So, I followed outdated way of
gerrit_util :(
Also contains refactoring of auth library, which was kept backwards
compatible for the same reasons as above.
R=vadimsh@chromium.org
Test-with: led tool
Test-artifact: https://ci.chromium.org/swarming/task/3cf4687dfd26ca10?server=chromium-swarm.appspot.com
Bug: 834536
Change-Id: I6b84b8b5732aee5d345e2b2ba44f47aaecc0f6c5
Reviewed-on: https://chromium-review.googlesource.com/1018488
Reviewed-by: Vadim Shtayura <vadimsh@chromium.org>
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
Looks like the error codes issued by python's http library might vary
between versions/distros. The code tries to reach an URL which is only valid
inside GCE and in the event of getting an error it assumes the user is
a non-googler.
At least in this case, we're seeing:
third_party.httplib2.socks.HTTPError: (404, 'No such domain')
Adding this exception type allows the function to work correctly.
Change-Id: I08d3656b25d9046f8b2fc0da2a2b7c7a698c1d92
Reviewed-on: https://chromium-review.googlesource.com/978212
Commit-Queue: Robbie Iannucci <iannucci@chromium.org>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
This allows people to configure their gitcookies file to be
whereever they want. As long as it actually exists and has
credentials in it, we'll accept it, just like git itself.
Change-Id: I4aa4806ddca0e61b28b003b0d3bc486407c13ab4
Reviewed-on: https://chromium-review.googlesource.com/951917
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>
This is a bandaid (over Gerrit's replication lag), but hopefully
this will increase the likelihood of success and improve the
messaging around retries at the same time.
R=tandrii
Bug: 792679
Change-Id: Ia6e3133cc23ea9b2abfeacc05750c002f8804e8b
Reviewed-on: https://chromium-review.googlesource.com/812564
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Gerrit's "Set Review" API has a built-in trap:
drafts (optional)
How draft comments are handled that are already in the
database but that were not also described in this input.
Allowed values are DELETE, PUBLISH, PUBLISH_ALL_REVISIONS and
KEEP. All values except PUBLISH_ALL_REVISIONS operate only on
drafts for a single revision.
If not set, the default is DELETE.
That destructive default is almost never what a person wants. Use
KEEP instead.
Noticed because "git cl try" was losing people's comments.
Bug: gerrit:6812
Change-Id: I1d09231ea23b13c925d301241bef266df2ddf1cf
Reviewed-on: https://chromium-review.googlesource.com/764488
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Jonathan Nieder <jrn@google.com>
This is needed in the release recipes because there are times when we
want to check that a file is _not_ there yet (i.e. 404 response).
R=agable@chromium.org, dnj@chromium.org
Change-Id: Iedf646d6c73ebf898e90afd2009840b1e5c5b1ab
Reviewed-on: https://chromium-review.googlesource.com/709916
Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Commit-Queue: Michael Moss <mmoss@chromium.org>
The 'notify' parameter was accidentally left out in
https://crrev.com/c/544118. The calling code in git_cl.py
already sets notify='NONE', so this will suppress all
notifications as before.
Bug: 757617
Change-Id: Ib0144d644d54b194fac0c0eb2fb51a6affa3340e
Reviewed-on: https://chromium-review.googlesource.com/634245
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>
In Rietveld, adding a comment to a change automatically
published it no matter what. In Gerrit, we need to explicitly
mark the change as Ready for Review. This CL adds a new
parameter to the wrapper methods around the SetReview
API so that they can mark changes as Ready.
Bug: 740950
Change-Id: Icb2ad7c5beb03a4760657a761841745f0d75514e
Reviewed-on: https://chromium-review.googlesource.com/572031
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>
This'll allow to query gerrit from recipes for e.g. current changes in
CQ.
Bug: 685318
Change-Id: I73d08d4b186b2e5fe044fd4d4fafd9db62e27066
Reviewed-on: https://chromium-review.googlesource.com/558939
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Reviewed-by: Aaron Gable <agable@chromium.org>
https://gerrit-review.googlesource.com/c/110057/ has been deployed,
so this change can now take advantage of that new API to make
git-cl-description more reliable.
Both the API to add content to a change edit and to publish a
change edit return '204 No Content' on a successful call, but this
API returns 200. It seems likely to me that one or the other will
change, so this calling code accepts both codes for future-proofness.
Bug: gerrit:5099
Change-Id: I14d5323f92c807036f8aa11b314d10706c51dbf5
Reviewed-on: https://chromium-review.googlesource.com/544118
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
This brings the gerrit version of "git cl comments" into line
with the Rietveld implementation by including file- and line-level
comments as well as top-level review comments. It requires an
extra API call to do so, so this may result in some slow-down, but
the result is worth it.
It formats the comments to match the formatting used in the
PolyGerrit UI, with the addition of visible URLs linking to
the comment since we can't hyperlink text in the terminal.
This CL also causes it to ignore messages and comments with
the 'autogenerated' tag, which are generally less interesting
and clutter the output.
Bug: 726514
Change-Id: I1fd939d90259b43886ddc209c0e727eab36cc9c9
Reviewed-on: https://chromium-review.googlesource.com/520722
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Using --no-squash resulted in an exception being thrown, because issue
number is not set in that case.
Change-Id: Iaf0d7eb05851eba0d5231640c077b845bca486ee
Reviewed-on: https://chromium-review.googlesource.com/505509
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Wiktor Garbacz <wiktorg@google.com>
We usually only retry on server errors. But most 404s that
clients will recieve are due to replication lag, not due to
querying an issue that truly doesn't exist. This CL lets
gerrit_util (which is used primarily by git-cl) to retry on
404s to help fight errors due to replication lag.
R=tandrii@chromium.org
Bug: 717197
Change-Id: I26dbbf6c4895a13f7d57e4be3c23450111c949bc
Reviewed-on: https://chromium-review.googlesource.com/510000
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>
The old system had two faults:
* It set reviewers and ccs via different mechanisms, which is confusing
* It set CCs with a single call for each, resulting in N separate emails,
with each email going to the uploader and all reviewers and all previous
CCs.
This new system just collects all reviewers and CCs, and sets them
in a single call. That call will fail if *any* of the individual
reviewers or ccs fail, so it also parses the response and retries with
only the ones which would have succeeded on their own. If that second
call fails, or the first fails in an unexpected way, it raises an
exception like normal
Bug: 710028
Change-Id: I1be508487a41f0b68f9c41908229b8f5342830a3
Reviewed-on: https://chromium-review.googlesource.com/479712
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
This makes the library behave more like what we'd expect,
while still allowing certain function to specify that
returning 204 or 404 is expected/acceptable behavior.
Change-Id: If3ce5598d1603819ee97aaeab0072a9e786ed96d
Reviewed-on: https://chromium-review.googlesource.com/481043
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>
Gerrit sometimes returns a full response json object at
the same time as returning a non-200 status code. This
refactor makes it easier for calling code to request
access to that object and handle error cases on its own.
The original version of this commit had a bug where
ReadHttpResponse properly set the default value for
accept_statuses, but all calls which came through
ReadHttpJsonResponse were setting None instead.
Bug: 710028
Change-Id: I8cee435d8acd487fb777b3fd69b5e48e19d2e5a3
Reviewed-on: https://chromium-review.googlesource.com/481060
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>
This reverts commit 6d7ab1bfe5.
Reason for revert: Stacktrace:
File "/s/depot_tools/gerrit_util.py", line 816, in GetAccountDetails
return ReadHttpJsonResponse(conn)
File "/s/depot_tools/gerrit_util.py", line 376, in ReadHttpJsonResponse
fh = ReadHttpResponse(conn, accept_statuses)
File "/s/depot_tools/gerrit_util.py", line 365, in ReadHttpResponse
if response.status not in accept_statuses:
TypeError: argument of type 'NoneType' is not iterable
Original change's description:
> Refactor ReadHttpResponse to be error-friendlier
>
> Gerrit sometimes returns a full response json object at
> the same time as returning a non-200 status code. This
> refactor makes it easier for calling code to request
> access to that object and handle error cases on its own.
>
> Bug: 710028
> Change-Id: Id1017d580d2fb843d5ca6287efcfed8775c52cd6
> Reviewed-on: https://chromium-review.googlesource.com/479450
> Commit-Queue: Aaron Gable <agable@chromium.org>
> Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
>
TBR=agable@chromium.org,tandrii@chromium.org,chromium-reviews@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
Change-Id: Ia9d9ce835e207a32e7cc8ee35c0cf40c823c7b78
Reviewed-on: https://chromium-review.googlesource.com/481059
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>
Gerrit sometimes returns a full response json object at
the same time as returning a non-200 status code. This
refactor makes it easier for calling code to request
access to that object and handle error cases on its own.
Bug: 710028
Change-Id: Id1017d580d2fb843d5ca6287efcfed8775c52cd6
Reviewed-on: https://chromium-review.googlesource.com/479450
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
httplib2 uses an on-disk cache whose path is given by the
first argument to the httplib2.Http constructor. The `(host)`
parameter was left over from some earlier edition of this
code that didn't do on-disk caching.
Bug: 705046
Change-Id: I061324e4d1bbd680502230aefc73fe66daccaba1
Reviewed-on: https://chromium-review.googlesource.com/458967
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>
Retain the httplib import to continue using its constants, but actually make
the http(s) connections using httplib2. The latter has built-in support for
proxy settings, which then actually allows people behind proxies to interact
with Gerrit.
Compared to httplib, the biggest changes are:
- There's only one Http class instead of HTTPConnection and HTTPSConnection.
- Http.request() returns a tuple (response, contents).
- Http.request() expects a full URI instead of just a path, as Http's
constructor does not take a host parameter.
- The response object inherits from dict.
- All headers in a response are lower-cased.
All in all, it is possible to see that httplib2 support was retro-fitted
into the code, but that should not worsen its readability.
Changes since https://chromium-review.googlesource.com/c/458221/:
- Catch httplib2.ServerNotFoundError exceptions in GceAuthenticator, as httplib2
catches socket.gaierror (which was previously being used to detect when
metadata.google.internal was not accessible) and throws a ServerNotFoundError
exception instead.
Patch written in collaboration with Alexis Menard <alexis.menard@intel.com>.
BUG=672729
R=alexis.menard@intel.com,agable@chromium.org,tandrii@chromium.org
Change-Id: Iaefa9caf3d8c0bc6ff67f562cd0b6bd9fade2f24
Reviewed-on: https://chromium-review.googlesource.com/458480
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
This reverts commit d75686454b.
Reason for revert:
third_party.httplib2.ServerNotFoundError: Unable to find the server at metadata.google.internal
Original change's description:
> gerrit_util: Use httplib2 for communication instead of httplib.
>
> Retain the httplib import to continue using its constants, but actually make
> the http(s) connections using httplib2. The latter has built-in support for
> proxy settings, which then actually allows people behind proxies to interact
> with Gerrit.
>
> Compared to httplib, the biggest changes are:
> - There's only one Http class instead of HTTPConnection and HTTPSConnection.
> - Http.request() returns a tuple (response, contents).
> - Http.request() expects a full URI instead of just a path, as Http's
> constructor does not take a host parameter.
> - The response object inherits from dict.
> - All headers in a response are lower-cased.
>
> All in all, it is possible to see that httplib2 support was retro-fitted
> into the code, but that should not worsen its readability overall.
>
> Patch written in collaboration with Alexis Menard <alexis.menard@intel.com>.
>
> BUG=672729
> R=alexis.menard@intel.com,agable@chromium.org,tandrii@chromium.org
>
> Change-Id: Ic40e804064e74e89bc2ad979572628f1bd78c19a
> Reviewed-on: https://chromium-review.googlesource.com/458221
> Reviewed-by: Aaron Gable <agable@chromium.org>
> Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
>
TBR=agable@chromium.org,alexis.menard@intel.com,raphael.kubo.da.costa@intel.com,tandrii@chromium.org,chromium-reviews@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=672729
Change-Id: Idfacd314b381232733bb92a02ec2fb85f016effd
Reviewed-on: https://chromium-review.googlesource.com/457792
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>
Retain the httplib import to continue using its constants, but actually make
the http(s) connections using httplib2. The latter has built-in support for
proxy settings, which then actually allows people behind proxies to interact
with Gerrit.
Compared to httplib, the biggest changes are:
- There's only one Http class instead of HTTPConnection and HTTPSConnection.
- Http.request() returns a tuple (response, contents).
- Http.request() expects a full URI instead of just a path, as Http's
constructor does not take a host parameter.
- The response object inherits from dict.
- All headers in a response are lower-cased.
All in all, it is possible to see that httplib2 support was retro-fitted
into the code, but that should not worsen its readability overall.
Patch written in collaboration with Alexis Menard <alexis.menard@intel.com>.
BUG=672729
R=alexis.menard@intel.com,agable@chromium.org,tandrii@chromium.org
Change-Id: Ic40e804064e74e89bc2ad979572628f1bd78c19a
Reviewed-on: https://chromium-review.googlesource.com/458221
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
It looks like this regressed when we switched from specifying CCs
via the refspec arguments to using the API.
BUG=680605
Change-Id: Iabd397b639989f050932188b1a1aa488639ffbbe
Reviewed-on: https://chromium-review.googlesource.com/427344
Reviewed-by: Andrew Bonventre <andybons@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>
If either the edit creation or edit publishing fail, we're left with
an unpublished edit hanging around. This can result in a follow-up
invocation of 'git cl desc' (e.g. if the user changes their mind, or
retries) will also fail. So, clean up after ourselves.
BUG=671091
Change-Id: Icfe53996acf2555b82be0695a3c86ebb8c50b936
Reviewed-on: https://chromium-review.googlesource.com/418449
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Aaron Gable <agable@chromium.org>
This is to make it easy to specify a different .gitcookies for bots.
BUG=skia:5979
Change-Id: Iaeb148d25f1c2c3d06ab5e1b155999566ef5f294
Reviewed-on: https://chromium-review.googlesource.com/413075
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Ravi Mistry <rmistry@google.com>
Strip the netrc hack; apparently it only works on some versions of
Python, and the bots have many different versions.
Instead, we will actually emit the modified netrc as a new file without
comments or newlines, then read that directly from the vanilla netrc
module.
BUG=chromium:664664
TEST=None
Change-Id: I245759e2eae4e6ddcdffa0aaef59866a62bd0240
Reviewed-on: https://chromium-review.googlesource.com/411416
Commit-Queue: Daniel Jacques <dnj@chromium.org>
Reviewed-by: Ryan Tseng <hinoka@chromium.org>
Reviewed-by: Michael Moss <mmoss@chromium.org>
Processing comments seems to trigger a bug on (some?) Windows systems
due to a negative seek. This CL runs a comment-free version of the
"netrc" file through the parser.
BUG=chromium:664664
TEST=local
- Uploaded this CL with the change.
Change-Id: I65f5d2f8c1162b3b3a13c66d8becb1cfccf27991
Reviewed-on: https://chromium-review.googlesource.com/411455
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Michael Moss <mmoss@chromium.org>
Commit-Queue: Daniel Jacques <dnj@chromium.org>
The path always starts with a / so we need to check if it starts with /a/ to see if it's already authorized, not a/.
BUG=664664
Change-Id: I2bf6abe36dd88bc7d096e67d5e7a56fe9f143f47
Reviewed-on: https://chromium-review.googlesource.com/410721
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: First Last <smut@chromium.org>
Sylvain Defresne
Michael Moss
Andrii Shyshkalov
Sergio Villar Senin
Aaron Gable
Jonathan Nieder
Nick Carter
smut
Sergey Berezin
Michael Achenbach
Wiktor Garbacz
dimu
Raphael Kubo da Costa
Ravi Mistry
Dan Jacques