git cl try --luci, a set of hacks to demonstrate and iterate LUCI

git cl try --luci, a set of hacks to demonstrate and iterate LUCI



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

[Ryan Tseng, 2015-09-15 21:33:40]

hinoka@google.com changed reviewers:
+ dnj@chromium.org, estaab@chromium.org, hinoka@google.com

[Ryan Tseng, 2015-09-15 21:33:41]

As requested, hacky luci

[estaab, 2015-09-15 23:05:14]

Nice, here are some quick comments. At a high level try to make the keepable
parts robust with good error handling and messages and keep the rest somewhat
hacky. Thanks!

https://codereview.chromium.org/1344183002/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/1344183002/diff/1/git_cl.py#newcode240
git_cl.py:240: for _, builders_and_tests in sorted(masters.iteritems()):
itervalues()

https://codereview.chromium.org/1344183002/diff/1/git_cl.py#newcode241
git_cl.py:241: for builder, _ in sorted(builders_and_tests.iteritems()):
iterkeys()

https://codereview.chromium.org/1344183002/diff/1/luci_hacks/luci_recipe_run.py
File luci_hacks/luci_recipe_run.py (right):

https://codereview.chromium.org/1344183002/diff/1/luci_hacks/luci_recipe_run....
luci_hacks/luci_recipe_run.py:16: import tempfile
You're not using this and probably should :)  Can you clean up all temp files
created by this tool, both in the success and error cases?

https://codereview.chromium.org/1344183002/diff/1/luci_hacks/luci_recipe_run....
luci_hacks/luci_recipe_run.py:61: download(dt_url, os.path.join(dt_zip))
Can you handle errors for the parts we'll likely keep when we transition to DM?

https://codereview.chromium.org/1344183002/diff/1/luci_hacks/trigger_luci_job.py
File luci_hacks/trigger_luci_job.py (right):

https://codereview.chromium.org/1344183002/diff/1/luci_hacks/trigger_luci_job...
luci_hacks/trigger_luci_job.py:64: def archive_isolate(ISOLATE):
lowercase here (and within this func), we're already passing in the uppercase
one as the value

https://codereview.chromium.org/1344183002/diff/1/luci_hacks/trigger_luci_job...
luci_hacks/trigger_luci_job.py:69: '--isolate-server',
'https://isolateserver.appspot.com',
make constant at top of file (and for URLs below)

https://codereview.chromium.org/1344183002/diff/1/luci_hacks/trigger_luci_job...
luci_hacks/trigger_luci_job.py:90: m = re.search(
if not m:
  logging.error("Couldn't find task id in swarming output: %s", out)

https://codereview.chromium.org/1344183002/diff/1/luci_hacks/trigger_luci_job...
luci_hacks/trigger_luci_job.py:97: master, recipe, platform =
RECIPE_MAPPING[builder]
.get(builder) and check for None and log a helpful error if so :)

https://codereview.chromium.org/1344183002/diff/1/luci_hacks/trigger_luci_job...
luci_hacks/trigger_luci_job.py:117: print
'https://luci-milo.appspot.com/swarming/%s' % task
Nice.

[Ryan Tseng, 2015-09-17 23:39:23]

ptal!

https://codereview.chromium.org/1344183002/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/1344183002/diff/1/git_cl.py#newcode240
git_cl.py:240: for _, builders_and_tests in sorted(masters.iteritems()):
On 2015/09/15 23:05:14, estaab wrote:
> itervalues()

Done.

https://codereview.chromium.org/1344183002/diff/1/git_cl.py#newcode241
git_cl.py:241: for builder, _ in sorted(builders_and_tests.iteritems()):
On 2015/09/15 23:05:14, estaab wrote:
> iterkeys()

Done.

https://codereview.chromium.org/1344183002/diff/1/luci_hacks/luci_recipe_run.py
File luci_hacks/luci_recipe_run.py (right):

https://codereview.chromium.org/1344183002/diff/1/luci_hacks/luci_recipe_run....
luci_hacks/luci_recipe_run.py:16: import tempfile
On 2015/09/15 23:05:14, estaab wrote:
> You're not using this and probably should :)  Can you clean up all temp files
> created by this tool, both in the success and error cases?

Actually swarming does it all for me if I leave all the files under cwd (yay).

https://codereview.chromium.org/1344183002/diff/1/luci_hacks/luci_recipe_run....
luci_hacks/luci_recipe_run.py:61: download(dt_url, os.path.join(dt_zip))
On 2015/09/15 23:05:14, estaab wrote:
> Can you handle errors for the parts we'll likely keep when we transition to
DM?

What do you mean? Like add retry logic?

This'll run on a bot so it seems reasonable to me for the errors to be
regurgitating stack traces instead of emitting smooth error messages

[estaab, 2015-09-24 02:06:28]

lgtm

Sorry, I missed the last patch set. LG

[hinoka, 2015-09-25 18:27:41]

The CQ bit was checked by hinoka@chromium.org

[commit-bot: I haz the power, 2015-09-25 18:27:56]

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

[commit-bot: I haz the power, 2015-09-25 18:36:31]

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

[commit-bot: I haz the power, 2015-09-25 18:36:32]

Try jobs failed on following builders:
  depot_tools_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/depot_tools_pre...)

[hinoka, 2015-09-25 19:07:13]

The CQ bit was checked by hinoka@chromium.org

[hinoka, 2015-09-25 19:07:13]

The patchset sent to the CQ was uploaded after l-g-t-m from estaab@chromium.org
Link to the patchset: https://codereview.chromium.org/1344183002/#ps40001
(title: "Lint")

[commit-bot: I haz the power, 2015-09-25 19:07:34]

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

[commit-bot: I haz the power, 2015-09-25 19:11:12]

Committed patchset #3 (id:40001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=296885

[MatsOphauG, 2015-09-28 12:42:06]

matsophaug@gmail.com changed reviewers:
+ MatsOphaug@gmail.com