[feat] Proto API for Gateway service#11
Conversation
|
only changed comments supplier-<provider so far, let me know if you have strong preference on naming |
| ) | ||
|
|
||
| // LandController handles land business logic for the gateway | ||
| type LandController struct { |
There was a problem hiding this comment.
Can this be called Controller simply?
There was a problem hiding this comment.
we could but then we might need create another folder say controller/land/controller.go
There was a problem hiding this comment.
I leave it as is for now, following the "Ping" example
| if logger == nil { | ||
| logger = zap.NewNop() | ||
| } | ||
| if scope == nil { | ||
| scope = tally.NoopScope | ||
| } |
There was a problem hiding this comment.
Can we default initialize? For tests we can initialize with noop or as needed (if validating things in the logger or scope).
There was a problem hiding this comment.
we probably should, again just following the example for now
| if err != nil { | ||
| t.Fatalf("Land() returned error: %v", err) | ||
| } | ||
|
|
||
| if resp.Sqid == "" { | ||
| t.Fatal("Expected sqid to be set, got empty string") | ||
| } |
There was a problem hiding this comment.
Any reason we are not performing assertions here instead? Why do we need t.Fatal?
There was a problem hiding this comment.
following the example, we should switch to asserts at some point, will require adding a dependency
| message LandRequest { | ||
| // Name of the queue processing the land request. The queue should be defined in the configuration, otherwise an error is returned. | ||
| string queue = 1; | ||
| // Change (such as a pull request) to land into the target branch. Target branch is defined by the queue configuration. |
There was a problem hiding this comment.
Can we clarify if this is a url or a change ID with an e.g?
4 participants
API object model for the controller implementing getting a SubmitQueue land request