-
Notifications
You must be signed in to change notification settings - Fork 0
[feat] Proto API for Gateway service #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| package controller | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "time" | ||
|
|
||
| "github.com/uber-go/tally/v4" | ||
| pb "github.com/uber/submitqueue/gateway/protopb" | ||
| "go.uber.org/zap" | ||
| ) | ||
|
|
||
| // LandController handles land business logic for the gateway | ||
| type LandController struct { | ||
| logger *zap.Logger | ||
| metricsScope tally.Scope | ||
| } | ||
|
|
||
| // NewLandController creates a new instance of the gateway land controller | ||
| func NewLandController(logger *zap.Logger, scope tally.Scope) *LandController { | ||
| if logger == nil { | ||
| logger = zap.NewNop() | ||
| } | ||
| if scope == nil { | ||
| scope = tally.NoopScope | ||
| } | ||
|
Comment on lines
+21
to
+26
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we default initialize? For tests we can initialize with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we probably should, again just following the example for now |
||
|
|
||
| return &LandController{ | ||
| logger: logger, | ||
| metricsScope: scope, | ||
| } | ||
| } | ||
|
|
||
| // Land handles the land request and returns a response | ||
| func (c *LandController) Land(ctx context.Context, req *pb.LandRequest) (*pb.LandResponse, error) { | ||
| start := time.Now() | ||
| defer func() { | ||
| c.metricsScope.Timer("land_request_latency").Record(time.Since(start)) | ||
| }() | ||
|
|
||
| c.metricsScope.Counter("land_request_count").Inc(1) | ||
|
|
||
| // TODO: Implement proper SQID generation and send the request to the appropriate queue. So far unix time to make it sequential. | ||
| sqid := fmt.Sprintf("%d", time.Now().Unix()) | ||
|
|
||
| c.logger.Debug("land request received", | ||
| zap.String("queue", req.Queue), | ||
| zap.String("sqid", sqid), | ||
| ) | ||
|
|
||
| return &pb.LandResponse{ | ||
| Sqid: sqid, | ||
| }, nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| package controller | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| pb "github.com/uber/submitqueue/gateway/protopb" | ||
| ) | ||
|
|
||
| func TestNewLandController(t *testing.T) { | ||
| controller := NewLandController(nil, nil) | ||
| if controller == nil { | ||
| t.Fatal("NewLandController() returned nil") | ||
| } | ||
| } | ||
|
|
||
| func TestLand_ReturnsSqid(t *testing.T) { | ||
| controller := NewLandController(nil, nil) | ||
| ctx := context.Background() | ||
|
|
||
| req := &pb.LandRequest{ | ||
| Queue: "test-queue", | ||
| Change: &pb.Change{Source: "github", Ids: []string{"123"}}, | ||
| } | ||
| resp, err := controller.Land(ctx, req) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("Land() returned error: %v", err) | ||
| } | ||
|
|
||
| if resp.Sqid == "" { | ||
| t.Fatal("Expected sqid to be set, got empty string") | ||
| } | ||
|
Comment on lines
+27
to
+33
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason we are not performing assertions here instead? Why do we need
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. following the example, we should switch to asserts at some point, will require adding a dependency |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,10 @@ option java_multiple_files = true; | |
| option java_outer_classname = "GatewayProto"; | ||
| option java_package = "com.uber.devexp.submitqueue.gateway"; | ||
|
|
||
| // *************** | ||
| // API domain definitions. | ||
| // *************** | ||
|
|
||
| // PingRequest is the request for the Ping method | ||
| message PingRequest { | ||
| // Optional message to include in the ping | ||
|
|
@@ -25,8 +29,72 @@ message PingResponse { | |
| string hostname = 4; | ||
| } | ||
|
|
||
| // Strategy defines the possible source control integration methods | ||
| enum Strategy { | ||
| // Default strategy (let server decide based on configuration) | ||
| STRATEGY_DEFAULT = 0; | ||
| // Rebase commits onto target branch before landing | ||
| STRATEGY_REBASE = 1; | ||
| // Same as REBASE but squash commits into a single commit before rebase | ||
| STRATEGY_SQUASH_REBASE = 2; | ||
| // Merge commits into the target branch by creating a separate merge commit, preserving the commit history along with hashes | ||
| STRATEGY_MERGE = 3; | ||
| } | ||
|
|
||
| // Change represents a set of related code changes identified by one or more IDs from a particular code change provider, like Github Pull Requests. | ||
| message Change { | ||
| // The code change provider (e.g., "github", "gerrit", "phabricator"). | ||
| string source = 1; | ||
|
sbalabanov marked this conversation as resolved.
|
||
| // List of change IDs, in a format specific to the code change provider, that should be landed together. SubmitQueue guarantees that the changes are landed in the order of the list, | ||
| // and no other changes are landed in between. SubmitQueue does not guarantee that each change is individually valid, but produces a special validity | ||
| // marker on such changes. The user is responsible to include all changes in a change stack in the order of the list, starting from the earlist change. | ||
| repeated string ids = 2; | ||
|
behinddwalls marked this conversation as resolved.
|
||
| } | ||
|
|
||
| // LandRequest defines a request to land (merge into target branch of the source control repository) a set of code changes. | ||
| message LandRequest { | ||
|
sbalabanov marked this conversation as resolved.
|
||
| // 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we clarify if this is a url or a change ID with an e.g? |
||
| Change change = 2; | ||
| // Source control integration strategy to use for this land operation. If not specified, the default queue strategy is used. | ||
| Strategy strategy = 4; | ||
| } | ||
|
|
||
| // LandResponse defines the response to a land request. | ||
| message LandResponse { | ||
| // Globally unique identifier for the land request. Used to track the land request lifecycle. | ||
| string sqid = 1; | ||
|
sbalabanov marked this conversation as resolved.
|
||
| } | ||
|
|
||
| // *************** | ||
| // Error messages, returned as `google.rpc.Status` messages. | ||
| // *************** | ||
|
|
||
| // Generic error with metadata. Each custom error type should extend this message. | ||
| message Error { | ||
| // Free text error message describing the error. | ||
| string message = 1; | ||
| } | ||
|
|
||
| // UnrecognizedQueueError is returned when a queue name is not recognized. Typically this indicates a typo in the queue name or server misconfiguration. | ||
| message UnrecognizedQueueError { | ||
| // Free text error message describing the error. | ||
| Error error = 1; | ||
| // Name of the queue that was not recognized. | ||
| string queue = 2; | ||
| } | ||
|
|
||
| // *************** | ||
| // Service definitions. | ||
| // *************** | ||
|
|
||
| // SubmitQueueGateway provides the gateway API | ||
| service SubmitQueueGateway { | ||
| // Ping returns a response indicating the service is alive | ||
| rpc Ping(PingRequest) returns (PingResponse) {} | ||
|
|
||
| // Land lands a set of code changes into a target branch, performing the necessary validations across all other changes in the queue. | ||
| // The processing is asynchronous and returns a LandResponse immediately. The land request is processed in the background. | ||
| rpc Land(LandRequest) returns (LandResponse) {} | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be called
Controllersimply?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could but then we might need create another folder say
controller/land/controller.goThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I leave it as is for now, following the "Ping" example