bot_update (+recipe_module): treat patch download as infra failure.

bot_update (+recipe_module): treat patch download as infra failure.

Depends on depot_tools change https://codereview.chromium.org/1380093003 .

R=phajdan.jr@chromium.org,sergiyb@chromium.org
BUG=537417
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=296984

[tandrii(chromium), 2015-10-02 11:34:39]

PTAL

https://codereview.chromium.org/1382203002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/bot_update/api.py (right):

https://codereview.chromium.org/1382203002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/bot_update/api.py:238: # TODO(tandrii):
self.m.tryserver.set_infra_failure() ?
what should I put here?

[Sergiy Byelozyorov, 2015-10-02 11:50:25]

https://codereview.chromium.org/1382203002/diff/1/scripts/slave/bot_update.py
File scripts/slave/bot_update.py (right):

https://codereview.chromium.org/1382203002/diff/1/scripts/slave/bot_update.py...
scripts/slave/bot_update.py:1691: if e.code == 89:
According to changes in other patch - change this to 2, but leave 89 below as
is.

https://codereview.chromium.org/1382203002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/bot_update/api.py (right):

https://codereview.chromium.org/1382203002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/bot_update/api.py:238: # TODO(tandrii):
self.m.tryserver.set_infra_failure() ?
On 2015/10/02 11:34:39, tandrii(chromium) wrote:
> what should I put here?

raise self.m.step.InfraFailure('Failed to download the patch')

[tandrii(chromium), 2015-10-02 12:38:25]

https://codereview.chromium.org/1382203002/diff/1/scripts/slave/bot_update.py
File scripts/slave/bot_update.py (right):

https://codereview.chromium.org/1382203002/diff/1/scripts/slave/bot_update.py...
scripts/slave/bot_update.py:1691: if e.code == 89:
On 2015/10/02 11:50:25, Sergiy Byelozyorov wrote:
> According to changes in other patch - change this to 2, but leave 89 below as
> is.

Done.

https://codereview.chromium.org/1382203002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/bot_update/api.py (right):

https://codereview.chromium.org/1382203002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/bot_update/api.py:238: # TODO(tandrii):
self.m.tryserver.set_infra_failure() ?
On 2015/10/02 11:50:25, Sergiy Byelozyorov wrote:
> On 2015/10/02 11:34:39, tandrii(chromium) wrote:
> > what should I put here?
> 
> raise self.m.step.InfraFailure('Failed to download the patch')

Done.

[tandrii(chromium), 2015-10-02 12:39:16]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-02 12:39:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1382203002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1382203002/20001

[commit-bot: I haz the power, 2015-10-02 12:43:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-02 12:43:17]

Dry run: This issue passed the CQ dry run.

[Sergiy Byelozyorov, 2015-10-02 12:44:35]

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/bot_updat...
File scripts/slave/bot_update.py (right):

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/bot_updat...
scripts/slave/bot_update.py:1693: return 3
change back to 89

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/bot_update/api.py (right):

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bot_update/api.py:212: self(name, cmd,
step_test_data=step_test_data, ok_ret=(0, 88), **kwargs)
add 89 here

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/recipe_mo...
File
scripts/slave/recipe_modules/bot_update/example.expected/tryjob_fail_patch_download.json
(right):

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bot_update/example.expected/tryjob_fail_patch_download.json:49:
"@@@STEP_EXCEPTION@@@"
I don't see INFRA_FAILURE anywhere in this expectations file...

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/bot_update/test_api.py (right):

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bot_update/test_api.py:75: output['return_code'] =
1
if fail_patch == 'apply':
  output['return_code'] = 1
elif fail_patch == 'download':
  output['return_code'] = 3
else:
  output['return_code'] = 0

[tandrii(chromium), 2015-10-02 12:50:36]

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/bot_updat...
File scripts/slave/bot_update.py (right):

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/bot_updat...
scripts/slave/bot_update.py:1693: return 3
On 2015/10/02 12:44:35, Sergiy Byelozyorov wrote:
> change back to 89

made it 87 as download happens before apply :)

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/bot_update/api.py (right):

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bot_update/api.py:212: self(name, cmd,
step_test_data=step_test_data, ok_ret=(0, 88), **kwargs)
On 2015/10/02 12:44:35, Sergiy Byelozyorov wrote:
> add 89 here

Done.

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/recipe_mo...
File
scripts/slave/recipe_modules/bot_update/example.expected/tryjob_fail_patch.json
(right):

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bot_update/example.expected/tryjob_fail_patch.json:41:
"@@@STEP_LOG_LINE@json.output@  \"return_code\": 1, @@@",
normal error code.

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/recipe_mo...
File
scripts/slave/recipe_modules/bot_update/example.expected/tryjob_fail_patch_download.json
(right):

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bot_update/example.expected/tryjob_fail_patch_download.json:42:
"@@@STEP_LOG_LINE@json.output@  \"return_code\": 3, @@@",
This is THE important change.

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bot_update/example.expected/tryjob_fail_patch_download.json:49:
"@@@STEP_EXCEPTION@@@"
On 2015/10/02 12:44:35, Sergiy Byelozyorov wrote:
> I don't see INFRA_FAILURE anywhere in this expectations file...

bad.. let's see...

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/bot_update/test_api.py (right):

https://codereview.chromium.org/1382203002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bot_update/test_api.py:75: output['return_code'] =
1
On 2015/10/02 12:44:35, Sergiy Byelozyorov wrote:
> if fail_patch == 'apply':
>   output['return_code'] = 1
> elif fail_patch == 'download':
>   output['return_code'] = 3
> else:
>   output['return_code'] = 0

If it's 0 OR if apply_issue isn't run at all, it's not set in bot_update. So, i
left as is.

[tandrii(chromium), 2015-10-02 12:59:52]

And I finally raise!

https://codereview.chromium.org/1382203002/diff/40001/scripts/slave/recipe_mo...
File
scripts/slave/recipe_modules/bot_update/example.expected/tryjob_fail_patch_download.json
(right):

https://codereview.chromium.org/1382203002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bot_update/example.expected/tryjob_fail_patch_download.json:29:
"step returned non-zero exit code: 3",
where does this come from?

[tandrii(chromium), 2015-10-02 13:03:39]

ok, this looks almost good to me.

[Sergiy Byelozyorov, 2015-10-02 13:06:36]

https://codereview.chromium.org/1382203002/diff/80001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/bot_update/api.py (right):

https://codereview.chromium.org/1382203002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bot_update/api.py:238: if return_code == 3:
now you also need to change 3 to 87

[Sergiy Byelozyorov, 2015-10-02 13:37:51]

https://codereview.chromium.org/1382203002/diff/80001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/bot_update/api.py (right):

https://codereview.chromium.org/1382203002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bot_update/api.py:212: # apply, respectively.
add a comment that we do not check them here, but instead parse JSON below and
that the codes are for cases when someone needs to run bot_update manually from
command line

https://codereview.chromium.org/1382203002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bot_update/api.py:238: if return_code == 3:
On 2015/10/02 13:06:35, Sergiy Byelozyorov wrote:
> now you also need to change 3 to 87

discussed offline... this is actually correct

[tandrii(chromium), 2015-10-02 13:49:44]

PTAL

https://codereview.chromium.org/1382203002/diff/80001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/bot_update/api.py (right):

https://codereview.chromium.org/1382203002/diff/80001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bot_update/api.py:212: # apply, respectively.
On 2015/10/02 13:37:50, Sergiy Byelozyorov wrote:
> add a comment that we do not check them here, but instead parse JSON below and
> that the codes are for cases when someone needs to run bot_update manually
from
> command line

Done.

[Sergiy Byelozyorov, 2015-10-02 13:53:20]

lgtm w/ comment

https://codereview.chromium.org/1382203002/diff/10003/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/bot_update/api.py (right):

https://codereview.chromium.org/1382203002/diff/10003/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bot_update/api.py:245: 'print "Check the bot_update
step for details";'
please change the message to explain that download of the patch failed

[tandrii(chromium), 2015-10-02 13:57:15]

https://codereview.chromium.org/1382203002/diff/10003/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/bot_update/api.py (right):

https://codereview.chromium.org/1382203002/diff/10003/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/bot_update/api.py:245: 'print "Check the bot_update
step for details";'
On 2015/10/02 13:53:20, Sergiy Byelozyorov wrote:
> please change the message to explain that download of the patch failed

Done.

[tandrii(chromium), 2015-10-02 14:05:57]

The CQ bit was checked by tandrii@chromium.org

[tandrii(chromium), 2015-10-02 14:05:57]

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/1382203002/#ps150001
(title: "NBits\")

[commit-bot: I haz the power, 2015-10-02 14:06:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1382203002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1382203002/150001

[commit-bot: I haz the power, 2015-10-02 14:09:59]

Committed patchset #9 (id:150001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=296984