CQ: handle more HTTP-related errors

CQ: handle more HTTP-related errors

BUG=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=228544

[Paweł Hajdan Jr., 2013-10-11 17:57:24]

Issac, could you also review this one? I just realized Robbie is OOO.

[Isaac (away), 2013-10-11 19:27:34]

generally looks good but question about 403

https://codereview.chromium.org/26951002/diff/1/commit-queue/pending_manager.py
File commit-queue/pending_manager.py (right):

https://codereview.chromium.org/26951002/diff/1/commit-queue/pending_manager....
commit-queue/pending_manager.py:420: # This means that either the codereview
issue is closed,
403 is generic forbidden -- I assume we could get a 403 for other reasons too?

[Paweł Hajdan Jr., 2013-10-11 21:42:11]

https://codereview.chromium.org/26951002/diff/1/commit-queue/pending_manager.py
File commit-queue/pending_manager.py (right):

https://codereview.chromium.org/26951002/diff/1/commit-queue/pending_manager....
commit-queue/pending_manager.py:420: # This means that either the codereview
issue is closed,
On 2013/10/11 19:27:34, Isaac wrote:
> 403 is generic forbidden -- I assume we could get a 403 for other reasons too?

Possibly. Note that here we're just unchecking the commit checkbox.

I think logging is fine - do you have a good recommendation of something else to
do?

[Isaac (use chromium), 2013-10-11 21:50:05]

On 2013/10/11 21:42:11, Paweł Hajdan Jr. wrote:
>
https://codereview.chromium.org/26951002/diff/1/commit-queue/pending_manager.py
> File commit-queue/pending_manager.py (right):
> 
>
https://codereview.chromium.org/26951002/diff/1/commit-queue/pending_manager....
> commit-queue/pending_manager.py:420: # This means that either the codereview
> issue is closed,
> On 2013/10/11 19:27:34, Isaac wrote:
> > 403 is generic forbidden -- I assume we could get a 403 for other reasons
too?
> 
> Possibly. Note that here we're just unchecking the commit checkbox.
> 
> I think logging is fine - do you have a good recommendation of something else
to
> do?

I wonder if we could change rietveld to return a more precise error code here,
or something that allows us to deduce the reason for the 403.

[Paweł Hajdan Jr., 2013-10-14 18:52:37]

On 2013/10/11 21:50:05, Isaac (use chromium) wrote:
> I wonder if we could change rietveld to return a more precise error code here,
> or something that allows us to deduce the reason for the 403.

Added a TODO.

Since we're just trying to uncheck the checkbox, I think it's fine for now.

We can only fix CQ issues one by one, not all at once. :) I know it's not
perfect, but I think it'sd better than swallowing exceptions.

PTAL, and we can chat over IM if you have more questions. I agree it's not ideal
in the long run.

[Isaac (away), 2013-10-14 21:37:44]

patchset 3 lgtm

[commit-bot: I haz the power, 2013-10-14 21:57:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/phajdan.jr@chromium.org/26951002/12001

[commit-bot: I haz the power, 2013-10-14 21:58:03]

Change committed as 228544

[cmp, 2013-11-02 19:54:22]

On 2013/10/14 21:58:03, I haz the power (commit-bot) wrote:
> Change committed as 228544

Hey Pawel, there was no bug on file for fixing this issue when this CL landed,
but there should have been to make it easier to track the problem.  It isn't
fixed completely.  I filed
https://code.google.com/p/chromium/issues/detail?id=314388.  Let's ensure that
future CLs on this issue cite that bug.