Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2056603002:
[monorail go api] add IssuesList rpc (Closed)
|
Created:
4 years, 6 months ago by seanmccullough1 Modified:
4 years, 6 months ago CC:
chromium-reviews, infra-reviews+infra_chromium.org, sheyang Base URL:
https://chromium.googlesource.com/infra/infra.git@master Target Ref:
refs/heads/master Project:
infra Visibility:
Public. |
Description[monorail go api] add IssuesList rpc
BUG=618434
Committed: https://chromium.googlesource.com/infra/infra/+/c5a21a35cdc0d08355cd00a125b80247a4e982e8
Patch Set 1 #
Total comments: 14
Patch Set 2 : #
Total comments: 15
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : . #
Total comments: 1
Patch Set 6 : doc #Patch Set 7 : example in doc #
Total comments: 2
Patch Set 8 : lowering test coverage #
Messages
Total messages: 27 (10 generated)
seanmccullough@google.com changed reviewers: + martiniss@chromium.org, nodir@chromium.org
lgtm https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/monor... File go/src/infra/monorail/monorail.proto (right): https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/monor... go/src/infra/monorail/monorail.proto:117: CannedQuery can= 3; space around = https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/monor... go/src/infra/monorail/monorail.proto:135: string kind = 4; what is this exactly?
https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/endpo... File go/src/infra/monorail/endpoints.go (right): https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/endpo... go/src/infra/monorail/endpoints.go:95: Please don't copy paste code such big chunks of code so close. Add "method string" parameter to call() https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/endpo... go/src/infra/monorail/endpoints.go:176: url := fmt.Sprintf("/projects/%s/issues?can=open&q=%s", req.ProjectId, req.Q) req.Can should be used instead of hardcoded open https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/endpo... go/src/infra/monorail/endpoints.go:176: url := fmt.Sprintf("/projects/%s/issues?can=open&q=%s", req.ProjectId, req.Q) What about all the other fields? Either don't declare them, or pass them here. Consider using url.Values type https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/monor... File go/src/infra/monorail/monorail.proto (right): https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/monor... go/src/infra/monorail/monorail.proto:105: message IssuesListRequest { Comments world be great, otherwise a reader may have hard time understanding what these fields mean. Other fields and messages in this file are documented https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/monor... go/src/infra/monorail/monorail.proto:132: // error = messages.MessageField(ErrorMessage, 1) Looks like an important field. Why not included?
https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/endpo... File go/src/infra/monorail/endpoints.go (right): https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/endpo... go/src/infra/monorail/endpoints.go:95: On 2016/06/09 04:09:28, nodir wrote: > Please don't copy paste code such big chunks of code so close. Add "method > string" parameter to call() Done. https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/endpo... go/src/infra/monorail/endpoints.go:176: url := fmt.Sprintf("/projects/%s/issues?can=open&q=%s", req.ProjectId, req.Q) On 2016/06/09 04:09:28, nodir wrote: > What about all the other fields? Either don't declare them, or pass them here. > Consider using url.Values type Done. https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/endpo... go/src/infra/monorail/endpoints.go:176: url := fmt.Sprintf("/projects/%s/issues?can=open&q=%s", req.ProjectId, req.Q) On 2016/06/09 04:09:28, nodir wrote: > req.Can should be used instead of hardcoded open Done. https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/monor... File go/src/infra/monorail/monorail.proto (right): https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/monor... go/src/infra/monorail/monorail.proto:105: message IssuesListRequest { On 2016/06/09 04:09:28, nodir wrote: > Comments world be great, otherwise a reader may have hard time understanding > what these fields mean. Other fields and messages in this file are documented Done. https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/monor... go/src/infra/monorail/monorail.proto:117: CannedQuery can= 3; On 2016/06/09 02:30:13, martiniss wrote: > space around = Done. https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/monor... go/src/infra/monorail/monorail.proto:132: // error = messages.MessageField(ErrorMessage, 1) On 2016/06/09 04:09:28, nodir wrote: > Looks like an important field. Why not included? It was ignored by other Request protos defined here. Added it for this one. https://codereview.chromium.org/2056603002/diff/1/go/src/infra/monorail/monor... go/src/infra/monorail/monorail.proto:135: string kind = 4; On 2016/06/09 02:30:13, martiniss wrote: > what is this exactly? I could defer to the original doc: https://chromium.googlesource.com/infra/infra/+/f2e2a4d/appengine/monorail/pr... :) I believe it is an endpoints thing, for this response I assume it will always be "monorail#issueList"
https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/e... File go/src/infra/monorail/endpoints.go (right): https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/e... go/src/infra/monorail/endpoints.go:133: args.Set("q", req.Q) other values are still ignored, e.g. "label" please add them to args https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/m... File go/src/infra/monorail/monorail.proto (right): https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/m... go/src/infra/monorail/monorail.proto:105: // Request for a list of Monorail.Issue() Why "()"? https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/m... go/src/infra/monorail/monorail.proto:123: string label = 4; this field should have been repeated I've filed https://bugs.chromium.org/p/monorail/issues/detail?id=1440 if you and/or sheyang agrees that it is a bug that needs to be fixed, please make this field repeated. It will work for 0 and 1 labels, but we won't work for more until monorail api is fixed https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/m... go/src/infra/monorail/monorail.proto:135: string sort = 10; same here https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/m... go/src/infra/monorail/monorail.proto:160: string kind = 4; kind is endpoints-specific (included in all responses), while this .proto is more general. Please remove this field
https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/e... File go/src/infra/monorail/endpoints.go (right): https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/e... go/src/infra/monorail/endpoints.go:133: args.Set("q", req.Q) On 2016/06/09 15:55:55, nodir wrote: > other values are still ignored, e.g. "label" > please add them to args Done. https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/m... File go/src/infra/monorail/monorail.proto (right): https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/m... go/src/infra/monorail/monorail.proto:105: // Request for a list of Monorail.Issue() On 2016/06/09 15:55:55, nodir wrote: > Why "()"? Why the () on every other Request message comment in this file? Removed. https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/m... go/src/infra/monorail/monorail.proto:123: string label = 4; On 2016/06/09 15:55:55, nodir wrote: > this field should have been repeated > I've filed https://bugs.chromium.org/p/monorail/issues/detail?id=1440 > if you and/or sheyang agrees that it is a bug that needs to be fixed, please > make this field repeated. It will work for 0 and 1 labels, but we won't work for > more until monorail api is fixed see comment on bug https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/m... go/src/infra/monorail/monorail.proto:135: string sort = 10; On 2016/06/09 15:55:55, nodir wrote: > same here The point of this CL is to make it possible to use the existing API from go, not to fix the API semantics. That conversation should happen separately and independently. https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/m... go/src/infra/monorail/monorail.proto:160: string kind = 4; On 2016/06/09 15:55:55, nodir wrote: > kind is endpoints-specific (included in all responses), while this .proto is > more general. Please remove this field Done.
lgtm % comments https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/m... File go/src/infra/monorail/monorail.proto (right): https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/m... go/src/infra/monorail/monorail.proto:105: // Request for a list of Monorail.Issue() On 2016/06/09 16:11:40, seanmccullough1 wrote: > On 2016/06/09 15:55:55, nodir wrote: > > Why "()"? > Why the () on every other Request message comment in this file? because those are RPCs, e.g. Monorail is a service name and InsertComment is an RPC. There is no "Issue" RPC in Monorail service To be consistent, it should be "Request for Monorail.IssuesList()" > > Removed. I don't see it removed https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/m... go/src/infra/monorail/monorail.proto:123: string label = 4; On 2016/06/09 16:11:41, seanmccullough1 wrote: > On 2016/06/09 15:55:55, nodir wrote: > > this field should have been repeated > > I've filed https://bugs.chromium.org/p/monorail/issues/detail?id=1440 > > if you and/or sheyang agrees that it is a bug that needs to be fixed, please > > make this field repeated. It will work for 0 and 1 labels, but we won't work > for > > more until monorail api is fixed > > see comment on bug then document that this is a space-separated list of labels same below https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/m... go/src/infra/monorail/monorail.proto:135: string sort = 10; On 2016/06/09 16:11:41, seanmccullough1 wrote: > On 2016/06/09 15:55:55, nodir wrote: > > same here > > The point of this CL is to make it possible to use the existing API from go, not > to fix the API semantics. That conversation should happen separately and > independently. ack
https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/m... File go/src/infra/monorail/monorail.proto (right): https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/m... go/src/infra/monorail/monorail.proto:105: // Request for a list of Monorail.Issue() On 2016/06/09 16:20:16, nodir wrote: > On 2016/06/09 16:11:40, seanmccullough1 wrote: > > On 2016/06/09 15:55:55, nodir wrote: > > > Why "()"? > > Why the () on every other Request message comment in this file? > > because those are RPCs, e.g. Monorail is a service name and InsertComment is an > RPC. There is no "Issue" RPC in Monorail service > > To be consistent, it should be "Request for Monorail.IssuesList()" > > > > > Removed. > > I don't see it removed oops sorry. Done. https://codereview.chromium.org/2056603002/diff/20001/go/src/infra/monorail/m... go/src/infra/monorail/monorail.proto:123: string label = 4; On 2016/06/09 16:20:16, nodir wrote: > On 2016/06/09 16:11:41, seanmccullough1 wrote: > > On 2016/06/09 15:55:55, nodir wrote: > > > this field should have been repeated > > > I've filed https://bugs.chromium.org/p/monorail/issues/detail?id=1440 > > > if you and/or sheyang agrees that it is a bug that needs to be fixed, please > > > make this field repeated. It will work for 0 and 1 labels, but we won't work > > for > > > more until monorail api is fixed > > > > see comment on bug > > then document that this is a space-separated list of labels > > same below Done.
The CQ bit was checked by seanmccullough@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from martiniss@chromium.org, nodir@chromium.org Link to the patchset: https://codereview.chromium.org/2056603002/#ps80001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2056603002/80001
https://codereview.chromium.org/2056603002/diff/80001/go/src/infra/monorail/m... File go/src/infra/monorail/monorail.proto (right): https://codereview.chromium.org/2056603002/diff/80001/go/src/infra/monorail/m... go/src/infra/monorail/monorail.proto:134: // Sort-by field or fields, concatenated with leading + or - to indicate it should have mentioned that the list is space-separated
The CQ bit was unchecked by seanmccullough@google.com
https://codereview.chromium.org/2056603002/diff/120001/go/src/infra/monorail/... File go/src/infra/monorail/monorail.proto (right): https://codereview.chromium.org/2056603002/diff/120001/go/src/infra/monorail/... go/src/infra/monorail/monorail.proto:136: // estdays increasing, then milestone decreasing. How's this?
https://codereview.chromium.org/2056603002/diff/120001/go/src/infra/monorail/... File go/src/infra/monorail/monorail.proto (right): https://codereview.chromium.org/2056603002/diff/120001/go/src/infra/monorail/... go/src/infra/monorail/monorail.proto:136: // estdays increasing, then milestone decreasing. On 2016/06/09 16:37:41, seanmccullough1 wrote: > How's this? this is nice. lgtm
The CQ bit was checked by seanmccullough@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from martiniss@chromium.org Link to the patchset: https://codereview.chromium.org/2056603002/#ps120001 (title: "example in doc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2056603002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Infra Linux Trusty 64 Tester on tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Infra%20Linux%20Trusty%...) Infra Mac Tester on tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Infra%20Mac%20Tester/bu...)
The CQ bit was checked by seanmccullough@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org, martiniss@chromium.org Link to the patchset: https://codereview.chromium.org/2056603002/#ps140001 (title: "lowering test coverage")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2056603002/140001
Message was sent while issue was closed.
Description was changed from ========== [monorail go api] add IssuesList rpc BUG=618434 ========== to ========== [monorail go api] add IssuesList rpc BUG=618434 Committed: https://chromium.googlesource.com/infra/infra/+/c5a21a35cdc0d08355cd00a125b80... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/infra/infra/+/c5a21a35cdc0d08355cd00a125b80...
Message was sent while issue was closed.
CQ bit was unchecked |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
