Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1373363006:
apply_issue: cleanup Rietveld patch download failure. (Closed)
|
Created:
5 years, 2 months ago by tandrii(chromium) Modified:
5 years, 2 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org, pgervais Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
Descriptionapply_issue: cleanup Rietveld patch download failure.
BUG=537417
R=sergiyb@chromium.org,phajdan.jr@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=297012
Patch Set 1 #Patch Set 2 : +TODO #
Total comments: 6
Patch Set 3 : nit #
Total comments: 2
Messages
Total messages: 17 (6 generated)
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/1373363006/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1373363006/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tandrii@chromium.org changed reviewers: + sergiyb@chromium.org - pgervais@chromium.org
PTAL +Pawel and Sergey because they are EMEA,
lgtm w/ comments https://codereview.chromium.org/1373363006/diff/20001/apply_issue.py File apply_issue.py (right): https://codereview.chromium.org/1373363006/diff/20001/apply_issue.py#newcode101 apply_issue.py:101: # TODO(pgervais,tandrii): split this func, it's still too long. lovely :-) https://codereview.chromium.org/1373363006/diff/20001/apply_issue.py#newcode150 apply_issue.py:150: logging.exception('failed to fetch issue properties') please also log exception, here and everywhere below https://codereview.chromium.org/1373363006/diff/20001/apply_issue.py#newcode214 apply_issue.py:214: logging.exception('failed to fetch iss') iss?
https://codereview.chromium.org/1373363006/diff/20001/apply_issue.py File apply_issue.py (right): https://codereview.chromium.org/1373363006/diff/20001/apply_issue.py#newcode150 apply_issue.py:150: logging.exception('failed to fetch issue properties') On 2015/10/05 14:18:13, Sergiy Byelozyorov wrote: > please also log exception, here and everywhere below https://docs.python.org/2/library/logging.html#logging.Logger.exception https://codereview.chromium.org/1373363006/diff/20001/apply_issue.py#newcode214 apply_issue.py:214: logging.exception('failed to fetch iss') On 2015/10/05 14:18:13, Sergiy Byelozyorov wrote: > iss? Done.
https://codereview.chromium.org/1373363006/diff/20001/apply_issue.py File apply_issue.py (right): https://codereview.chromium.org/1373363006/diff/20001/apply_issue.py#newcode150 apply_issue.py:150: logging.exception('failed to fetch issue properties') On 2015/10/05 14:47:00, tandrii(chromium) wrote: > On 2015/10/05 14:18:13, Sergiy Byelozyorov wrote: > > please also log exception, here and everywhere below > > https://docs.python.org/2/library/logging.html#logging.Logger.exception Acknowledged.
The CQ bit was checked by tandrii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergiyb@chromium.org Link to the patchset: https://codereview.chromium.org/1373363006/#ps40001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1373363006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1373363006/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=297012
Message was sent while issue was closed.
pgervais@chromium.org changed reviewers: + pgervais@chromium.org
Message was sent while issue was closed.
Thanks for fixing that. Added one comment. https://codereview.chromium.org/1373363006/diff/40001/apply_issue.py File apply_issue.py (right): https://codereview.chromium.org/1373363006/diff/40001/apply_issue.py#newcode32 apply_issue.py:32: RETURN_CODE_OTHERWISE = 1 # any other failure, likely patch apply one. Nit: RETURN_CODE_FAILURE_OTHER ? 'RETURN_CODE_OTHERWISE' in isolation doesn't help very much. Is it a failure or a success? Same for ARG_PARSER. I would add 'failure' somewhere in the name.
Message was sent while issue was closed.
On 2015/10/05 16:13:51, pgervais wrote: > Thanks for fixing that. Added one comment. > > https://codereview.chromium.org/1373363006/diff/40001/apply_issue.py > File apply_issue.py (right): > > https://codereview.chromium.org/1373363006/diff/40001/apply_issue.py#newcode32 > apply_issue.py:32: RETURN_CODE_OTHERWISE = 1 # any other failure, likely > patch apply one. > Nit: RETURN_CODE_FAILURE_OTHER ? > > 'RETURN_CODE_OTHERWISE' in isolation doesn't help very much. Is it a failure or > a success? > > Same for ARG_PARSER. I would add 'failure' somewhere in the name. Thanks for comments, Philippe! Creating a CL...
Message was sent while issue was closed.
As promised: https://codereview.chromium.org/1372133004 https://codereview.chromium.org/1373363006/diff/40001/apply_issue.py File apply_issue.py (right): https://codereview.chromium.org/1373363006/diff/40001/apply_issue.py#newcode32 apply_issue.py:32: RETURN_CODE_OTHERWISE = 1 # any other failure, likely patch apply one. On 2015/10/05 16:13:51, pgervais wrote: > Nit: RETURN_CODE_FAILURE_OTHER ? > > 'RETURN_CODE_OTHERWISE' in isolation doesn't help very much. Is it a failure or > a success? > > Same for ARG_PARSER. I would add 'failure' somewhere in the name. Done. |
