Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2274743003:
Add a --json option to git cl try-results. (Closed)
|
Created:
4 years, 4 months ago by qyearsley Modified:
4 years, 3 months ago CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org, tandrii+omg_git_cl_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
DescriptionAdd a --json option to git cl try-results.
For use in scripts that check try job results, this is
currently practically the same as making requests to
https://codereview.chromium.org/api/<cl-number>/<patchset-number>;.
However, I think that this might be more future-proof, since
it fetches try job information from Buildbucket, and I expect
to still work after migration from Rietveld to Gerrit.
BUG=640354
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/53f48a1c52f7d35daa160894585dffe8aafe231d
Patch Set 1 #
Total comments: 5
Patch Set 2 : WIP: Make --json option output the same info as human-readable option; refactor #
Total comments: 2
Patch Set 3 : Make --json option write to file and output pared-down version of buildbucket builds list #
Total comments: 10
Patch Set 4 : Add url and bucket, update test #Messages
Total messages: 34 (8 generated)
qyearsley@chromium.org changed reviewers: + tandrii@chromium.org
Andrii, in http://crbug.com/639533#c2 you suggested perhaps making a --json option which takes a file name and outputs to a file; I wanted to ask, what is the advantage of outputting to a file rather than stdout?
On 2016/08/23 22:15:17, qyearsley wrote: > Andrii, in http://crbug.com/639533#c2 you suggested perhaps making a --json > option which takes a file name and outputs to a file; I wanted to ask, what is > the advantage of outputting to a file rather than stdout? It's much easier to ensure that only your json goes into the file you open than stdout into which anything can write. Basically, logging and warning messages don't pollute your outout.
your description is correct, don't check rietveld directly.
tandrii@chromium.org changed reviewers: + iannucci@chromium.org, nodir@chromium.org
https://codereview.chromium.org/2274743003/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2274743003/diff/1/git_cl.py#newcode4761 git_cl.py:4761: "--json", action='store_true', default=False, no need for default. https://codereview.chromium.org/2274743003/diff/1/git_cl.py#newcode4762 git_cl.py:4762: help="Output try job results as JSON instead of a human-readable format.") IMO, change to 'write try job results as JSON to a file' and change code appropriately. oh, wow, double quotes everywhere here :( keep as is, i'll clean it up once I make it work for Gerrit. https://codereview.chromium.org/2274743003/diff/1/git_cl.py#newcode4792 git_cl.py:4792: print(json.dumps(jobs, indent=2)) hm, you want to dump the *whole* metadata? i don't really approve. This makes us tied to buildbucket's current output. Can this instead be processed s.t. only useful its are stored to json? +nodir@/iannucci@ am I unreasonable? and in any case, this warrants a test because machines will now rely on this.
Hi Andrii, I've now started changing this CL so that it makes the --json option output essentially the same information as the human-readable option (grouping the builds into categories and keeping a subset of the information that buildbucket returns). What do you think about this approach? Does this look like the right direction? Robbie and Nodir, do you have any suggestions? https://codereview.chromium.org/2274743003/diff/1/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2274743003/diff/1/git_cl.py#newcode4761 git_cl.py:4761: "--json", action='store_true', default=False, On 2016/08/24 at 11:32:23, tandrii(chromium) wrote: > no need for default. Now changing this to be an option that takes a filename. https://codereview.chromium.org/2274743003/diff/1/git_cl.py#newcode4762 git_cl.py:4762: help="Output try job results as JSON instead of a human-readable format.") On 2016/08/24 at 11:32:23, tandrii(chromium) wrote: > IMO, change to 'write try job results as JSON to a file' and change code appropriately. > > > oh, wow, double quotes everywhere here :( keep as is, i'll clean it up once I make it work for Gerrit. Sounds good.
On 2016/08/26 01:56:47, qyearsley wrote: > Hi Andrii, I've now started changing this CL so that it makes the --json option > output essentially the same information as the human-readable option (grouping > the builds into categories and keeping a subset of the information that > buildbucket returns). > > What do you think about this approach? Does this look like the right direction? I dislike it a bit, because it ties human output to specific format, but your rational could convince me. So, what is it? > > Robbie and Nodir, do you have any suggestions? > > https://codereview.chromium.org/2274743003/diff/1/git_cl.py > File git_cl.py (right): > > https://codereview.chromium.org/2274743003/diff/1/git_cl.py#newcode4761 > git_cl.py:4761: "--json", action='store_true', default=False, > On 2016/08/24 at 11:32:23, tandrii(chromium) wrote: > > no need for default. > > Now changing this to be an option that takes a filename. Oh, yes, of course. > https://codereview.chromium.org/2274743003/diff/1/git_cl.py#newcode4762 > git_cl.py:4762: help="Output try job results as JSON instead of a human-readable > format.") > On 2016/08/24 at 11:32:23, tandrii(chromium) wrote: > > IMO, change to 'write try job results as JSON to a file' and change code > appropriately. > > > > > > oh, wow, double quotes everywhere here :( keep as is, i'll clean it up once I > make it work for Gerrit. > > Sounds good.
On 2016/08/26 14:56:52, tandrii(chromium) wrote: > On 2016/08/26 01:56:47, qyearsley wrote: > > Hi Andrii, I've now started changing this CL so that it makes the --json > option > > output essentially the same information as the human-readable option (grouping > > the builds into categories and keeping a subset of the information that > > buildbucket returns). > > > > What do you think about this approach? Does this look like the right > direction? > I dislike it a bit, because it ties human output to specific format, > but your rational could convince me. So, what is it? Specifically, what's the reason for the tool consuming the output not grouping the tryjobs by category whichever way it wants?
On 2016/08/26 at 14:57:32, tandrii wrote: > On 2016/08/26 14:56:52, tandrii(chromium) wrote: > > On 2016/08/26 01:56:47, qyearsley wrote: > > > Hi Andrii, I've now started changing this CL so that it makes the --json > > option > > > output essentially the same information as the human-readable option (grouping > > > the builds into categories and keeping a subset of the information that > > > buildbucket returns). > > > > > > What do you think about this approach? Does this look like the right > > direction? > > I dislike it a bit, because it ties human output to specific format, > > but your rational could convince me. So, what is it? > Specifically, what's the reason for the tool consuming the output not grouping the tryjobs by category whichever way it wants? Yeah, I'm not sold on the group-by-human-readable-english-description approach. I think that git_cl should spit out the raw machine-readable information, and then whatever consumes it can reformat it however it likes.
https://codereview.chromium.org/2274743003/diff/20001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/2274743003/diff/20001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1871: pass this test doesn't look like it tests anything? It would be great if we could see an example of the output JSON in this test.
On 2016/08/26 at 18:18:36, iannucci wrote: > On 2016/08/26 at 14:57:32, tandrii wrote: > > Specifically, what's the reason for the tool consuming the output not grouping the tryjobs by category whichever way it wants? > > Yeah, I'm not sold on the group-by-human-readable-english-description approach. I think that git_cl should spit out the raw machine-readable information, and then whatever consumes it can reformat it however it likes. Makes sense -- I think that Andrii noted before that outputting exactly the information from buildbucket may make it so that anything that uses `git cl try-results --json` would depend on the format that buildbucket uses. But, for my purposes I'm OK with that, and I don't expect it to cause much of an issue. Reading the information in the way that the Buildbucket API outputs it is much better than parsing the human-readable git cl try-results output, which is what I'm doing now. So, I think the next steps for this CL are: - Make it more like the first patch again, but outputting JSON to a file instead of stdout - Add a working test https://codereview.chromium.org/2274743003/diff/20001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/2274743003/diff/20001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1871: pass On 2016/08/26 at 18:21:13, iannucci wrote: > this test doesn't look like it tests anything? It would be great if we could see an example of the output JSON in this test. Yeah, the test isn't written properly yet, I uploaded a patch before it was finished :-/
On 2016/08/26 18:32:13, qyearsley wrote: > On 2016/08/26 at 18:18:36, iannucci wrote: > > On 2016/08/26 at 14:57:32, tandrii wrote: > > > Specifically, what's the reason for the tool consuming the output not > grouping the tryjobs by category whichever way it wants? > > > > Yeah, I'm not sold on the group-by-human-readable-english-description > approach. I think that git_cl should spit out the raw machine-readable > information, and then whatever consumes it can reformat it however it likes. > > Makes sense -- I think that Andrii noted before that outputting exactly the > information from buildbucket may make it so that anything that uses `git cl > try-results --json` would depend on the format that buildbucket uses. So, I don't like people "exploiting" buildbucket, because we might one day migrate to DM bypassing buildbucket. But, IMO, this is still better being tied to user-friendly output. > But, for my purposes I'm OK with that, and I don't expect it to cause much of an > issue. Reading the information in the way that the Buildbucket API outputs it is > much better than parsing the human-readable git cl try-results output, which is > what I'm doing now. > > So, I think the next steps for this CL are: > - Make it more like the first patch again, but outputting JSON to a file > instead of stdout > - Add a working test SGTM. Sorry for making you do redundant work. I should have been clearer: I'd pop all the keys from buildbucket return that your tool doesn't need. There is also perhaps some value in post-processing some json properties encoded as json inside json, but we can leave it for later time.
This CL has now been updated and is ready to be reviewed again :-) https://codereview.chromium.org/2274743003/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2274743003/diff/40001/git_cl.py#newcode550 git_cl.py:550: 'failure_reason': build.get('failure_reason'), Does it seem like a good idea to use build['key'] instead for "id" and "status" and "parameters_json", and raise a KeyError and abort if the Buildbucket builds list doesn't match what we expect? I'm thinking here that whatever consumes this JSON should check for nulls, and handle that, and if at some point we discover that builder_name or status is null, then that indicates that maybe the buildbucket response isn't as expected and something here should be updated. https://codereview.chromium.org/2274743003/diff/40001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/2274743003/diff/40001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1895: git_cl.write_try_results_json('output.json', builds) I'm not sure whether it would be worth it to write a method that invokes the top-level command function (i.e. git_cl.main(['try-results', '--json', 'output.json ])); I imagine such a method would assert that a series of calls is made (e.g. ['git', 'symbolic-ref', 'HEAD'], ...) and then assert that write_json got called in the expected way. But, the above more specific test method already tests what the expected format should be like, and I manually verified that git cl try-results --json output.json works on my workstation.
very sgtm https://codereview.chromium.org/2274743003/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2274743003/diff/40001/git_cl.py#newcode549 git_cl.py:549: build.get('parameters_json', '{}')).get('builder_name'), what about bucket_name? (that's where you can get what we call master name in buildbot world). also, i think it's worthwhile to report category here and url. https://codereview.chromium.org/2274743003/diff/40001/git_cl.py#newcode550 git_cl.py:550: 'failure_reason': build.get('failure_reason'), On 2016/08/30 22:13:22, qyearsley wrote: > Does it seem like a good idea to use build['key'] instead for "id" and "status" > and "parameters_json", and raise a KeyError and abort if the Buildbucket builds > list doesn't match what we expect? What's build['key']? I don't see it in buildbucket docs. > > I'm thinking here that whatever consumes this JSON should check for nulls, and > handle that, and if at some point we discover that builder_name or status is > null, then that indicates that maybe the buildbucket response isn't as expected > and something here should be updated. this -> sgtm. https://codereview.chromium.org/2274743003/diff/40001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/2274743003/diff/40001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1895: git_cl.write_try_results_json('output.json', builds) On 2016/08/30 22:13:22, qyearsley wrote: > I'm not sure whether it would be worth it to write a method that invokes the > top-level command function (i.e. git_cl.main(['try-results', '--json', > 'output.json > ])); I imagine such a method would assert that a series of calls is made (e.g. > ['git', 'symbolic-ref', 'HEAD'], ...) and then assert that write_json got called > in the expected way. > > But, the above more specific test method already tests what the expected format > should be like, and I manually verified that git cl try-results --json > output.json works on my workstation. I'd keep more specific here, but it'd still have a general one for top level to be sure nobody breaks your tooling, for which you can mock git_cl.write_try_results_json since it's tested here.
https://codereview.chromium.org/2274743003/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2274743003/diff/40001/git_cl.py#newcode549 git_cl.py:549: build.get('parameters_json', '{}')).get('builder_name'), On 2016/08/30 at 22:22:45, tandrii(chromium) wrote: > what about bucket_name? (that's where you can get what we call master name in buildbot world). > > also, i think it's worthwhile to report category here and url. Added bucket and url. By category, do you mean something like the category printed with `git cl try-results`, which is generally a combination of "status", "result", and "failure_reason"? https://codereview.chromium.org/2274743003/diff/40001/git_cl.py#newcode550 git_cl.py:550: 'failure_reason': build.get('failure_reason'), On 2016/08/30 at 22:22:45, tandrii(chromium) wrote: > On 2016/08/30 22:13:22, qyearsley wrote: > > Does it seem like a good idea to use build['key'] instead for "id" and "status" > > and "parameters_json", and raise a KeyError and abort if the Buildbucket builds > > list doesn't match what we expect? > What's build['key']? I don't see it in buildbucket docs. > Ah, that's not a real thing, what I meant was "does it seem like a good idea to use the square bracket operator (__getitem__) which raises a KeyError, instead of dict.get?". Sounds like dict.get is a reasonable way to do it? https://codereview.chromium.org/2274743003/diff/40001/tests/git_cl_test.py File tests/git_cl_test.py (right): https://codereview.chromium.org/2274743003/diff/40001/tests/git_cl_test.py#ne... tests/git_cl_test.py:1895: git_cl.write_try_results_json('output.json', builds) On 2016/08/30 at 22:22:45, tandrii(chromium) wrote: > On 2016/08/30 22:13:22, qyearsley wrote: > > I'm not sure whether it would be worth it to write a method that invokes the > > top-level command function (i.e. git_cl.main(['try-results', '--json', > > 'output.json > > ])); I imagine such a method would assert that a series of calls is made (e.g. > > ['git', 'symbolic-ref', 'HEAD'], ...) and then assert that write_json got called > > in the expected way. > > > > But, the above more specific test method already tests what the expected format > > should be like, and I manually verified that git cl try-results --json > > output.json works on my workstation. > > I'd keep more specific here, but it'd still have a general one for top level to be sure nobody breaks your tooling, for which you can mock git_cl.write_try_results_json since it's tested here. Alright, sounds reasonable ^^
I don't want to hold you any longer. Code as is => LGTM, but I stand by suggestion to add simple test to avoid breakage. https://codereview.chromium.org/2274743003/diff/40001/git_cl.py File git_cl.py (right): https://codereview.chromium.org/2274743003/diff/40001/git_cl.py#newcode549 git_cl.py:549: build.get('parameters_json', '{}')).get('builder_name'), On 2016/08/30 23:41:24, qyearsley wrote: > On 2016/08/30 at 22:22:45, tandrii(chromium) wrote: > > what about bucket_name? (that's where you can get what we call master name in > buildbot world). > > > > also, i think it's worthwhile to report category here and url. > > Added bucket and url. > > By category, do you mean something like the category printed with `git cl > try-results`, which is generally a combination of "status", "result", and > "failure_reason"? no, i meant parameters_json.category. It's usually CQ or "git cl" or "experimental". That said, doesn't have to be in this CL. https://codereview.chromium.org/2274743003/diff/40001/git_cl.py#newcode550 git_cl.py:550: 'failure_reason': build.get('failure_reason'), On 2016/08/30 23:41:25, qyearsley wrote: > On 2016/08/30 at 22:22:45, tandrii(chromium) wrote: > > On 2016/08/30 22:13:22, qyearsley wrote: > > > Does it seem like a good idea to use build['key'] instead for "id" and > "status" > > > and "parameters_json", and raise a KeyError and abort if the Buildbucket > builds > > > list doesn't match what we expect? > > What's build['key']? I don't see it in buildbucket docs. > > > > Ah, that's not a real thing, what I meant was "does it seem like a good idea to > use the square bracket operator (__getitem__) which raises a KeyError, instead > of dict.get?". Sounds like dict.get is a reasonable way to do it? ah, dict.get() is the right way.
On 2016/08/30 at 23:47:35, tandrii wrote: > I don't want to hold you any longer. Code as is => LGTM, > but I stand by suggestion to add simple test to avoid breakage. I tried adding a test, but so far my attempt add a test method has ended up complicated and not clear. Rather than add a test which I don't really understand, I'd prefer to leave it as is :-/ > https://codereview.chromium.org/2274743003/diff/40001/git_cl.py > File git_cl.py (right): > > https://codereview.chromium.org/2274743003/diff/40001/git_cl.py#newcode549 > git_cl.py:549: build.get('parameters_json', '{}')).get('builder_name'), > On 2016/08/30 23:41:24, qyearsley wrote: > > On 2016/08/30 at 22:22:45, tandrii(chromium) wrote: > > > what about bucket_name? (that's where you can get what we call master name in > > buildbot world). > > > > > > also, i think it's worthwhile to report category here and url. > > > > Added bucket and url. > > > > By category, do you mean something like the category printed with `git cl > > try-results`, which is generally a combination of "status", "result", and > > "failure_reason"? > > no, i meant parameters_json.category. It's usually CQ or "git cl" or "experimental". That said, doesn't have to be in this CL. Aye, I think it's not necessary right now, and if someone needs it and adds it later, that should be OK :-)
ACK, still LGTM
The CQ bit was checked by qyearsley@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Depot Tools Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/30fa797f71528c10)
the presubmit failure could be due to me, reverting my change now https://codereview.chromium.org/2295043003/
The CQ bit was checked by tandrii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Depot Tools Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/30fa956cf87d9e10)
On 2016/08/31 18:05:08, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > Depot Tools Presubmit on luci.infra.try (JOB_FAILED, > https://luci-milo.appspot.com/swarming/task/30fa956cf87d9e10) I filed http://crbug.com/642816
The CQ bit was checked by qyearsley@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add a --json option to git cl try-results. For use in scripts that check try job results, this is currently practically the same as making requests to https://codereview.chromium.org/api/<cl-number>/<patchset-number>. However, I think that this might be more future-proof, since it fetches try job information from Buildbucket, and I expect to still work after migration from Rietveld to Gerrit. BUG=640354 ========== to ========== Add a --json option to git cl try-results. For use in scripts that check try job results, this is currently practically the same as making requests to https://codereview.chromium.org/api/<cl-number>/<patchset-number>. However, I think that this might be more future-proof, since it fetches try job information from Buildbucket, and I expect to still work after migration from Rietveld to Gerrit. BUG=640354 Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/53f48a1c52f7d3... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/tools/depot_tools/+/53f48a1c52f7d3... |
