Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1164363003:
Add script for triggering Buildbucket builds (Closed)
|
Created:
5 years, 6 months ago by smut Modified:
5 years, 6 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org, sheyang Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionAdd script for triggering Buildbucket builds
BUG=493885
TESTED=See https://paste.googleplex.com/5622248052359168
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=295569
R=nodir@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=295570
Patch Set 1 #
Total comments: 16
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #Patch Set 4 : Add -b as alias for --bucket #Messages
Total messages: 15 (5 generated)
smut@google.com changed reviewers: + nodir@chromium.org
+sheyang, since he implemented buildbucket support in git-cl-try https://codereview.chromium.org/1164363003/diff/1/buildbucket.py File buildbucket.py (right): https://codereview.chromium.org/1164363003/diff/1/buildbucket.py#newcode36 buildbucket.py:36: parser = argparse.ArgumentParser() Please make it buildbucket put --bucket ..... instead of buildbucket --bucket ..... https://codereview.chromium.org/1164363003/diff/1/buildbucket.py#newcode63 buildbucket.py:63: 'reason': os.path.basename(argv[0]), "buildbucket.py" is not a good category or reason. I think it is better not to specify anything than do this https://codereview.chromium.org/1164363003/diff/1/buildbucket.py#newcode67 buildbucket.py:67: properties.update(json.load(fp)) Add exception handling with meaningful explanation that the properties file is malformed https://codereview.chromium.org/1164363003/diff/1/buildbucket.py#newcode91 buildbucket.py:91: return response.status != 200 It should return an int, not boolean, no?
sheyang@chromium.org changed reviewers: + sheyang@chromium.org
https://codereview.chromium.org/1164363003/diff/1/buildbucket.py File buildbucket.py (right): https://codereview.chromium.org/1164363003/diff/1/buildbucket.py#newcode6 buildbucket.py:6: """Tool for scheduling builds on Buildbucket. I guess there'll be a follow-up CL to make "git cl try" use this module? https://codereview.chromium.org/1164363003/diff/1/buildbucket.py#newcode12 buildbucket.py:12: --bucket master.chromium.tryserver.linux \ master.tryserver.chromium.linux https://codereview.chromium.org/1164363003/diff/1/buildbucket.py#newcode38 buildbucket.py:38: '-m', The combination ('-m' and '--bucket') seems strange. https://codereview.chromium.org/1164363003/diff/1/buildbucket.py#newcode40 buildbucket.py:40: help='The bucket to schedule the build on. Typically the master name.', I would give an example such as 'master.tryserver.chromium.linux' here so people wouldn't input 'tryserver.chromium.linux' as they normally do.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/1164363003/diff/1/buildbucket.py File buildbucket.py (right): https://codereview.chromium.org/1164363003/diff/1/buildbucket.py#newcode6 buildbucket.py:6: """Tool for scheduling builds on Buildbucket. On 2015/06/08 22:02:13, sheyang wrote: > I guess there'll be a follow-up CL to make "git cl try" use this module? Potentially. https://codereview.chromium.org/1164363003/diff/1/buildbucket.py#newcode12 buildbucket.py:12: --bucket master.chromium.tryserver.linux \ On 2015/06/08 22:02:13, sheyang wrote: > master.tryserver.chromium.linux Done. https://codereview.chromium.org/1164363003/diff/1/buildbucket.py#newcode36 buildbucket.py:36: parser = argparse.ArgumentParser() On 2015/06/08 21:46:15, nodir wrote: > Please make it > buildbucket put --bucket ..... > instead of > buildbucket --bucket ..... Done. https://codereview.chromium.org/1164363003/diff/1/buildbucket.py#newcode38 buildbucket.py:38: '-m', On 2015/06/08 22:02:13, sheyang wrote: > The combination ('-m' and '--bucket') seems strange. -m for master because -b was taken. added --master-name alias. https://codereview.chromium.org/1164363003/diff/1/buildbucket.py#newcode40 buildbucket.py:40: help='The bucket to schedule the build on. Typically the master name.', On 2015/06/08 22:02:13, sheyang wrote: > I would give an example such as 'master.tryserver.chromium.linux' here so people > wouldn't input 'tryserver.chromium.linux' as they normally do. Done. https://codereview.chromium.org/1164363003/diff/1/buildbucket.py#newcode63 buildbucket.py:63: 'reason': os.path.basename(argv[0]), On 2015/06/08 21:46:15, nodir wrote: > "buildbucket.py" is not a good category or reason. I think it is better not to > specify anything than do this I've seen "CQ" as the category and reason, so I thought at least one of those properties referred to the source of the build. I will get rid of this. https://codereview.chromium.org/1164363003/diff/1/buildbucket.py#newcode67 buildbucket.py:67: properties.update(json.load(fp)) On 2015/06/08 21:46:15, nodir wrote: > Add exception handling with meaningful explanation that the properties file is > malformed Exception is pretty clear: ValueError: No JSON object could be decoded Added stderr output to be explicit. https://codereview.chromium.org/1164363003/diff/1/buildbucket.py#newcode91 buildbucket.py:91: return response.status != 200 On 2015/06/08 21:46:15, nodir wrote: > It should return an int, not boolean, no? Doesn't really matter. int(False) is 0, int(True) is 1.
https://codereview.chromium.org/1164363003/diff/40001/buildbucket.py File buildbucket.py (right): https://codereview.chromium.org/1164363003/diff/40001/buildbucket.py#newcode54 buildbucket.py:54: '--master-name', Let's not tie it to buildbot. Bucket name will be always required to put a build. Remove the option altogether? buildbucket.py put master.tryserver.chromium.linux ... https://codereview.chromium.org/1164363003/diff/40001/buildbucket.py#newcode75 buildbucket.py:75: except ValueError: Do except (ValueError, TypeError) because dict.update raises TypeError if non-dict is passed
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/1164363003/diff/40001/buildbucket.py File buildbucket.py (right): https://codereview.chromium.org/1164363003/diff/40001/buildbucket.py#newcode54 buildbucket.py:54: '--master-name', On 2015/06/08 23:15:32, nodir wrote: > Let's not tie it to buildbot. > > Bucket name will be always required to put a build. Remove the option > altogether? > > buildbucket.py put master.tryserver.chromium.linux ... I prefer explicit --flags rather than positional arguments. That way you always know what you're passing a value for, and you can do it in any order. I changed this to -b/--bucket and -n/--builder-name. https://codereview.chromium.org/1164363003/diff/40001/buildbucket.py#newcode75 buildbucket.py:75: except ValueError: On 2015/06/08 23:15:32, nodir wrote: > Do > except (ValueError, TypeError) > > because dict.update raises TypeError if non-dict is passed Done.
lgtm
The CQ bit was checked by smut@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1164363003/80001
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=295569
Message was sent while issue was closed.
Committed patchset #4 (id:100001) manually as 295570. |
