Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions gateway/controller/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ load("@rules_go//go:def.bzl", "go_library", "go_test")

go_library(
name = "controller",
srcs = ["ping.go"],
srcs = ["ping.go", "land.go"],
importpath = "github.com/uber/submitqueue/gateway/controller",
visibility = ["//visibility:public"],
deps = [
Expand All @@ -14,7 +14,7 @@ go_library(

go_test(
name = "controller_test",
srcs = ["ping_test.go"],
srcs = ["ping_test.go", "land_test.go"],
embed = [":controller"],
deps = ["//gateway/protopb"],
)
54 changes: 54 additions & 0 deletions gateway/controller/land.go
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 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be called Controller simply?

Copy link
Copy Markdown
Collaborator

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.go

Copy link
Copy Markdown
Contributor Author

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

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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we default initialize? For tests we can initialize with noop or as needed (if validating things in the logger or scope).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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
}
34 changes: 34 additions & 0 deletions gateway/controller/land_test.go
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 t.Fatal?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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

}
68 changes: 68 additions & 0 deletions gateway/proto/gateway.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Comment thread
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;
Comment thread
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 {
Comment thread
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.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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;
Comment thread
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) {}
}
Loading