Monorail client in Go

Monorail client in Go

Minimal monorail client in Go to create issues, update status and post
comments.

R=seanmccullough@chromium.org
BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/655127e47701299be14d5df02901d82b0a54c4c6

[nodir, 2016-06-03 18:03:21]

PTAL

[seanmccullough1, 2016-06-03 18:45:32]

seanmccullough@google.com changed reviewers:
+ seanmccullough@google.com

[seanmccullough1, 2016-06-03 18:45:33]

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/httputil/httpu...
File go/src/infra/httputil/httputil.go (right):

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/httputil/httpu...
go/src/infra/httputil/httputil.go:26: // ctx deadline is limited to client's
timeout.
Nice. Does this do exponential back-off?

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/monorail/endpo...
File go/src/infra/monorail/endpoints.go (right):

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/monorail/endpo...
go/src/infra/monorail/endpoints.go:60: // Occasionally Cloud Endpoints returns
404.
Even when it really should be a 500 :)

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/monorail/endpo...
go/src/infra/monorail/endpoints.go:72: func (c *epClient) InsertIssue(ctx
context.Context, req *InsertIssueRequest, options ...grpc.CallOption)
(*InsertIssueResponse, error) {
Godoc here and below. Mention that even though the signature accepts
...grpc.CallOption that actually passing any will cause an error. Either that or
don't accept CallOptions at all.

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/monorail/monor...
File go/src/infra/monorail/monorail.proto (right):

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/monorail/monor...
go/src/infra/monorail/monorail.proto:13: package monorail;
What you've done here is handy because you can add funcs to the generated types
like you've done in ext.go, but that also feels a little like it violates the
"Do not modify generated code" guideline.  

Is this a common practice in LUCI?

In google3 we typically keep the generated go code in separate foo_pb packages
and write any helper funcs so they act on protos as params rather than
receivers.

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/monorail/valid...
File go/src/infra/monorail/validation.go (right):

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/monorail/valid...
go/src/infra/monorail/validation.go:10: func (i *IssueRef) Validate() error {
I see. This could be annoying to do without adding funcs to the generated types.
You'd wouldn't have to write ValidateFoo for every $Foo though.

You could write it as one func Validate(i interface{}) error {...}, switch on
i's type inside it to do each of these validations, and return an error for
unrecognized types.

Or (cutting the Gordian knot here) you could just leave all of this validation
up to the server, which has its own notion of what is valid and what isn't. Just
send whatever the user of the client library has given you, and let the server
report back validation errors.

[seanmccullough1, 2016-06-03 18:45:54]

Description was changed from

==========
Monorail client in Go

Minimal monorail client in Go to create issues, update status and post
comments.

R=seanmccullough@chromium.org
BUG=
==========

to

==========
Monorail client in Go

Minimal monorail client in Go to create issues, update status and post
comments.

R=seanmccullough@chromium.org
BUG=
==========

[seanmccullough1, 2016-06-03 18:45:54]

seanmccullough@google.com changed reviewers:
+ sheyang@chromium.org

[nodir, 2016-06-03 18:56:12]

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/httputil/httpu...
File go/src/infra/httputil/httputil.go (right):

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/httputil/httpu...
go/src/infra/httputil/httputil.go:26: // ctx deadline is limited to client's
timeout.
On 2016/06/03 18:45:33, seanmccullough1 wrote:
> Nice. Does this do exponential back-off?  

retry.Default does

updated docstring

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/monorail/endpo...
File go/src/infra/monorail/endpoints.go (right):

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/monorail/endpo...
go/src/infra/monorail/endpoints.go:60: // Occasionally Cloud Endpoints returns
404.
On 2016/06/03 18:45:33, seanmccullough1 wrote:
> Even when it really should be a 500 :)

yes!

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/monorail/endpo...
go/src/infra/monorail/endpoints.go:72: func (c *epClient) InsertIssue(ctx
context.Context, req *InsertIssueRequest, options ...grpc.CallOption)
(*InsertIssueResponse, error) {
On 2016/06/03 18:45:33, seanmccullough1 wrote:
> Godoc here and below. Mention that even though the signature accepts
> ...grpc.CallOption that actually passing any will cause an error. Either that
or
> don't accept CallOptions at all.

these functions are internal, so nobody will see their docstrings.

Mentioned CallOption in NewEndpointsClient docstring

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/monorail/monor...
File go/src/infra/monorail/monorail.proto (right):

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/monorail/monor...
go/src/infra/monorail/monorail.proto:13: package monorail;
On 2016/06/03 18:45:33, seanmccullough1 wrote:
> What you've done here is handy because you can add funcs to the generated
types
> like you've done in ext.go, but that also feels a little like it violates the
> "Do not modify generated code" guideline.  
> 
> Is this a common practice in LUCI?

Yes
https://cs.chromium.org/chromium/infra/go/src/github.com/luci/luci-go/common/...
 
> In google3 we typically keep the generated go code in separate foo_pb packages
> and write any helper funcs so they act on protos as params rather than
> receivers. 

I can do either way, let me know

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/monorail/valid...
File go/src/infra/monorail/validation.go (right):

https://codereview.chromium.org/2037143002/diff/1/go/src/infra/monorail/valid...
go/src/infra/monorail/validation.go:10: func (i *IssueRef) Validate() error {
On 2016/06/03 18:45:33, seanmccullough1 wrote:
> I see. This could be annoying to do without adding funcs to the generated
types.
> You'd wouldn't have to write ValidateFoo for every $Foo though.

yeah, we use this approach a lot in luci-go.
 
> You could write it as one func Validate(i interface{}) error {...}, switch on
> i's type inside it to do each of these validations, and return an error for
> unrecognized types.

This is not type safe.

> Or (cutting the Gordian knot here) you could just leave all of this validation
> up to the server, which has its own notion of what is valid and what isn't.
Just
> send whatever the user of the client library has given you, and let the server
> report back validation errors.

I did that and server replied with very weird error messages that used internal
terms.

[nodir, 2016-06-03 19:02:59]

a validation method of a proto struct sounds fine to me because code of the
validation function is tightly coupled to the struct fields, and in most cases
the set of fields and validation code will be updated together

[seanmccullough1, 2016-06-03 19:29:50]

lgtm

[nodir, 2016-06-03 19:43:58]

The CQ bit was checked by nodir@chromium.org

[commit-bot: I haz the power, 2016-06-03 19:44:02]

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

[commit-bot: I haz the power, 2016-06-03 19:49:35]

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

[commit-bot: I haz the power, 2016-06-03 19:49:36]

Try jobs failed on following builders:
  Infra Win Tester on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Win%20Tester/bu...)

[nodir, 2016-06-03 19:53:23]

tests.. right

[nodir, 2016-06-03 21:51:49]

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

[commit-bot: I haz the power, 2016-06-03 21:51:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2037143002/40001

[commit-bot: I haz the power, 2016-06-03 21:56:10]

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

[commit-bot: I haz the power, 2016-06-03 21:56:11]

Dry run: 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%...)

[nodir, 2016-06-03 22:38:42]

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

[commit-bot: I haz the power, 2016-06-03 22:38:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2037143002/60001

[nodir, 2016-06-03 22:41:09]

The CQ bit was unchecked by nodir@chromium.org

[nodir, 2016-06-03 22:41:15]

The CQ bit was checked by nodir@chromium.org

[nodir, 2016-06-03 22:41:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
seanmccullough@google.com
Link to the patchset: https://codereview.chromium.org/2037143002/#ps60001
(title: "more tests")

[commit-bot: I haz the power, 2016-06-03 22:41:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2037143002/60001

[commit-bot: I haz the power, 2016-06-03 22:45:03]

Description was changed from

==========
Monorail client in Go

Minimal monorail client in Go to create issues, update status and post
comments.

R=seanmccullough@chromium.org
BUG=
==========

to

==========
Monorail client in Go

Minimal monorail client in Go to create issues, update status and post
comments.

R=seanmccullough@chromium.org
BUG=

Committed:
https://chromium.googlesource.com/infra/infra/+/655127e47701299be14d5df02901d...
==========

[commit-bot: I haz the power, 2016-06-03 22:45:04]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/infra/infra/+/655127e47701299be14d5df02901d...