From 7b895a16416be2eb854914b5b616e552a4f07628 Mon Sep 17 00:00:00 2001 From: rprithyani Date: Wed, 25 Feb 2026 00:10:36 +0000 Subject: [PATCH 1/5] interface for change provider and update request to include uri --- entity/request.go | 10 +-- entity/request_test.go | 20 +++--- extension/change_provider/change_provider.go | 34 ++++++++++ extension/storage/mysql/request_store.go | 18 +++--- extension/storage/mysql/schema/request.sql | 2 +- gateway/controller/land.go | 8 +-- gateway/controller/land_test.go | 24 +++---- gateway/proto/gateway.proto | 15 +++-- gateway/protopb/gateway.pb.go | 24 ++++--- gateway/protopb/gateway.pb.yarpc.go | 62 +++++++++---------- orchestrator/controller/request/request.go | 2 +- .../controller/request/request_test.go | 8 +-- test/e2e/suite_test.go | 2 +- test/integration/extension/storage/suite.go | 4 +- test/integration/gateway/suite_test.go | 2 +- 15 files changed, 139 insertions(+), 96 deletions(-) create mode 100644 extension/change_provider/change_provider.go diff --git a/entity/request.go b/entity/request.go index 0dd33f4d..5762cfe5 100644 --- a/entity/request.go +++ b/entity/request.go @@ -33,13 +33,15 @@ const ( RequestStateError RequestState = "error" ) -// Change represents a set of related code changes identified by one or more IDs from a particular code change provider, like Github Pull Requests. +// Change represents a set of related code changes identified by one or more URIs from a particular code change provider, like Github Pull Requests. // The object is immutable after creation. type Change struct { - // Source is the code change provider (e.g., "github", "gerrit", "phabricator"). + // Source is the code change provider ID that maps to a registered provider (e.g., "github", "github-enterprise", "phabricator"). Source string `json:"source"` - // IDs is a list of change IDs, in a format specific to the code change provider, that should be landed together. - IDs []string `json:"ids"` + // URIs is a list of change URIs that should be landed together. The format is provider-specific: + // - GitHub: "owner/repo/pr_number@commit_hash" (e.g., "uber/submitqueue/123@abc123def") + // - Phabricator: "revision_id@commit_hash" (e.g., "D12345@abc123def") + URIs []string `json:"uris"` } // Request defines a request to land (merge into target branch of the source control repository) a set of code changes. diff --git a/entity/request_test.go b/entity/request_test.go index 74bf2a00..3c5f1d22 100644 --- a/entity/request_test.go +++ b/entity/request_test.go @@ -11,7 +11,7 @@ func TestRequest_ToBytes(t *testing.T) { req := Request{ ID: "test-queue/123", Queue: "test-queue", - Change: Change{Source: "github", IDs: []string{"PR-456", "PR-789"}}, + Change: Change{Source: "github", URIs: []string{"uber/submitqueue/456@abc123def", "uber/submitqueue/789@def456abc"}}, LandStrategy: RequestLandStrategyRebase, State: RequestStateNew, Version: 1, @@ -25,7 +25,7 @@ func TestRequest_ToBytes(t *testing.T) { jsonStr := string(data) assert.Contains(t, jsonStr, "test-queue/123") assert.Contains(t, jsonStr, "github") - assert.Contains(t, jsonStr, "PR-456") + assert.Contains(t, jsonStr, "uber/submitqueue/456@abc123def") assert.Contains(t, jsonStr, "rebase") assert.Contains(t, jsonStr, "new") } @@ -34,7 +34,7 @@ func TestRequestFromBytes(t *testing.T) { original := Request{ ID: "my-queue/999", Queue: "my-queue", - Change: Change{Source: "gerrit", IDs: []string{"CL-111"}}, + Change: Change{Source: "phabricator", URIs: []string{"D111@fedcba987"}}, LandStrategy: RequestLandStrategyMerge, State: RequestStateProcessing, Version: 3, @@ -52,7 +52,7 @@ func TestRequestFromBytes(t *testing.T) { assert.Equal(t, original.ID, deserialized.ID) assert.Equal(t, original.Queue, deserialized.Queue) assert.Equal(t, original.Change.Source, deserialized.Change.Source) - assert.Equal(t, original.Change.IDs, deserialized.Change.IDs) + assert.Equal(t, original.Change.URIs, deserialized.Change.URIs) assert.Equal(t, original.LandStrategy, deserialized.LandStrategy) assert.Equal(t, original.State, deserialized.State) assert.Equal(t, original.Version, deserialized.Version) @@ -85,33 +85,33 @@ func TestRequest_SerializationRoundTrip(t *testing.T) { req Request }{ { - name: "full request", + name: "full request with multiple PRs", req: Request{ ID: "queue1/100", Queue: "queue1", - Change: Change{Source: "github", IDs: []string{"PR-1", "PR-2", "PR-3"}}, + Change: Change{Source: "github", URIs: []string{"uber/repo-a/101@aaa111", "uber/repo-a/102@bbb222", "uber/repo-a/103@ccc333"}}, LandStrategy: RequestLandStrategySquashRebase, State: RequestStateLanded, Version: 5, }, }, { - name: "minimal request", + name: "phabricator revision", req: Request{ ID: "queue2/200", Queue: "queue2", - Change: Change{Source: "phabricator", IDs: []string{"D123"}}, + Change: Change{Source: "phabricator", URIs: []string{"D12345@abc123def456"}}, LandStrategy: RequestLandStrategyRebase, State: RequestStateNew, Version: 1, }, }, { - name: "error state request", + name: "github enterprise request", req: Request{ ID: "queue3/300", Queue: "queue3", - Change: Change{Source: "github", IDs: []string{"PR-999"}}, + Change: Change{Source: "github-enterprise", URIs: []string{"internal/service/999@deadbeef12"}}, LandStrategy: RequestLandStrategyMerge, State: RequestStateError, Version: 10, diff --git a/extension/change_provider/change_provider.go b/extension/change_provider/change_provider.go new file mode 100644 index 00000000..11470617 --- /dev/null +++ b/extension/change_provider/change_provider.go @@ -0,0 +1,34 @@ +package changeprovider + +import "context" + +// FileChangeStatus defines the type of modification made to a file. +type FileChangeStatus string + +const ( + // FileChangeStatusAdded indicates a new file was added. + FileChangeStatusAdded FileChangeStatus = "added" + // FileChangeStatusModified indicates an existing file was modified. + FileChangeStatusModified FileChangeStatus = "modified" + // FileChangeStatusDeleted indicates a file was deleted. + FileChangeStatusDeleted FileChangeStatus = "deleted" +) + +// FileChange represents a single file modification in a change. +type FileChange struct { + // Path is the file path relative to the repository root. + Path string + // Status is the type of change made to the file. + Status FileChangeStatus +} + +// ChangeProvider fetches the list of changed files from external code review providers. +// Each implementation is configured for a specific provider (GitHub, GitLab, Phabricator). +type ChangeProvider interface { + // Get retrieves the list of files changed across all provided URIs. + // The URI format is provider-specific: + // - GitHub: "owner/repo/pr_number@commit_hash" (e.g., "uber/submitqueue/123@abc123def") + // - Phabricator: "revision_id@commit_hash" (e.g., "D12345@abc123def") + // Returns the aggregated list of changed files across all URIs. + Get(ctx context.Context, uris []string) ([]FileChange, error) +} diff --git a/extension/storage/mysql/request_store.go b/extension/storage/mysql/request_store.go index 8f9841a8..42b87eba 100644 --- a/extension/storage/mysql/request_store.go +++ b/extension/storage/mysql/request_store.go @@ -25,12 +25,12 @@ func NewRequestStore(db *sql.DB) storage.RequestStore { // Get retrieves a land request by ID. Returns ErrNotFound if the request is not found. func (r *requestStore) Get(ctx context.Context, id string) (entity.Request, error) { var req entity.Request - var changeIDsJSON []byte + var changeURIsJSON []byte err := r.db.QueryRowContext(ctx, - "SELECT id, queue, change_source, change_ids, land_strategy, state, version FROM request WHERE id = ?", + "SELECT id, queue, change_source, change_uris, land_strategy, state, version FROM request WHERE id = ?", id, - ).Scan(&req.ID, &req.Queue, &req.Change.Source, &changeIDsJSON, &req.LandStrategy, &req.State, &req.Version) + ).Scan(&req.ID, &req.Queue, &req.Change.Source, &changeURIsJSON, &req.LandStrategy, &req.State, &req.Version) if errors.Is(err, sql.ErrNoRows) { return entity.Request{}, storage.WrapNotFound(err) @@ -39,8 +39,8 @@ func (r *requestStore) Get(ctx context.Context, id string) (entity.Request, erro return entity.Request{}, fmt.Errorf("failed to get request entity id=%s from the database: %w", id, err) } - if err := json.Unmarshal(changeIDsJSON, &req.Change.IDs); err != nil { - return entity.Request{}, fmt.Errorf("failed to unmarshal change IDs for request entity id=%s from the database: %w", id, err) + if err := json.Unmarshal(changeURIsJSON, &req.Change.URIs); err != nil { + return entity.Request{}, fmt.Errorf("failed to unmarshal change URIs for request entity id=%s from the database: %w", id, err) } return req, nil @@ -48,14 +48,14 @@ func (r *requestStore) Get(ctx context.Context, id string) (entity.Request, erro // Create creates a new land request. The request must have a unique ID already assigned. Returns ErrAlreadyExists if the request ID already exists. func (r *requestStore) Create(ctx context.Context, request entity.Request) error { - changeIDsJSON, err := json.Marshal(request.Change.IDs) + changeURIsJSON, err := json.Marshal(request.Change.URIs) if err != nil { - return fmt.Errorf("failed to marshal change IDs=%v id=%s for Create request entity: %w", request.Change.IDs, request.ID, err) + return fmt.Errorf("failed to marshal change URIs=%v id=%s for Create request entity: %w", request.Change.URIs, request.ID, err) } _, err = r.db.ExecContext(ctx, - "INSERT INTO request (id, queue, change_source, change_ids, land_strategy, state, version) VALUES (?, ?, ?, ?, ?, ?, ?)", - request.ID, request.Queue, request.Change.Source, changeIDsJSON, request.LandStrategy, request.State, request.Version, + "INSERT INTO request (id, queue, change_source, change_uris, land_strategy, state, version) VALUES (?, ?, ?, ?, ?, ?, ?)", + request.ID, request.Queue, request.Change.Source, changeURIsJSON, request.LandStrategy, request.State, request.Version, ) if err != nil { var mysqlErr *mysql.MySQLError diff --git a/extension/storage/mysql/schema/request.sql b/extension/storage/mysql/schema/request.sql index 5011f094..8012909e 100644 --- a/extension/storage/mysql/schema/request.sql +++ b/extension/storage/mysql/schema/request.sql @@ -2,7 +2,7 @@ CREATE TABLE IF NOT EXISTS request ( id VARCHAR(255) NOT NULL, queue VARCHAR(255) NOT NULL, change_source VARCHAR(255) NOT NULL, - change_ids JSON NOT NULL, + change_uris JSON NOT NULL, land_strategy VARCHAR(64) NOT NULL, state VARCHAR(64) NOT NULL, version INT NOT NULL, diff --git a/gateway/controller/land.go b/gateway/controller/land.go index 27fe58e6..679c1fa5 100644 --- a/gateway/controller/land.go +++ b/gateway/controller/land.go @@ -64,13 +64,13 @@ func (c *LandController) Land(ctx context.Context, req *pb.LandRequest) (*pb.Lan if req.Change == nil || req.Change.Source == "" { return nil, fmt.Errorf("LandController requires the request to have a change source specified: %w", ErrInvalidRequest) } - if len(req.Change.GetIds()) == 0 { - return nil, fmt.Errorf("LandController requires the request to have at least one change ID specified: %w", ErrInvalidRequest) + if len(req.Change.GetUris()) == 0 { + return nil, fmt.Errorf("LandController requires the request to have at least one change URI specified: %w", ErrInvalidRequest) } change := entity.Change{ Source: req.Change.GetSource(), - IDs: req.Change.GetIds(), + URIs: req.Change.GetUris(), } // TODO: validate that queue is configured. Return error if not. @@ -105,7 +105,7 @@ func (c *LandController) Land(ctx context.Context, req *pb.LandRequest) (*pb.Lan "queue", req.Queue, "sqid", request.ID, "change_source", change.Source, - "change_ids", change.IDs, + "change_uris", change.URIs, "strategy", string(strategy), ) diff --git a/gateway/controller/land_test.go b/gateway/controller/land_test.go index 9154e160..6400c17a 100644 --- a/gateway/controller/land_test.go +++ b/gateway/controller/land_test.go @@ -248,7 +248,7 @@ func TestLand_ReturnsSqid(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Ids: []string{"123"}}, + Change: &pb.Change{Source: "github", Uris: []string{"uber/test-repo/123@abc123def"}}, } resp, err := controller.Land(ctx, req) @@ -280,7 +280,7 @@ func TestLand_PassesCorrectParametersToStore(t *testing.T) { req := &pb.LandRequest{ Queue: "my-queue", - Change: &pb.Change{Source: "github", Ids: []string{"pr-1", "pr-2"}}, + Change: &pb.Change{Source: "github", Uris: []string{"uber/myservice/1@abc111", "uber/myservice/2@def222"}}, Strategy: pb.Strategy_REBASE, } resp, err := controller.Land(ctx, req) @@ -289,7 +289,7 @@ func TestLand_PassesCorrectParametersToStore(t *testing.T) { assert.Equal(t, "my-queue/42", capturedRequest.ID) assert.Equal(t, "my-queue", capturedRequest.Queue) assert.Equal(t, "github", capturedRequest.Change.Source) - assert.Equal(t, []string{"pr-1", "pr-2"}, capturedRequest.Change.IDs) + assert.Equal(t, []string{"uber/myservice/1@abc111", "uber/myservice/2@def222"}, capturedRequest.Change.URIs) assert.Equal(t, entity.RequestLandStrategyRebase, capturedRequest.LandStrategy) assert.Equal(t, entity.RequestStateNew, capturedRequest.State) assert.Equal(t, int32(1), capturedRequest.Version) @@ -317,7 +317,7 @@ func TestLand_ReturnsErrorOnStorageFailure(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Ids: []string{"123"}}, + Change: &pb.Change{Source: "github", Uris: []string{"uber/test-repo/123@abc123def"}}, } _, err := controller.Land(ctx, req) @@ -345,7 +345,7 @@ func TestLand_ReturnsErrorOnCounterFailure(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Ids: []string{"123"}}, + Change: &pb.Change{Source: "github", Uris: []string{"uber/test-repo/123@abc123def"}}, } _, err := controller.Land(ctx, req) @@ -376,7 +376,7 @@ func TestLand_CounterDomainIncludesQueue(t *testing.T) { req := &pb.LandRequest{ Queue: "my-queue", - Change: &pb.Change{Source: "github", Ids: []string{"123"}}, + Change: &pb.Change{Source: "github", Uris: []string{"uber/test-repo/123@abc123def"}}, } _, err := controller.Land(ctx, req) @@ -398,7 +398,7 @@ func TestLand_ReturnsErrorOnEmptyQueue(t *testing.T) { req := &pb.LandRequest{ Queue: "", - Change: &pb.Change{Source: "github", Ids: []string{"123"}}, + Change: &pb.Change{Source: "github", Uris: []string{"uber/test-repo/123@abc123def"}}, } _, err := controller.Land(ctx, req) @@ -420,7 +420,7 @@ func TestLand_ReturnsErrorOnEmptyChangeSource(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "", Ids: []string{"123"}}, + Change: &pb.Change{Source: "", Uris: []string{"uber/test-repo/123@abc123def"}}, } _, err := controller.Land(ctx, req) @@ -464,7 +464,7 @@ func TestLand_ReturnsErrorOnEmptyChangeIDs(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Ids: []string{}}, + Change: &pb.Change{Source: "github", Uris: []string{}}, } _, err := controller.Land(ctx, req) @@ -495,7 +495,7 @@ func TestLand_PublishesToQueue(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Ids: []string{"PR-456"}}, + Change: &pb.Change{Source: "github", Uris: []string{"uber/backend/456@fed987cba"}}, Strategy: pb.Strategy_REBASE, } resp, err := controller.Land(ctx, req) @@ -514,7 +514,7 @@ func TestLand_PublishesToQueue(t *testing.T) { assert.Equal(t, "test-queue/123", deserializedReq.ID) assert.Equal(t, "test-queue", deserializedReq.Queue) assert.Equal(t, "github", deserializedReq.Change.Source) - assert.Equal(t, []string{"PR-456"}, deserializedReq.Change.IDs) + assert.Equal(t, []string{"uber/backend/456@fed987cba"}, deserializedReq.Change.URIs) assert.Equal(t, entity.RequestLandStrategyRebase, deserializedReq.LandStrategy) assert.Equal(t, entity.RequestStateNew, deserializedReq.State) assert.Equal(t, int32(1), deserializedReq.Version) @@ -538,7 +538,7 @@ func TestLand_ContinuesWhenPublishFails(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Ids: []string{"PR-1"}}, + Change: &pb.Change{Source: "github", Uris: []string{"uber/service/1@abc123def"}}, } _, err := controller.Land(ctx, req) diff --git a/gateway/proto/gateway.proto b/gateway/proto/gateway.proto index 44a2a10b..054f32b0 100644 --- a/gateway/proto/gateway.proto +++ b/gateway/proto/gateway.proto @@ -41,14 +41,17 @@ enum 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. +// Change represents a set of related code changes identified by one or more URIs from a particular code change provider, like Github Pull Requests. message Change { - // The code change provider (e.g., "github", "gerrit", "phabricator"). + // The code change provider ID that maps to a registered provider (e.g., "github", "github-enterprise", "phabricator"). string source = 1; - // 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; + // List of change URIs that should be landed together. The format is provider-specific: + // - GitHub: "owner/repo/pr_number@commit_hash" (e.g., "uber/submitqueue/123@abc123def") + // - Phabricator: "revision_id@commit_hash" (e.g., "D12345@abc123def") + // 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 earliest change. + repeated string uris = 2; } // LandRequest defines a request to land (merge into target branch of the source control repository) a set of code changes. diff --git a/gateway/protopb/gateway.pb.go b/gateway/protopb/gateway.pb.go index 175058b8..9060d36a 100644 --- a/gateway/protopb/gateway.pb.go +++ b/gateway/protopb/gateway.pb.go @@ -197,15 +197,19 @@ func (x *PingResponse) GetHostname() string { return "" } -// Change represents a set of related code changes identified by one or more IDs from a particular code change provider, like Github Pull Requests. +// Change represents a set of related code changes identified by one or more URIs from a particular code change provider, like Github Pull Requests. type Change struct { state protoimpl.MessageState `protogen:"open.v1"` // The code change provider (e.g., "github", "gerrit", "phabricator"). Source string `protobuf:"bytes,1,opt,name=source,proto3" json:"source,omitempty"` - // 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. - Ids []string `protobuf:"bytes,2,rep,name=ids,proto3" json:"ids,omitempty"` + // List of change URIs that should be landed together. The format is provider-specific: + // - GitHub: "owner/repo/pr_number@commit_hash" (e.g., "uber/submitqueue/123@abc123def") + // - Phabricator: "revision_id@commit_hash" (e.g., "D12345@abc123def") + // + // 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 earliest change. + Uris []string `protobuf:"bytes,2,rep,name=uris,proto3" json:"uris,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -247,9 +251,9 @@ func (x *Change) GetSource() string { return "" } -func (x *Change) GetIds() []string { +func (x *Change) GetUris() []string { if x != nil { - return x.Ids + return x.Uris } return nil } @@ -476,10 +480,10 @@ const file_gateway_proto_rawDesc = "" + "\amessage\x18\x01 \x01(\tR\amessage\x12!\n" + "\fservice_name\x18\x02 \x01(\tR\vserviceName\x12\x1c\n" + "\ttimestamp\x18\x03 \x01(\x03R\ttimestamp\x12\x1a\n" + - "\bhostname\x18\x04 \x01(\tR\bhostname\"2\n" + + "\bhostname\x18\x04 \x01(\tR\bhostname\"4\n" + "\x06Change\x12\x16\n" + - "\x06source\x18\x01 \x01(\tR\x06source\x12\x10\n" + - "\x03ids\x18\x02 \x03(\tR\x03ids\"\xab\x01\n" + + "\x06source\x18\x01 \x01(\tR\x06source\x12\x12\n" + + "\x04uris\x18\x02 \x03(\tR\x04uris\"\xab\x01\n" + "\vLandRequest\x12\x14\n" + "\x05queue\x18\x01 \x01(\tR\x05queue\x12?\n" + "\x06change\x18\x02 \x01(\v2'.uber.devexp.submitqueue.gateway.ChangeR\x06change\x12E\n" + diff --git a/gateway/protopb/gateway.pb.yarpc.go b/gateway/protopb/gateway.pb.yarpc.go index c1cbe323..db603ba0 100644 --- a/gateway/protopb/gateway.pb.yarpc.go +++ b/gateway/protopb/gateway.pb.yarpc.go @@ -283,37 +283,37 @@ var ( var yarpcFileDescriptorClosuref1a937782ebbded5 = [][]byte{ // gateway.proto []byte{ - 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x8c, 0x93, 0xcd, 0x6e, 0xd3, 0x4e, - 0x14, 0xc5, 0xe3, 0x38, 0x71, 0x93, 0xeb, 0xf4, 0xaf, 0xfc, 0x47, 0xa8, 0xb2, 0x2a, 0x24, 0x52, - 0x23, 0xd1, 0xf0, 0xe5, 0x48, 0x66, 0x8b, 0x84, 0x12, 0x30, 0x65, 0x51, 0x50, 0x6a, 0x93, 0x0d, - 0x9b, 0xca, 0x1f, 0x57, 0x8e, 0x25, 0xec, 0x71, 0x66, 0xc6, 0x85, 0xb2, 0xe7, 0x69, 0x78, 0x2b, - 0x9e, 0x04, 0x79, 0x3c, 0x69, 0xbd, 0x01, 0x67, 0x77, 0xef, 0xf8, 0xfe, 0x7c, 0xce, 0x9c, 0x99, - 0x81, 0xe3, 0x34, 0x14, 0xf8, 0x2d, 0xbc, 0x75, 0x4a, 0x46, 0x05, 0x25, 0x8f, 0xaa, 0x08, 0x99, - 0x93, 0xe0, 0x0d, 0x7e, 0x2f, 0x1d, 0x5e, 0x45, 0x79, 0x26, 0x76, 0x15, 0x56, 0xe8, 0xa8, 0x31, - 0xfb, 0x1c, 0xcc, 0x75, 0x56, 0xa4, 0x3e, 0xee, 0x2a, 0xe4, 0x82, 0x58, 0x70, 0x94, 0x23, 0xe7, - 0x61, 0x8a, 0x96, 0x36, 0xd3, 0xe6, 0x63, 0x7f, 0xdf, 0xda, 0x3f, 0x35, 0x98, 0x34, 0x93, 0xbc, - 0xa4, 0x05, 0xc7, 0xbf, 0x8f, 0x92, 0x33, 0x98, 0x70, 0x64, 0x37, 0x59, 0x8c, 0xd7, 0x45, 0x98, - 0xa3, 0xd5, 0x97, 0x9f, 0x4d, 0xb5, 0xf6, 0x29, 0xcc, 0x91, 0x3c, 0x84, 0xb1, 0xc8, 0x72, 0xe4, - 0x22, 0xcc, 0x4b, 0x4b, 0x9f, 0x69, 0x73, 0xdd, 0xbf, 0x5f, 0x20, 0xa7, 0x30, 0xda, 0x52, 0x2e, - 0x24, 0x3c, 0x90, 0xf0, 0x5d, 0x6f, 0xbb, 0x60, 0xbc, 0xdd, 0x86, 0x45, 0x8a, 0xe4, 0x04, 0x0c, - 0x4e, 0x2b, 0x16, 0xef, 0xf5, 0x55, 0x47, 0xa6, 0xa0, 0x67, 0x09, 0xb7, 0xfa, 0x33, 0x7d, 0x3e, - 0xf6, 0xeb, 0xd2, 0xfe, 0xa5, 0x81, 0x79, 0x19, 0x16, 0xc9, 0x7e, 0x97, 0x0f, 0x60, 0x28, 0x53, - 0x50, 0x60, 0xd3, 0x90, 0x37, 0x60, 0xc4, 0xf2, 0xcf, 0xd2, 0xb0, 0xe9, 0x9e, 0x3b, 0x1d, 0xe1, - 0x39, 0x8d, 0x11, 0x5f, 0x61, 0xc4, 0x83, 0x11, 0x17, 0x2c, 0x14, 0x98, 0xde, 0x4a, 0xdb, 0xff, - 0xb9, 0x4f, 0x3b, 0x7f, 0x11, 0x28, 0xc0, 0xbf, 0x43, 0x6d, 0x1b, 0x26, 0x8d, 0x59, 0x15, 0x34, - 0x81, 0x01, 0xdf, 0x65, 0x89, 0x32, 0x2b, 0x6b, 0xfb, 0x0c, 0x86, 0x1e, 0x63, 0x94, 0xfd, 0xe3, - 0xc0, 0xbe, 0xc2, 0xc9, 0xa6, 0x60, 0x18, 0xd3, 0xb4, 0xc8, 0x7e, 0x60, 0x72, 0x55, 0xcb, 0x36, - 0xcc, 0x6b, 0x18, 0x62, 0x5d, 0x48, 0xc2, 0x74, 0x9f, 0x74, 0x9a, 0x94, 0x98, 0xdf, 0x40, 0xf7, - 0xe1, 0xf5, 0x5b, 0xe1, 0x3d, 0x5b, 0xc2, 0x68, 0xbf, 0x15, 0x62, 0xc2, 0xd1, 0x3b, 0xef, 0xfd, - 0x72, 0x73, 0xf9, 0x79, 0xda, 0x23, 0x00, 0x86, 0xef, 0xad, 0x96, 0x81, 0x37, 0xd5, 0xc8, 0xff, - 0x70, 0x1c, 0x5c, 0x6d, 0x96, 0xc1, 0x87, 0x6b, 0xb5, 0xd4, 0x27, 0x63, 0x18, 0x7e, 0xf4, 0xfc, - 0x0b, 0x6f, 0xaa, 0xbb, 0xbf, 0x35, 0x20, 0x81, 0x54, 0x97, 0x5e, 0x2f, 0x1a, 0x71, 0x82, 0x30, - 0xa8, 0xef, 0x1d, 0x79, 0xd1, 0x69, 0xb3, 0x75, 0x91, 0x4f, 0x5f, 0x1e, 0x38, 0xdd, 0x64, 0x6c, - 0xf7, 0x6a, 0x99, 0x3a, 0xf5, 0x03, 0x64, 0x5a, 0x37, 0xe9, 0x00, 0x99, 0xf6, 0x51, 0xda, 0xbd, - 0x55, 0x04, 0x8f, 0x63, 0x9a, 0x77, 0x51, 0xab, 0x89, 0xda, 0xfd, 0xba, 0x7e, 0xc5, 0x6b, 0xed, - 0xcb, 0xf3, 0x34, 0x13, 0xdb, 0x2a, 0x72, 0x62, 0x9a, 0x2f, 0x6a, 0x76, 0xd1, 0x82, 0x16, 0x0a, - 0x5a, 0xc8, 0x27, 0x5f, 0x46, 0x91, 0x21, 0x8b, 0x57, 0x7f, 0x02, 0x00, 0x00, 0xff, 0xff, 0x20, - 0x51, 0xf6, 0x91, 0x0c, 0x04, 0x00, 0x00, + 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x8c, 0x93, 0x4d, 0x6f, 0xd3, 0x40, + 0x10, 0x86, 0xe3, 0x7c, 0xb8, 0xc9, 0x38, 0x45, 0x61, 0x84, 0x2a, 0xab, 0x42, 0x22, 0x35, 0x12, + 0x0d, 0x5f, 0x8e, 0x14, 0x38, 0x22, 0xa1, 0x04, 0x4c, 0x39, 0x14, 0x94, 0xda, 0xe4, 0xc2, 0xa5, + 0xb2, 0x9d, 0x91, 0x63, 0x09, 0x7f, 0x64, 0x77, 0x5d, 0x28, 0x77, 0x7e, 0x0d, 0xff, 0x8a, 0x5f, + 0x82, 0xbc, 0xde, 0x34, 0xbe, 0x80, 0x73, 0x9b, 0x59, 0xcf, 0xe3, 0xf7, 0xdd, 0x77, 0x77, 0xe1, + 0x38, 0xf2, 0x05, 0x7d, 0xf7, 0x6f, 0xed, 0x9c, 0x65, 0x22, 0xc3, 0x47, 0x45, 0x40, 0xcc, 0x5e, + 0xd3, 0x0d, 0xfd, 0xc8, 0x6d, 0x5e, 0x04, 0x49, 0x2c, 0xb6, 0x05, 0x15, 0x64, 0xab, 0x31, 0xeb, + 0x1c, 0x8c, 0x65, 0x9c, 0x46, 0x2e, 0x6d, 0x0b, 0xe2, 0x02, 0x4d, 0x38, 0x4a, 0x88, 0x73, 0x3f, + 0x22, 0x53, 0x1b, 0x6b, 0x93, 0x81, 0xbb, 0x6b, 0xad, 0x5f, 0x1a, 0x0c, 0xab, 0x49, 0x9e, 0x67, + 0x29, 0xa7, 0x7f, 0x8f, 0xe2, 0x19, 0x0c, 0x39, 0xb1, 0x9b, 0x38, 0xa4, 0xeb, 0xd4, 0x4f, 0xc8, + 0x6c, 0xcb, 0xcf, 0x86, 0x5a, 0xfb, 0xec, 0x27, 0x84, 0x0f, 0x61, 0x20, 0xe2, 0x84, 0xb8, 0xf0, + 0x93, 0xdc, 0xec, 0x8c, 0xb5, 0x49, 0xc7, 0xdd, 0x2f, 0xe0, 0x29, 0xf4, 0x37, 0x19, 0x17, 0x12, + 0xee, 0x4a, 0xf8, 0xae, 0xb7, 0x5e, 0x83, 0xfe, 0x6e, 0xe3, 0xa7, 0x11, 0xe1, 0x09, 0xe8, 0x3c, + 0x2b, 0x58, 0xb8, 0xd3, 0x57, 0x1d, 0x22, 0x74, 0x0b, 0x16, 0x73, 0xb3, 0x3d, 0xee, 0x4c, 0x06, + 0xae, 0xac, 0xad, 0xdf, 0x1a, 0x18, 0x97, 0x7e, 0xba, 0xde, 0xed, 0xf3, 0x01, 0xf4, 0x64, 0x0e, + 0x0a, 0xad, 0x1a, 0x7c, 0x0b, 0x7a, 0x28, 0xff, 0x2d, 0x2d, 0x1b, 0xb3, 0x73, 0xbb, 0x21, 0x3e, + 0xbb, 0xb2, 0xe2, 0x2a, 0x0c, 0x1d, 0xe8, 0x73, 0xc1, 0x7c, 0x41, 0xd1, 0xad, 0x34, 0x7e, 0x6f, + 0xf6, 0xb4, 0xf1, 0x17, 0x9e, 0x02, 0xdc, 0x3b, 0xd4, 0xb2, 0x60, 0x58, 0x99, 0x55, 0x51, 0x23, + 0x74, 0xf9, 0x36, 0x5e, 0x2b, 0xb3, 0xb2, 0xb6, 0xce, 0xa0, 0xe7, 0x30, 0x96, 0xb1, 0xff, 0x1c, + 0xd9, 0x37, 0x38, 0x59, 0xa5, 0x8c, 0xc2, 0x2c, 0x4a, 0xe3, 0x9f, 0xb4, 0xbe, 0x2a, 0x65, 0x2b, + 0xe6, 0x0d, 0xf4, 0xa8, 0x2c, 0x24, 0x61, 0xcc, 0x9e, 0x34, 0x9a, 0x94, 0x98, 0x5b, 0x41, 0xfb, + 0xf0, 0xda, 0xb5, 0xf0, 0x9e, 0xcd, 0xa1, 0xbf, 0xdb, 0x0a, 0x1a, 0x70, 0xf4, 0xde, 0xf9, 0x30, + 0x5f, 0x5d, 0x7e, 0x19, 0xb5, 0x10, 0x40, 0x77, 0x9d, 0xc5, 0xdc, 0x73, 0x46, 0x1a, 0xde, 0x87, + 0x63, 0xef, 0x6a, 0x35, 0xf7, 0x3e, 0x5e, 0xab, 0xa5, 0x36, 0x0e, 0xa0, 0xf7, 0xc9, 0x71, 0x2f, + 0x9c, 0x51, 0x67, 0xf6, 0x47, 0x03, 0xf4, 0xa4, 0xba, 0xf4, 0x7a, 0x51, 0x89, 0x23, 0x41, 0xb7, + 0xbc, 0x79, 0xf8, 0xa2, 0xd1, 0x66, 0xed, 0x2a, 0x9f, 0xbe, 0x3c, 0x70, 0xba, 0xca, 0xd8, 0x6a, + 0x95, 0x32, 0x65, 0xea, 0x07, 0xc8, 0xd4, 0x6e, 0xd2, 0x01, 0x32, 0xf5, 0xa3, 0xb4, 0x5a, 0x8b, + 0x00, 0x1e, 0x87, 0x59, 0xd2, 0x44, 0x2d, 0x86, 0x6a, 0xf7, 0xcb, 0xf2, 0x1d, 0x2f, 0xb5, 0xaf, + 0xcf, 0xa3, 0x58, 0x6c, 0x8a, 0xc0, 0x0e, 0xb3, 0x64, 0x5a, 0xb2, 0xd3, 0x1a, 0x34, 0x55, 0xd0, + 0x54, 0x3e, 0xfa, 0x3c, 0x08, 0x74, 0x59, 0xbc, 0xfa, 0x1b, 0x00, 0x00, 0xff, 0xff, 0x5f, 0x50, + 0x90, 0x90, 0x0e, 0x04, 0x00, 0x00, }, } diff --git a/orchestrator/controller/request/request.go b/orchestrator/controller/request/request.go index 3b6f9a9e..1cc0b1d6 100644 --- a/orchestrator/controller/request/request.go +++ b/orchestrator/controller/request/request.go @@ -70,7 +70,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er "state", string(request.State), "land_strategy", string(request.LandStrategy), "change_source", request.Change.Source, - "change_ids", request.Change.IDs, + "change_uris", request.Change.URIs, "version", request.Version, "attempt", delivery.Attempt(), "partition_key", msg.PartitionKey, diff --git a/orchestrator/controller/request/request_test.go b/orchestrator/controller/request/request_test.go index 1752d192..e822823e 100644 --- a/orchestrator/controller/request/request_test.go +++ b/orchestrator/controller/request/request_test.go @@ -57,7 +57,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", IDs: []string{"PR-456"}}, + Change: entity.Change{Source: "github", URIs: []string{"uber/service/456@abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -115,7 +115,7 @@ func TestController_Process_AllRequestStates(t *testing.T) { request := entity.Request{ ID: fmt.Sprintf("queue/%s", tt.state), Queue: "test-queue", - Change: entity.Change{Source: "github", IDs: []string{"PR-1"}}, + Change: entity.Change{Source: "github", URIs: []string{"uber/service/1@aaa111bbb"}}, LandStrategy: tt.strategy, State: tt.state, Version: 1, @@ -145,7 +145,7 @@ func TestController_Process_MultipleChanges(t *testing.T) { Queue: "test-queue", Change: entity.Change{ Source: "github", - IDs: []string{"PR-1", "PR-2", "PR-3"}, + URIs: []string{"uber/monorepo/1@aaa111", "uber/monorepo/2@bbb222", "uber/monorepo/3@ccc333"}, }, LandStrategy: entity.RequestLandStrategySquashRebase, State: entity.RequestStateNew, @@ -172,7 +172,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", IDs: []string{"PR-1"}}, + Change: entity.Change{Source: "github", URIs: []string{"uber/service/1@xyz789abc"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/test/e2e/suite_test.go b/test/e2e/suite_test.go index 97539b4e..eab334aa 100644 --- a/test/e2e/suite_test.go +++ b/test/e2e/suite_test.go @@ -114,7 +114,7 @@ func (s *E2EIntegrationSuite) TestPingOrchestrator() { func (s *E2EIntegrationSuite) TestLandRequest() { req := &gatewaypb.LandRequest{ Queue: "e2e-test-queue", - Change: &gatewaypb.Change{Source: "github", Ids: []string{"pr-100", "pr-101"}}, + Change: &gatewaypb.Change{Source: "github", Uris: []string{"uber/e2e-service/100@aaa100bbb", "uber/e2e-service/101@ccc101ddd"}}, Strategy: gatewaypb.Strategy_REBASE, } diff --git a/test/integration/extension/storage/suite.go b/test/integration/extension/storage/suite.go index 4a7df35b..e46a00cb 100644 --- a/test/integration/extension/storage/suite.go +++ b/test/integration/extension/storage/suite.go @@ -47,7 +47,7 @@ func (s *StorageContractSuite) TestStorage_CreateAndGet() { State: entity.RequestStateNew, Change: entity.Change{ Source: "github", - IDs: []string{"123"}, + URIs: []string{"uber/storage-test/123@abc123def"}, }, LandStrategy: entity.RequestLandStrategyMerge, Version: 1, @@ -66,7 +66,7 @@ func (s *StorageContractSuite) TestStorage_CreateAndGet() { assert.Equal(t, request.Queue, retrieved.Queue) assert.Equal(t, request.State, retrieved.State) assert.Equal(t, request.Change.Source, retrieved.Change.Source) - assert.Equal(t, request.Change.IDs, retrieved.Change.IDs) + assert.Equal(t, request.Change.URIs, retrieved.Change.URIs) assert.Equal(t, request.LandStrategy, retrieved.LandStrategy) assert.Equal(t, request.Version, retrieved.Version) diff --git a/test/integration/gateway/suite_test.go b/test/integration/gateway/suite_test.go index c16cf931..9740f941 100644 --- a/test/integration/gateway/suite_test.go +++ b/test/integration/gateway/suite_test.go @@ -111,7 +111,7 @@ func (s *GatewayIntegrationSuite) TestLandAPI() { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Ids: []string{"PR-123"}}, + Change: &pb.Change{Source: "github", Uris: []string{"uber/integration-test/123@abc123def"}}, Strategy: pb.Strategy_REBASE, } From 1285c4c0438899a6bb1e23e0cc09e843c2191b12 Mon Sep 17 00:00:00 2001 From: rprithyani Date: Wed, 25 Feb 2026 00:29:36 +0000 Subject: [PATCH 2/5] fix tests --- orchestrator/controller/batch/batch_test.go | 4 ++-- orchestrator/controller/build/build_test.go | 4 ++-- orchestrator/controller/buildsignal/buildsignal_test.go | 2 +- orchestrator/controller/finalize/finalize_test.go | 2 +- orchestrator/controller/merge/merge_test.go | 4 ++-- orchestrator/controller/mergesignal/mergesignal_test.go | 2 +- orchestrator/controller/speculate/speculate_test.go | 4 ++-- 7 files changed, 11 insertions(+), 11 deletions(-) diff --git a/orchestrator/controller/batch/batch_test.go b/orchestrator/controller/batch/batch_test.go index 2383e6db..49a47932 100644 --- a/orchestrator/controller/batch/batch_test.go +++ b/orchestrator/controller/batch/batch_test.go @@ -57,7 +57,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", IDs: []string{"PR-456"}}, + Change: entity.Change{Source: "github", URIs: []string{"uber/service/456@abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -100,7 +100,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", IDs: []string{"PR-1"}}, + Change: entity.Change{Source: "github", URIs: []string{"uber/service/1@xyz789abc"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/build/build_test.go b/orchestrator/controller/build/build_test.go index fa7b8d36..caaee83d 100644 --- a/orchestrator/controller/build/build_test.go +++ b/orchestrator/controller/build/build_test.go @@ -57,7 +57,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", IDs: []string{"PR-456"}}, + Change: entity.Change{Source: "github", URIs: []string{"uber/service/456@abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -100,7 +100,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", IDs: []string{"PR-1"}}, + Change: entity.Change{Source: "github", URIs: []string{"uber/service/1@xyz789abc"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/buildsignal/buildsignal_test.go b/orchestrator/controller/buildsignal/buildsignal_test.go index 41792c59..5352c5e2 100644 --- a/orchestrator/controller/buildsignal/buildsignal_test.go +++ b/orchestrator/controller/buildsignal/buildsignal_test.go @@ -42,7 +42,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", IDs: []string{"PR-456"}}, + Change: entity.Change{Source: "github", URIs: []string{"uber/service/456@abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/finalize/finalize_test.go b/orchestrator/controller/finalize/finalize_test.go index 609c0011..7482629e 100644 --- a/orchestrator/controller/finalize/finalize_test.go +++ b/orchestrator/controller/finalize/finalize_test.go @@ -42,7 +42,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", IDs: []string{"PR-456"}}, + Change: entity.Change{Source: "github", URIs: []string{"uber/service/456@abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/merge/merge_test.go b/orchestrator/controller/merge/merge_test.go index efbe4146..3d7846ed 100644 --- a/orchestrator/controller/merge/merge_test.go +++ b/orchestrator/controller/merge/merge_test.go @@ -57,7 +57,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", IDs: []string{"PR-456"}}, + Change: entity.Change{Source: "github", URIs: []string{"uber/service/456@abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -100,7 +100,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", IDs: []string{"PR-1"}}, + Change: entity.Change{Source: "github", URIs: []string{"uber/service/1@xyz789abc"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/mergesignal/mergesignal_test.go b/orchestrator/controller/mergesignal/mergesignal_test.go index 228d4f42..b901f563 100644 --- a/orchestrator/controller/mergesignal/mergesignal_test.go +++ b/orchestrator/controller/mergesignal/mergesignal_test.go @@ -42,7 +42,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", IDs: []string{"PR-456"}}, + Change: entity.Change{Source: "github", URIs: []string{"uber/service/456@abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/speculate/speculate_test.go b/orchestrator/controller/speculate/speculate_test.go index 56e38de3..433c048c 100644 --- a/orchestrator/controller/speculate/speculate_test.go +++ b/orchestrator/controller/speculate/speculate_test.go @@ -60,7 +60,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", IDs: []string{"PR-456"}}, + Change: entity.Change{Source: "github", URIs: []string{"uber/service/456@abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -103,7 +103,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", IDs: []string{"PR-1"}}, + Change: entity.Change{Source: "github", URIs: []string{"uber/service/1@xyz789abc"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, From 802eae0f37bf8d78a7b7c87dc264f8431c0af2b6 Mon Sep 17 00:00:00 2001 From: rprithyani Date: Wed, 25 Feb 2026 02:38:21 +0000 Subject: [PATCH 3/5] code review changes --- entity/request.go | 9 ++-- entity/request_test.go | 14 +++--- extension/change_provider/change_provider.go | 34 -------------- extension/changeprovider/change_provider.go | 45 +++++++++++++++++++ extension/storage/mysql/request_store.go | 4 +- gateway/controller/land.go | 6 +-- gateway/controller/land_test.go | 26 +++++------ gateway/proto/gateway.proto | 5 +-- gateway/protopb/gateway.pb.go | 8 ++-- orchestrator/controller/batch/batch_test.go | 4 +- orchestrator/controller/build/build_test.go | 4 +- .../buildsignal/buildsignal_test.go | 2 +- .../controller/finalize/finalize_test.go | 2 +- orchestrator/controller/merge/merge_test.go | 4 +- .../mergesignal/mergesignal_test.go | 2 +- orchestrator/controller/request/request.go | 2 +- .../controller/request/request_test.go | 10 ++--- .../controller/speculate/speculate_test.go | 4 +- test/e2e/suite_test.go | 2 +- test/integration/extension/storage/suite.go | 6 +-- test/integration/gateway/suite_test.go | 2 +- 21 files changed, 101 insertions(+), 94 deletions(-) delete mode 100644 extension/change_provider/change_provider.go create mode 100644 extension/changeprovider/change_provider.go diff --git a/entity/request.go b/entity/request.go index 5762cfe5..9bc77e85 100644 --- a/entity/request.go +++ b/entity/request.go @@ -36,11 +36,10 @@ const ( // Change represents a set of related code changes identified by one or more URIs from a particular code change provider, like Github Pull Requests. // The object is immutable after creation. type Change struct { - // Source is the code change provider ID that maps to a registered provider (e.g., "github", "github-enterprise", "phabricator"). - Source string `json:"source"` - // URIs is a list of change URIs that should be landed together. The format is provider-specific: - // - GitHub: "owner/repo/pr_number@commit_hash" (e.g., "uber/submitqueue/123@abc123def") - // - Phabricator: "revision_id@commit_hash" (e.g., "D12345@abc123def") + // Provider is the code change provider that maps to a registered provider (e.g., "github", "github-enterprise", "phabricator"). + Provider string `json:"provider"` + // URIs is a list of change URIs that should be landed together. The format is determined by the change-provider implementation. + // Default format: "github.com////" (e.g., "github.com/uber/submitqueue/123/abc123def") URIs []string `json:"uris"` } diff --git a/entity/request_test.go b/entity/request_test.go index 3c5f1d22..29eec367 100644 --- a/entity/request_test.go +++ b/entity/request_test.go @@ -11,7 +11,7 @@ func TestRequest_ToBytes(t *testing.T) { req := Request{ ID: "test-queue/123", Queue: "test-queue", - Change: Change{Source: "github", URIs: []string{"uber/submitqueue/456@abc123def", "uber/submitqueue/789@def456abc"}}, + Change: Change{Provider: "github", URIs: []string{"github.com/uber/submitqueue/456/abc123def", "github.com/uber/submitqueue/789/def456abc"}}, LandStrategy: RequestLandStrategyRebase, State: RequestStateNew, Version: 1, @@ -25,7 +25,7 @@ func TestRequest_ToBytes(t *testing.T) { jsonStr := string(data) assert.Contains(t, jsonStr, "test-queue/123") assert.Contains(t, jsonStr, "github") - assert.Contains(t, jsonStr, "uber/submitqueue/456@abc123def") + assert.Contains(t, jsonStr, "github.com/uber/submitqueue/456/abc123def") assert.Contains(t, jsonStr, "rebase") assert.Contains(t, jsonStr, "new") } @@ -34,7 +34,7 @@ func TestRequestFromBytes(t *testing.T) { original := Request{ ID: "my-queue/999", Queue: "my-queue", - Change: Change{Source: "phabricator", URIs: []string{"D111@fedcba987"}}, + Change: Change{Provider: "phabricator", URIs: []string{"phabricator.uber.com/D111/fedcba987"}}, LandStrategy: RequestLandStrategyMerge, State: RequestStateProcessing, Version: 3, @@ -51,7 +51,7 @@ func TestRequestFromBytes(t *testing.T) { // Verify all fields match assert.Equal(t, original.ID, deserialized.ID) assert.Equal(t, original.Queue, deserialized.Queue) - assert.Equal(t, original.Change.Source, deserialized.Change.Source) + assert.Equal(t, original.Change.Provider, deserialized.Change.Provider) assert.Equal(t, original.Change.URIs, deserialized.Change.URIs) assert.Equal(t, original.LandStrategy, deserialized.LandStrategy) assert.Equal(t, original.State, deserialized.State) @@ -89,7 +89,7 @@ func TestRequest_SerializationRoundTrip(t *testing.T) { req: Request{ ID: "queue1/100", Queue: "queue1", - Change: Change{Source: "github", URIs: []string{"uber/repo-a/101@aaa111", "uber/repo-a/102@bbb222", "uber/repo-a/103@ccc333"}}, + Change: Change{Provider: "github", URIs: []string{"github.com/uber/repo-a/101/aaa111", "github.com/uber/repo-a/102/bbb222", "github.com/uber/repo-a/103/ccc333"}}, LandStrategy: RequestLandStrategySquashRebase, State: RequestStateLanded, Version: 5, @@ -100,7 +100,7 @@ func TestRequest_SerializationRoundTrip(t *testing.T) { req: Request{ ID: "queue2/200", Queue: "queue2", - Change: Change{Source: "phabricator", URIs: []string{"D12345@abc123def456"}}, + Change: Change{Provider: "phabricator", URIs: []string{"phabricator.uber.com/D12345/abc123def456"}}, LandStrategy: RequestLandStrategyRebase, State: RequestStateNew, Version: 1, @@ -111,7 +111,7 @@ func TestRequest_SerializationRoundTrip(t *testing.T) { req: Request{ ID: "queue3/300", Queue: "queue3", - Change: Change{Source: "github-enterprise", URIs: []string{"internal/service/999@deadbeef12"}}, + Change: Change{Provider: "github-enterprise", URIs: []string{"github.uber.com/internal/service/999/deadbeef12"}}, LandStrategy: RequestLandStrategyMerge, State: RequestStateError, Version: 10, diff --git a/extension/change_provider/change_provider.go b/extension/change_provider/change_provider.go deleted file mode 100644 index 11470617..00000000 --- a/extension/change_provider/change_provider.go +++ /dev/null @@ -1,34 +0,0 @@ -package changeprovider - -import "context" - -// FileChangeStatus defines the type of modification made to a file. -type FileChangeStatus string - -const ( - // FileChangeStatusAdded indicates a new file was added. - FileChangeStatusAdded FileChangeStatus = "added" - // FileChangeStatusModified indicates an existing file was modified. - FileChangeStatusModified FileChangeStatus = "modified" - // FileChangeStatusDeleted indicates a file was deleted. - FileChangeStatusDeleted FileChangeStatus = "deleted" -) - -// FileChange represents a single file modification in a change. -type FileChange struct { - // Path is the file path relative to the repository root. - Path string - // Status is the type of change made to the file. - Status FileChangeStatus -} - -// ChangeProvider fetches the list of changed files from external code review providers. -// Each implementation is configured for a specific provider (GitHub, GitLab, Phabricator). -type ChangeProvider interface { - // Get retrieves the list of files changed across all provided URIs. - // The URI format is provider-specific: - // - GitHub: "owner/repo/pr_number@commit_hash" (e.g., "uber/submitqueue/123@abc123def") - // - Phabricator: "revision_id@commit_hash" (e.g., "D12345@abc123def") - // Returns the aggregated list of changed files across all URIs. - Get(ctx context.Context, uris []string) ([]FileChange, error) -} diff --git a/extension/changeprovider/change_provider.go b/extension/changeprovider/change_provider.go new file mode 100644 index 00000000..d95fe460 --- /dev/null +++ b/extension/changeprovider/change_provider.go @@ -0,0 +1,45 @@ +package changeprovider + +import "context" + +// User represents the author of a change. +type User struct { + // Name is the display name of the user. + Name string + // Email is the email address of the user. + Email string +} + +// ChangedFile represents a single file modification in a change. +type ChangedFile struct { + // Path is the file path relative to the repository root. + Path string + // Patch is the diff patch content for this file. + Patch string + // LinesAdded is the number of lines added in this file. + LinesAdded int + // LinesDeleted is the number of lines deleted in this file. + LinesDeleted int + // LinesModified is the number of lines modified in this file. + LinesModified int +} + +// ChangeInfo contains metadata and file changes for a code change. +type ChangeInfo struct { + // ID is the change identifier (e.g., "PR: uber-code/go-code/1" or "diff: uber-code/go-code/D1"). + ID string + // User is the author of the change. + User User + // ChangedFiles is the list of files modified in this change. + ChangedFiles []ChangedFile +} + +// ChangeProvider fetches change information from external code review providers. +// Each implementation is configured for a specific provider (GitHub, GitLab, Phabricator). +type ChangeProvider interface { + // Get retrieves change information for the provided URIs. + // The URI format is determined by the implementation. + // Default format: "github.com////" (e.g., "github.com/uber/submitqueue/123/abc123def") + // Returns the change info containing metadata and file changes. + Get(ctx context.Context, uris []string) (ChangeInfo, error) +} diff --git a/extension/storage/mysql/request_store.go b/extension/storage/mysql/request_store.go index 42b87eba..522b3e55 100644 --- a/extension/storage/mysql/request_store.go +++ b/extension/storage/mysql/request_store.go @@ -30,7 +30,7 @@ func (r *requestStore) Get(ctx context.Context, id string) (entity.Request, erro err := r.db.QueryRowContext(ctx, "SELECT id, queue, change_source, change_uris, land_strategy, state, version FROM request WHERE id = ?", id, - ).Scan(&req.ID, &req.Queue, &req.Change.Source, &changeURIsJSON, &req.LandStrategy, &req.State, &req.Version) + ).Scan(&req.ID, &req.Queue, &req.Change.Provider, &changeURIsJSON, &req.LandStrategy, &req.State, &req.Version) if errors.Is(err, sql.ErrNoRows) { return entity.Request{}, storage.WrapNotFound(err) @@ -55,7 +55,7 @@ func (r *requestStore) Create(ctx context.Context, request entity.Request) error _, err = r.db.ExecContext(ctx, "INSERT INTO request (id, queue, change_source, change_uris, land_strategy, state, version) VALUES (?, ?, ?, ?, ?, ?, ?)", - request.ID, request.Queue, request.Change.Source, changeURIsJSON, request.LandStrategy, request.State, request.Version, + request.ID, request.Queue, request.Change.Provider, changeURIsJSON, request.LandStrategy, request.State, request.Version, ) if err != nil { var mysqlErr *mysql.MySQLError diff --git a/gateway/controller/land.go b/gateway/controller/land.go index 679c1fa5..f96a6ccf 100644 --- a/gateway/controller/land.go +++ b/gateway/controller/land.go @@ -69,8 +69,8 @@ func (c *LandController) Land(ctx context.Context, req *pb.LandRequest) (*pb.Lan } change := entity.Change{ - Source: req.Change.GetSource(), - URIs: req.Change.GetUris(), + Provider: req.Change.GetSource(), + URIs: req.Change.GetUris(), } // TODO: validate that queue is configured. Return error if not. @@ -104,7 +104,7 @@ func (c *LandController) Land(ctx context.Context, req *pb.LandRequest) (*pb.Lan c.logger.Debugw("land request created", "queue", req.Queue, "sqid", request.ID, - "change_source", change.Source, + "change_source", change.Provider, "change_uris", change.URIs, "strategy", string(strategy), ) diff --git a/gateway/controller/land_test.go b/gateway/controller/land_test.go index 6400c17a..4b6d0d7a 100644 --- a/gateway/controller/land_test.go +++ b/gateway/controller/land_test.go @@ -248,7 +248,7 @@ func TestLand_ReturnsSqid(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Uris: []string{"uber/test-repo/123@abc123def"}}, + Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}}, } resp, err := controller.Land(ctx, req) @@ -280,7 +280,7 @@ func TestLand_PassesCorrectParametersToStore(t *testing.T) { req := &pb.LandRequest{ Queue: "my-queue", - Change: &pb.Change{Source: "github", Uris: []string{"uber/myservice/1@abc111", "uber/myservice/2@def222"}}, + Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/myservice/1/abc111", "github.com/uber/myservice/2/def222"}}, Strategy: pb.Strategy_REBASE, } resp, err := controller.Land(ctx, req) @@ -288,8 +288,8 @@ func TestLand_PassesCorrectParametersToStore(t *testing.T) { require.NoError(t, err) assert.Equal(t, "my-queue/42", capturedRequest.ID) assert.Equal(t, "my-queue", capturedRequest.Queue) - assert.Equal(t, "github", capturedRequest.Change.Source) - assert.Equal(t, []string{"uber/myservice/1@abc111", "uber/myservice/2@def222"}, capturedRequest.Change.URIs) + assert.Equal(t, "github", capturedRequest.Change.Provider) + assert.Equal(t, []string{"github.com/uber/myservice/1/abc111", "github.com/uber/myservice/2/def222"}, capturedRequest.Change.URIs) assert.Equal(t, entity.RequestLandStrategyRebase, capturedRequest.LandStrategy) assert.Equal(t, entity.RequestStateNew, capturedRequest.State) assert.Equal(t, int32(1), capturedRequest.Version) @@ -317,7 +317,7 @@ func TestLand_ReturnsErrorOnStorageFailure(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Uris: []string{"uber/test-repo/123@abc123def"}}, + Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}}, } _, err := controller.Land(ctx, req) @@ -345,7 +345,7 @@ func TestLand_ReturnsErrorOnCounterFailure(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Uris: []string{"uber/test-repo/123@abc123def"}}, + Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}}, } _, err := controller.Land(ctx, req) @@ -376,7 +376,7 @@ func TestLand_CounterDomainIncludesQueue(t *testing.T) { req := &pb.LandRequest{ Queue: "my-queue", - Change: &pb.Change{Source: "github", Uris: []string{"uber/test-repo/123@abc123def"}}, + Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}}, } _, err := controller.Land(ctx, req) @@ -398,7 +398,7 @@ func TestLand_ReturnsErrorOnEmptyQueue(t *testing.T) { req := &pb.LandRequest{ Queue: "", - Change: &pb.Change{Source: "github", Uris: []string{"uber/test-repo/123@abc123def"}}, + Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}}, } _, err := controller.Land(ctx, req) @@ -420,7 +420,7 @@ func TestLand_ReturnsErrorOnEmptyChangeSource(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "", Uris: []string{"uber/test-repo/123@abc123def"}}, + Change: &pb.Change{Source: "", Uris: []string{"github.com/uber/test-repo/123/abc123def"}}, } _, err := controller.Land(ctx, req) @@ -495,7 +495,7 @@ func TestLand_PublishesToQueue(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Uris: []string{"uber/backend/456@fed987cba"}}, + Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/backend/456/fed987cba"}}, Strategy: pb.Strategy_REBASE, } resp, err := controller.Land(ctx, req) @@ -513,8 +513,8 @@ func TestLand_PublishesToQueue(t *testing.T) { require.NoError(t, err) assert.Equal(t, "test-queue/123", deserializedReq.ID) assert.Equal(t, "test-queue", deserializedReq.Queue) - assert.Equal(t, "github", deserializedReq.Change.Source) - assert.Equal(t, []string{"uber/backend/456@fed987cba"}, deserializedReq.Change.URIs) + assert.Equal(t, "github", deserializedReq.Change.Provider) + assert.Equal(t, []string{"github.com/uber/backend/456/fed987cba"}, deserializedReq.Change.URIs) assert.Equal(t, entity.RequestLandStrategyRebase, deserializedReq.LandStrategy) assert.Equal(t, entity.RequestStateNew, deserializedReq.State) assert.Equal(t, int32(1), deserializedReq.Version) @@ -538,7 +538,7 @@ func TestLand_ContinuesWhenPublishFails(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Uris: []string{"uber/service/1@abc123def"}}, + Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/service/1/abc123def"}}, } _, err := controller.Land(ctx, req) diff --git a/gateway/proto/gateway.proto b/gateway/proto/gateway.proto index 054f32b0..534923bd 100644 --- a/gateway/proto/gateway.proto +++ b/gateway/proto/gateway.proto @@ -45,9 +45,8 @@ enum Strategy { message Change { // The code change provider ID that maps to a registered provider (e.g., "github", "github-enterprise", "phabricator"). string source = 1; - // List of change URIs that should be landed together. The format is provider-specific: - // - GitHub: "owner/repo/pr_number@commit_hash" (e.g., "uber/submitqueue/123@abc123def") - // - Phabricator: "revision_id@commit_hash" (e.g., "D12345@abc123def") + // List of change URIs that should be landed together. The format is determined by the change-provider implementation. + // Default format: "github.com////" (e.g., "github.com/uber/submitqueue/123/abc123def") // 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 earliest change. diff --git a/gateway/protopb/gateway.pb.go b/gateway/protopb/gateway.pb.go index 9060d36a..75927880 100644 --- a/gateway/protopb/gateway.pb.go +++ b/gateway/protopb/gateway.pb.go @@ -200,12 +200,10 @@ func (x *PingResponse) GetHostname() string { // Change represents a set of related code changes identified by one or more URIs from a particular code change provider, like Github Pull Requests. type Change struct { state protoimpl.MessageState `protogen:"open.v1"` - // The code change provider (e.g., "github", "gerrit", "phabricator"). + // The code change provider ID that maps to a registered provider (e.g., "github", "github-enterprise", "phabricator"). Source string `protobuf:"bytes,1,opt,name=source,proto3" json:"source,omitempty"` - // List of change URIs that should be landed together. The format is provider-specific: - // - GitHub: "owner/repo/pr_number@commit_hash" (e.g., "uber/submitqueue/123@abc123def") - // - Phabricator: "revision_id@commit_hash" (e.g., "D12345@abc123def") - // + // List of change URIs that should be landed together. The format is determined by the change-provider implementation. + // Default format: "github.com////" (e.g., "github.com/uber/submitqueue/123/abc123def") // 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 earliest change. diff --git a/orchestrator/controller/batch/batch_test.go b/orchestrator/controller/batch/batch_test.go index 49a47932..fecea17d 100644 --- a/orchestrator/controller/batch/batch_test.go +++ b/orchestrator/controller/batch/batch_test.go @@ -57,7 +57,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", URIs: []string{"uber/service/456@abc123def"}}, + Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/456/abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -100,7 +100,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", URIs: []string{"uber/service/1@xyz789abc"}}, + Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/1/xyz789abc"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/build/build_test.go b/orchestrator/controller/build/build_test.go index caaee83d..c3d4a1c2 100644 --- a/orchestrator/controller/build/build_test.go +++ b/orchestrator/controller/build/build_test.go @@ -57,7 +57,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", URIs: []string{"uber/service/456@abc123def"}}, + Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/456/abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -100,7 +100,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", URIs: []string{"uber/service/1@xyz789abc"}}, + Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/1/xyz789abc"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/buildsignal/buildsignal_test.go b/orchestrator/controller/buildsignal/buildsignal_test.go index 5352c5e2..9273db1d 100644 --- a/orchestrator/controller/buildsignal/buildsignal_test.go +++ b/orchestrator/controller/buildsignal/buildsignal_test.go @@ -42,7 +42,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", URIs: []string{"uber/service/456@abc123def"}}, + Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/456/abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/finalize/finalize_test.go b/orchestrator/controller/finalize/finalize_test.go index 7482629e..16220c5c 100644 --- a/orchestrator/controller/finalize/finalize_test.go +++ b/orchestrator/controller/finalize/finalize_test.go @@ -42,7 +42,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", URIs: []string{"uber/service/456@abc123def"}}, + Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/456/abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/merge/merge_test.go b/orchestrator/controller/merge/merge_test.go index 3d7846ed..d7f84050 100644 --- a/orchestrator/controller/merge/merge_test.go +++ b/orchestrator/controller/merge/merge_test.go @@ -57,7 +57,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", URIs: []string{"uber/service/456@abc123def"}}, + Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/456/abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -100,7 +100,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", URIs: []string{"uber/service/1@xyz789abc"}}, + Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/1/xyz789abc"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/mergesignal/mergesignal_test.go b/orchestrator/controller/mergesignal/mergesignal_test.go index b901f563..42dc733c 100644 --- a/orchestrator/controller/mergesignal/mergesignal_test.go +++ b/orchestrator/controller/mergesignal/mergesignal_test.go @@ -42,7 +42,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", URIs: []string{"uber/service/456@abc123def"}}, + Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/456/abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/request/request.go b/orchestrator/controller/request/request.go index 1cc0b1d6..6545f7f2 100644 --- a/orchestrator/controller/request/request.go +++ b/orchestrator/controller/request/request.go @@ -69,7 +69,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er "queue", request.Queue, "state", string(request.State), "land_strategy", string(request.LandStrategy), - "change_source", request.Change.Source, + "change_source", request.Change.Provider, "change_uris", request.Change.URIs, "version", request.Version, "attempt", delivery.Attempt(), diff --git a/orchestrator/controller/request/request_test.go b/orchestrator/controller/request/request_test.go index e822823e..0c92fe86 100644 --- a/orchestrator/controller/request/request_test.go +++ b/orchestrator/controller/request/request_test.go @@ -57,7 +57,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", URIs: []string{"uber/service/456@abc123def"}}, + Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/456/abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -115,7 +115,7 @@ func TestController_Process_AllRequestStates(t *testing.T) { request := entity.Request{ ID: fmt.Sprintf("queue/%s", tt.state), Queue: "test-queue", - Change: entity.Change{Source: "github", URIs: []string{"uber/service/1@aaa111bbb"}}, + Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/1/aaa111bbb"}}, LandStrategy: tt.strategy, State: tt.state, Version: 1, @@ -144,8 +144,8 @@ func TestController_Process_MultipleChanges(t *testing.T) { ID: "queue/999", Queue: "test-queue", Change: entity.Change{ - Source: "github", - URIs: []string{"uber/monorepo/1@aaa111", "uber/monorepo/2@bbb222", "uber/monorepo/3@ccc333"}, + Provider: "github", + URIs: []string{"github.com/uber/monorepo/1/aaa111", "github.com/uber/monorepo/2/bbb222", "github.com/uber/monorepo/3/ccc333"}, }, LandStrategy: entity.RequestLandStrategySquashRebase, State: entity.RequestStateNew, @@ -172,7 +172,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", URIs: []string{"uber/service/1@xyz789abc"}}, + Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/1/xyz789abc"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/speculate/speculate_test.go b/orchestrator/controller/speculate/speculate_test.go index 433c048c..78c4f55f 100644 --- a/orchestrator/controller/speculate/speculate_test.go +++ b/orchestrator/controller/speculate/speculate_test.go @@ -60,7 +60,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", URIs: []string{"uber/service/456@abc123def"}}, + Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/456/abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -103,7 +103,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Source: "github", URIs: []string{"uber/service/1@xyz789abc"}}, + Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/1/xyz789abc"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/test/e2e/suite_test.go b/test/e2e/suite_test.go index eab334aa..3eb9676f 100644 --- a/test/e2e/suite_test.go +++ b/test/e2e/suite_test.go @@ -114,7 +114,7 @@ func (s *E2EIntegrationSuite) TestPingOrchestrator() { func (s *E2EIntegrationSuite) TestLandRequest() { req := &gatewaypb.LandRequest{ Queue: "e2e-test-queue", - Change: &gatewaypb.Change{Source: "github", Uris: []string{"uber/e2e-service/100@aaa100bbb", "uber/e2e-service/101@ccc101ddd"}}, + Change: &gatewaypb.Change{Source: "github", Uris: []string{"github.com/uber/e2e-service/100/aaa100bbb", "github.com/uber/e2e-service/101/ccc101ddd"}}, Strategy: gatewaypb.Strategy_REBASE, } diff --git a/test/integration/extension/storage/suite.go b/test/integration/extension/storage/suite.go index e46a00cb..33ddb248 100644 --- a/test/integration/extension/storage/suite.go +++ b/test/integration/extension/storage/suite.go @@ -46,8 +46,8 @@ func (s *StorageContractSuite) TestStorage_CreateAndGet() { Queue: "test-queue", State: entity.RequestStateNew, Change: entity.Change{ - Source: "github", - URIs: []string{"uber/storage-test/123@abc123def"}, + Provider: "github", + URIs: []string{"github.com/uber/storage-test/123/abc123def"}, }, LandStrategy: entity.RequestLandStrategyMerge, Version: 1, @@ -65,7 +65,7 @@ func (s *StorageContractSuite) TestStorage_CreateAndGet() { assert.Equal(t, request.ID, retrieved.ID) assert.Equal(t, request.Queue, retrieved.Queue) assert.Equal(t, request.State, retrieved.State) - assert.Equal(t, request.Change.Source, retrieved.Change.Source) + assert.Equal(t, request.Change.Provider, retrieved.Change.Provider) assert.Equal(t, request.Change.URIs, retrieved.Change.URIs) assert.Equal(t, request.LandStrategy, retrieved.LandStrategy) assert.Equal(t, request.Version, retrieved.Version) diff --git a/test/integration/gateway/suite_test.go b/test/integration/gateway/suite_test.go index 9740f941..133cb541 100644 --- a/test/integration/gateway/suite_test.go +++ b/test/integration/gateway/suite_test.go @@ -111,7 +111,7 @@ func (s *GatewayIntegrationSuite) TestLandAPI() { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Uris: []string{"uber/integration-test/123@abc123def"}}, + Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/integration-test/123/abc123def"}}, Strategy: pb.Strategy_REBASE, } From ac67b09a79dd8865c00a419a6f9195a088c99687 Mon Sep 17 00:00:00 2001 From: rprithyani Date: Wed, 25 Feb 2026 06:31:09 +0000 Subject: [PATCH 4/5] review changes part 2 --- entity/request.go | 18 +++--- entity/request_test.go | 18 +++--- extension/changeprovider/BUILD.bazel | 8 +++ extension/changeprovider/change_provider.go | 13 ++-- extension/storage/mysql/request_store.go | 21 ++----- extension/storage/mysql/schema/request.sql | 3 +- gateway/controller/land.go | 13 ++-- gateway/controller/land_test.go | 48 ++++----------- gateway/proto/gateway.proto | 23 ++++--- gateway/protopb/gateway.pb.go | 41 ++++++------- gateway/protopb/gateway.pb.yarpc.go | 61 +++++++++---------- orchestrator/controller/batch/batch_test.go | 4 +- orchestrator/controller/build/build_test.go | 4 +- .../buildsignal/buildsignal_test.go | 2 +- .../controller/finalize/finalize_test.go | 2 +- orchestrator/controller/merge/merge_test.go | 4 +- .../mergesignal/mergesignal_test.go | 2 +- orchestrator/controller/request/request.go | 3 +- .../controller/request/request_test.go | 9 ++- .../controller/speculate/speculate_test.go | 4 +- test/e2e/suite_test.go | 22 +++++-- test/integration/extension/storage/suite.go | 41 +++++++++++-- test/integration/gateway/suite_test.go | 37 ++++++++++- 23 files changed, 227 insertions(+), 174 deletions(-) create mode 100644 extension/changeprovider/BUILD.bazel diff --git a/entity/request.go b/entity/request.go index 9bc77e85..8ab002e5 100644 --- a/entity/request.go +++ b/entity/request.go @@ -33,14 +33,18 @@ const ( RequestStateError RequestState = "error" ) -// Change represents a set of related code changes identified by one or more URIs from a particular code change provider, like Github Pull Requests. -// The object is immutable after creation. +// Change represents a code change identified by a URI from a code change provider (e.g., GitHub Pull Request, Phabricator Diff). +// The provider is extracted from the URI scheme. The object is immutable after creation. type Change struct { - // Provider is the code change provider that maps to a registered provider (e.g., "github", "github-enterprise", "phabricator"). - Provider string `json:"provider"` - // URIs is a list of change URIs that should be landed together. The format is determined by the change-provider implementation. - // Default format: "github.com////" (e.g., "github.com/uber/submitqueue/123/abc123def") - URIs []string `json:"uris"` + // URI identifies the change(s) to land (RFC 3986 compliant). + // The scheme identifies the change provider, and the path contains provider-specific resource identifiers. + // + // By default, the GitHub format is supported (though other providers can be added): + // Single PR: "github:////pull//" + // Stacked PRs: "github:////pull/////..." + // Example: "github://uber/submitqueue/pull/123/abc123def" + // + URI string `json:"uri"` } // Request defines a request to land (merge into target branch of the source control repository) a set of code changes. diff --git a/entity/request_test.go b/entity/request_test.go index 29eec367..8b410529 100644 --- a/entity/request_test.go +++ b/entity/request_test.go @@ -11,7 +11,7 @@ func TestRequest_ToBytes(t *testing.T) { req := Request{ ID: "test-queue/123", Queue: "test-queue", - Change: Change{Provider: "github", URIs: []string{"github.com/uber/submitqueue/456/abc123def", "github.com/uber/submitqueue/789/def456abc"}}, + Change: Change{URI: "github://uber/submitqueue/pull/456/abc123def/789/def456abc"}, LandStrategy: RequestLandStrategyRebase, State: RequestStateNew, Version: 1, @@ -24,8 +24,7 @@ func TestRequest_ToBytes(t *testing.T) { // Verify JSON contains expected fields jsonStr := string(data) assert.Contains(t, jsonStr, "test-queue/123") - assert.Contains(t, jsonStr, "github") - assert.Contains(t, jsonStr, "github.com/uber/submitqueue/456/abc123def") + assert.Contains(t, jsonStr, "github://uber/submitqueue/pull/456/abc123def/789/def456abc") assert.Contains(t, jsonStr, "rebase") assert.Contains(t, jsonStr, "new") } @@ -34,7 +33,7 @@ func TestRequestFromBytes(t *testing.T) { original := Request{ ID: "my-queue/999", Queue: "my-queue", - Change: Change{Provider: "phabricator", URIs: []string{"phabricator.uber.com/D111/fedcba987"}}, + Change: Change{URI: "code.uber.internal.com/D111"}, LandStrategy: RequestLandStrategyMerge, State: RequestStateProcessing, Version: 3, @@ -51,8 +50,7 @@ func TestRequestFromBytes(t *testing.T) { // Verify all fields match assert.Equal(t, original.ID, deserialized.ID) assert.Equal(t, original.Queue, deserialized.Queue) - assert.Equal(t, original.Change.Provider, deserialized.Change.Provider) - assert.Equal(t, original.Change.URIs, deserialized.Change.URIs) + assert.Equal(t, original.Change.URI, deserialized.Change.URI) assert.Equal(t, original.LandStrategy, deserialized.LandStrategy) assert.Equal(t, original.State, deserialized.State) assert.Equal(t, original.Version, deserialized.Version) @@ -85,11 +83,11 @@ func TestRequest_SerializationRoundTrip(t *testing.T) { req Request }{ { - name: "full request with multiple PRs", + name: "github stacked diff", req: Request{ ID: "queue1/100", Queue: "queue1", - Change: Change{Provider: "github", URIs: []string{"github.com/uber/repo-a/101/aaa111", "github.com/uber/repo-a/102/bbb222", "github.com/uber/repo-a/103/ccc333"}}, + Change: Change{URI: "github://uber/repo-a/pull/101/aaa111/102/bbb222/103/ccc333"}, LandStrategy: RequestLandStrategySquashRebase, State: RequestStateLanded, Version: 5, @@ -100,7 +98,7 @@ func TestRequest_SerializationRoundTrip(t *testing.T) { req: Request{ ID: "queue2/200", Queue: "queue2", - Change: Change{Provider: "phabricator", URIs: []string{"phabricator.uber.com/D12345/abc123def456"}}, + Change: Change{URI: "code.uber.internal.com/D12345"}, LandStrategy: RequestLandStrategyRebase, State: RequestStateNew, Version: 1, @@ -111,7 +109,7 @@ func TestRequest_SerializationRoundTrip(t *testing.T) { req: Request{ ID: "queue3/300", Queue: "queue3", - Change: Change{Provider: "github-enterprise", URIs: []string{"github.uber.com/internal/service/999/deadbeef12"}}, + Change: Change{URI: "github.uber.com/internal/service/999/deadbeef12"}, LandStrategy: RequestLandStrategyMerge, State: RequestStateError, Version: 10, diff --git a/extension/changeprovider/BUILD.bazel b/extension/changeprovider/BUILD.bazel new file mode 100644 index 00000000..a0d94b55 --- /dev/null +++ b/extension/changeprovider/BUILD.bazel @@ -0,0 +1,8 @@ +load("@rules_go//go:def.bzl", "go_library") + +go_library( + name = "changeprovider", + srcs = ["change_provider.go"], + importpath = "github.com/uber/submitqueue/extension/changeprovider", + visibility = ["//visibility:public"], +) diff --git a/extension/changeprovider/change_provider.go b/extension/changeprovider/change_provider.go index d95fe460..c611bfcf 100644 --- a/extension/changeprovider/change_provider.go +++ b/extension/changeprovider/change_provider.go @@ -34,12 +34,15 @@ type ChangeInfo struct { ChangedFiles []ChangedFile } -// ChangeProvider fetches change information from external code review providers. +// ChangeProvider fetches change metadata from external systems // Each implementation is configured for a specific provider (GitHub, GitLab, Phabricator). type ChangeProvider interface { - // Get retrieves change information for the provided URIs. - // The URI format is determined by the implementation. - // Default format: "github.com////" (e.g., "github.com/uber/submitqueue/123/abc123def") + // Get retrieves change information for the provided URI (RFC 3986 compliant). + // The URI scheme identifies the provider, and the path contains provider-specific resource identifiers. + // + // By default, the GitHub format is supported (though other providers can be added): + // Single PR: "github:////pull//" + // Stacked PRs: "github:////pull/////..." // Returns the change info containing metadata and file changes. - Get(ctx context.Context, uris []string) (ChangeInfo, error) + Get(ctx context.Context, uri string) (ChangeInfo, error) } diff --git a/extension/storage/mysql/request_store.go b/extension/storage/mysql/request_store.go index 522b3e55..38c81b21 100644 --- a/extension/storage/mysql/request_store.go +++ b/extension/storage/mysql/request_store.go @@ -3,7 +3,6 @@ package mysql import ( "context" "database/sql" - "encoding/json" "errors" "fmt" @@ -25,12 +24,11 @@ func NewRequestStore(db *sql.DB) storage.RequestStore { // Get retrieves a land request by ID. Returns ErrNotFound if the request is not found. func (r *requestStore) Get(ctx context.Context, id string) (entity.Request, error) { var req entity.Request - var changeURIsJSON []byte err := r.db.QueryRowContext(ctx, - "SELECT id, queue, change_source, change_uris, land_strategy, state, version FROM request WHERE id = ?", + "SELECT id, queue, change_uri, land_strategy, state, version FROM request WHERE id = ?", id, - ).Scan(&req.ID, &req.Queue, &req.Change.Provider, &changeURIsJSON, &req.LandStrategy, &req.State, &req.Version) + ).Scan(&req.ID, &req.Queue, &req.Change.URI, &req.LandStrategy, &req.State, &req.Version) if errors.Is(err, sql.ErrNoRows) { return entity.Request{}, storage.WrapNotFound(err) @@ -39,23 +37,14 @@ func (r *requestStore) Get(ctx context.Context, id string) (entity.Request, erro return entity.Request{}, fmt.Errorf("failed to get request entity id=%s from the database: %w", id, err) } - if err := json.Unmarshal(changeURIsJSON, &req.Change.URIs); err != nil { - return entity.Request{}, fmt.Errorf("failed to unmarshal change URIs for request entity id=%s from the database: %w", id, err) - } - return req, nil } // Create creates a new land request. The request must have a unique ID already assigned. Returns ErrAlreadyExists if the request ID already exists. func (r *requestStore) Create(ctx context.Context, request entity.Request) error { - changeURIsJSON, err := json.Marshal(request.Change.URIs) - if err != nil { - return fmt.Errorf("failed to marshal change URIs=%v id=%s for Create request entity: %w", request.Change.URIs, request.ID, err) - } - - _, err = r.db.ExecContext(ctx, - "INSERT INTO request (id, queue, change_source, change_uris, land_strategy, state, version) VALUES (?, ?, ?, ?, ?, ?, ?)", - request.ID, request.Queue, request.Change.Provider, changeURIsJSON, request.LandStrategy, request.State, request.Version, + _, err := r.db.ExecContext(ctx, + "INSERT INTO request (id, queue, change_uri, land_strategy, state, version) VALUES (?, ?, ?, ?, ?, ?)", + request.ID, request.Queue, request.Change.URI, request.LandStrategy, request.State, request.Version, ) if err != nil { var mysqlErr *mysql.MySQLError diff --git a/extension/storage/mysql/schema/request.sql b/extension/storage/mysql/schema/request.sql index 8012909e..444047d8 100644 --- a/extension/storage/mysql/schema/request.sql +++ b/extension/storage/mysql/schema/request.sql @@ -1,8 +1,7 @@ CREATE TABLE IF NOT EXISTS request ( id VARCHAR(255) NOT NULL, queue VARCHAR(255) NOT NULL, - change_source VARCHAR(255) NOT NULL, - change_uris JSON NOT NULL, + change_uri TEXT NOT NULL, land_strategy VARCHAR(64) NOT NULL, state VARCHAR(64) NOT NULL, version INT NOT NULL, diff --git a/gateway/controller/land.go b/gateway/controller/land.go index f96a6ccf..bb624d1d 100644 --- a/gateway/controller/land.go +++ b/gateway/controller/land.go @@ -61,16 +61,12 @@ func (c *LandController) Land(ctx context.Context, req *pb.LandRequest) (*pb.Lan if req.Queue == "" { return nil, fmt.Errorf("LandController requires the request to have a queue name specified: %w", ErrInvalidRequest) } - if req.Change == nil || req.Change.Source == "" { - return nil, fmt.Errorf("LandController requires the request to have a change source specified: %w", ErrInvalidRequest) - } - if len(req.Change.GetUris()) == 0 { - return nil, fmt.Errorf("LandController requires the request to have at least one change URI specified: %w", ErrInvalidRequest) + if req.Change == nil || req.Change.Uri == "" { + return nil, fmt.Errorf("LandController requires the request to have a change URI specified: %w", ErrInvalidRequest) } change := entity.Change{ - Provider: req.Change.GetSource(), - URIs: req.Change.GetUris(), + URI: req.Change.GetUri(), } // TODO: validate that queue is configured. Return error if not. @@ -104,8 +100,7 @@ func (c *LandController) Land(ctx context.Context, req *pb.LandRequest) (*pb.Lan c.logger.Debugw("land request created", "queue", req.Queue, "sqid", request.ID, - "change_source", change.Provider, - "change_uris", change.URIs, + "change_uri", change.URI, "strategy", string(strategy), ) diff --git a/gateway/controller/land_test.go b/gateway/controller/land_test.go index 4b6d0d7a..eb948877 100644 --- a/gateway/controller/land_test.go +++ b/gateway/controller/land_test.go @@ -248,7 +248,7 @@ func TestLand_ReturnsSqid(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}}, + Change: &pb.Change{Uri: "github://uber/test-repo/pull/123/abc123def"}, } resp, err := controller.Land(ctx, req) @@ -280,7 +280,7 @@ func TestLand_PassesCorrectParametersToStore(t *testing.T) { req := &pb.LandRequest{ Queue: "my-queue", - Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/myservice/1/abc111", "github.com/uber/myservice/2/def222"}}, + Change: &pb.Change{Uri: "github://uber/myservice/pull/1/abc111/2/def222"}, Strategy: pb.Strategy_REBASE, } resp, err := controller.Land(ctx, req) @@ -288,8 +288,7 @@ func TestLand_PassesCorrectParametersToStore(t *testing.T) { require.NoError(t, err) assert.Equal(t, "my-queue/42", capturedRequest.ID) assert.Equal(t, "my-queue", capturedRequest.Queue) - assert.Equal(t, "github", capturedRequest.Change.Provider) - assert.Equal(t, []string{"github.com/uber/myservice/1/abc111", "github.com/uber/myservice/2/def222"}, capturedRequest.Change.URIs) + assert.Equal(t, "github://uber/myservice/pull/1/abc111/2/def222", capturedRequest.Change.URI) assert.Equal(t, entity.RequestLandStrategyRebase, capturedRequest.LandStrategy) assert.Equal(t, entity.RequestStateNew, capturedRequest.State) assert.Equal(t, int32(1), capturedRequest.Version) @@ -317,7 +316,7 @@ func TestLand_ReturnsErrorOnStorageFailure(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}}, + Change: &pb.Change{Uri: "github://uber/test-repo/pull/123/abc123def"}, } _, err := controller.Land(ctx, req) @@ -345,7 +344,7 @@ func TestLand_ReturnsErrorOnCounterFailure(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}}, + Change: &pb.Change{Uri: "github://uber/test-repo/pull/123/abc123def"}, } _, err := controller.Land(ctx, req) @@ -376,7 +375,7 @@ func TestLand_CounterDomainIncludesQueue(t *testing.T) { req := &pb.LandRequest{ Queue: "my-queue", - Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}}, + Change: &pb.Change{Uri: "github://uber/test-repo/pull/123/abc123def"}, } _, err := controller.Land(ctx, req) @@ -398,7 +397,7 @@ func TestLand_ReturnsErrorOnEmptyQueue(t *testing.T) { req := &pb.LandRequest{ Queue: "", - Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/test-repo/123/abc123def"}}, + Change: &pb.Change{Uri: "github://uber/test-repo/pull/123/abc123def"}, } _, err := controller.Land(ctx, req) @@ -406,7 +405,7 @@ func TestLand_ReturnsErrorOnEmptyQueue(t *testing.T) { assert.True(t, IsInvalidRequest(err)) } -func TestLand_ReturnsErrorOnEmptyChangeSource(t *testing.T) { +func TestLand_ReturnsErrorOnEmptyChangeUri(t *testing.T) { store := &mockStorage{requestStore: &mockRequestStore{ createFunc: func(ctx context.Context, request entity.Request) error { return nil @@ -420,7 +419,7 @@ func TestLand_ReturnsErrorOnEmptyChangeSource(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "", Uris: []string{"github.com/uber/test-repo/123/abc123def"}}, + Change: &pb.Change{Uri: ""}, } _, err := controller.Land(ctx, req) @@ -450,28 +449,6 @@ func TestLand_ReturnsErrorOnNilChange(t *testing.T) { assert.True(t, IsInvalidRequest(err)) } -func TestLand_ReturnsErrorOnEmptyChangeIDs(t *testing.T) { - store := &mockStorage{requestStore: &mockRequestStore{ - createFunc: func(ctx context.Context, request entity.Request) error { - return nil - }, - }} - cnt := &mockCounter{nextFunc: func(ctx context.Context, domain string) (int64, error) { - return 1, nil - }} - controller := NewLandController(zap.NewNop().Sugar(), tally.NoopScope, store, cnt, noopPublisher(), "request") - ctx := context.Background() - - req := &pb.LandRequest{ - Queue: "test-queue", - Change: &pb.Change{Source: "github", Uris: []string{}}, - } - _, err := controller.Land(ctx, req) - - require.Error(t, err) - assert.True(t, IsInvalidRequest(err)) -} - func TestLand_PublishesToQueue(t *testing.T) { var publishedTopic string var publishedMessage queue.Message @@ -495,7 +472,7 @@ func TestLand_PublishesToQueue(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/backend/456/fed987cba"}}, + Change: &pb.Change{Uri: "github://uber/backend/pull/456/fed987cba"}, Strategy: pb.Strategy_REBASE, } resp, err := controller.Land(ctx, req) @@ -513,8 +490,7 @@ func TestLand_PublishesToQueue(t *testing.T) { require.NoError(t, err) assert.Equal(t, "test-queue/123", deserializedReq.ID) assert.Equal(t, "test-queue", deserializedReq.Queue) - assert.Equal(t, "github", deserializedReq.Change.Provider) - assert.Equal(t, []string{"github.com/uber/backend/456/fed987cba"}, deserializedReq.Change.URIs) + assert.Equal(t, "github://uber/backend/pull/456/fed987cba", deserializedReq.Change.URI) assert.Equal(t, entity.RequestLandStrategyRebase, deserializedReq.LandStrategy) assert.Equal(t, entity.RequestStateNew, deserializedReq.State) assert.Equal(t, int32(1), deserializedReq.Version) @@ -538,7 +514,7 @@ func TestLand_ContinuesWhenPublishFails(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/service/1/abc123def"}}, + Change: &pb.Change{Uri: "github://uber/service/pull/1/abc123def"}, } _, err := controller.Land(ctx, req) diff --git a/gateway/proto/gateway.proto b/gateway/proto/gateway.proto index 534923bd..36fb60ec 100644 --- a/gateway/proto/gateway.proto +++ b/gateway/proto/gateway.proto @@ -41,16 +41,21 @@ enum Strategy { MERGE = 3; } -// Change represents a set of related code changes identified by one or more URIs from a particular code change provider, like Github Pull Requests. +// Change represents a code change identified by a URI from a code change provider (e.g., GitHub Pull Request, Phabricator Diff). +// The provider is extracted from the URI scheme. message Change { - // The code change provider ID that maps to a registered provider (e.g., "github", "github-enterprise", "phabricator"). - string source = 1; - // List of change URIs that should be landed together. The format is determined by the change-provider implementation. - // Default format: "github.com////" (e.g., "github.com/uber/submitqueue/123/abc123def") - // 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 earliest change. - repeated string uris = 2; + // URI identifying the change(s) to land (RFC 3986 compliant). + // The scheme identifies the change provider, and the path contains provider-specific resource identifiers. + // + // By default, the GitHub format is supported (though other providers can be added): + // Single PR: "github:////pull//" + // Stacked PRs: "github:////pull/////..." + // Example: "github://uber/submitqueue/pull/123/abc123def" + // + // SubmitQueue guarantees changes are landed in order with no other changes in between. + // SubmitQueue does not guarantee each change is individually valid, but produces a validity marker on such changes. + // The user is responsible for including all changes in a stack in order, starting from the earliest change. + string uri = 1; } // LandRequest defines a request to land (merge into target branch of the source control repository) a set of code changes. diff --git a/gateway/protopb/gateway.pb.go b/gateway/protopb/gateway.pb.go index 75927880..801ce669 100644 --- a/gateway/protopb/gateway.pb.go +++ b/gateway/protopb/gateway.pb.go @@ -197,17 +197,22 @@ func (x *PingResponse) GetHostname() string { return "" } -// Change represents a set of related code changes identified by one or more URIs from a particular code change provider, like Github Pull Requests. +// Change represents a set of code changes identified by a URI from a code change provider like Github Pull Requests. +// The provider is extracted from the URI hostname. type Change struct { state protoimpl.MessageState `protogen:"open.v1"` - // The code change provider ID that maps to a registered provider (e.g., "github", "github-enterprise", "phabricator"). - Source string `protobuf:"bytes,1,opt,name=source,proto3" json:"source,omitempty"` - // List of change URIs that should be landed together. The format is determined by the change-provider implementation. - // Default format: "github.com////" (e.g., "github.com/uber/submitqueue/123/abc123def") - // 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 earliest change. - Uris []string `protobuf:"bytes,2,rep,name=uris,proto3" json:"uris,omitempty"` + // URI identifying the change(s) to land. The format is provider-specific. + // + // By default we support GitHub format (though other providers can be added): + // + // Single diff: "github.com////" + // Stacked diff: "github.com///////..." + // Example: "github.com/uber/submitqueue/123/abc123def" + // + // SubmitQueue guarantees changes are landed in order with no other changes in between. + // SubmitQueue does not guarantee each change is individually valid, but produces a validity marker on such changes. + // The user is responsible for including all changes in a stack in order, starting from the earliest change. + Uri string `protobuf:"bytes,1,opt,name=uri,proto3" json:"uri,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -242,20 +247,13 @@ func (*Change) Descriptor() ([]byte, []int) { return file_gateway_proto_rawDescGZIP(), []int{2} } -func (x *Change) GetSource() string { +func (x *Change) GetUri() string { if x != nil { - return x.Source + return x.Uri } return "" } -func (x *Change) GetUris() []string { - if x != nil { - return x.Uris - } - return nil -} - // LandRequest defines a request to land (merge into target branch of the source control repository) a set of code changes. type LandRequest struct { state protoimpl.MessageState `protogen:"open.v1"` @@ -478,10 +476,9 @@ const file_gateway_proto_rawDesc = "" + "\amessage\x18\x01 \x01(\tR\amessage\x12!\n" + "\fservice_name\x18\x02 \x01(\tR\vserviceName\x12\x1c\n" + "\ttimestamp\x18\x03 \x01(\x03R\ttimestamp\x12\x1a\n" + - "\bhostname\x18\x04 \x01(\tR\bhostname\"4\n" + - "\x06Change\x12\x16\n" + - "\x06source\x18\x01 \x01(\tR\x06source\x12\x12\n" + - "\x04uris\x18\x02 \x03(\tR\x04uris\"\xab\x01\n" + + "\bhostname\x18\x04 \x01(\tR\bhostname\"\x1a\n" + + "\x06Change\x12\x10\n" + + "\x03uri\x18\x01 \x01(\tR\x03uri\"\xab\x01\n" + "\vLandRequest\x12\x14\n" + "\x05queue\x18\x01 \x01(\tR\x05queue\x12?\n" + "\x06change\x18\x02 \x01(\v2'.uber.devexp.submitqueue.gateway.ChangeR\x06change\x12E\n" + diff --git a/gateway/protopb/gateway.pb.yarpc.go b/gateway/protopb/gateway.pb.yarpc.go index db603ba0..69a2eb70 100644 --- a/gateway/protopb/gateway.pb.yarpc.go +++ b/gateway/protopb/gateway.pb.yarpc.go @@ -283,37 +283,36 @@ var ( var yarpcFileDescriptorClosuref1a937782ebbded5 = [][]byte{ // gateway.proto []byte{ - 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x8c, 0x93, 0x4d, 0x6f, 0xd3, 0x40, - 0x10, 0x86, 0xe3, 0x7c, 0xb8, 0xc9, 0x38, 0x45, 0x61, 0x84, 0x2a, 0xab, 0x42, 0x22, 0x35, 0x12, - 0x0d, 0x5f, 0x8e, 0x14, 0x38, 0x22, 0xa1, 0x04, 0x4c, 0x39, 0x14, 0x94, 0xda, 0xe4, 0xc2, 0xa5, - 0xb2, 0x9d, 0x91, 0x63, 0x09, 0x7f, 0x64, 0x77, 0x5d, 0x28, 0x77, 0x7e, 0x0d, 0xff, 0x8a, 0x5f, - 0x82, 0xbc, 0xde, 0x34, 0xbe, 0x80, 0x73, 0x9b, 0x59, 0xcf, 0xe3, 0xf7, 0xdd, 0x77, 0x77, 0xe1, - 0x38, 0xf2, 0x05, 0x7d, 0xf7, 0x6f, 0xed, 0x9c, 0x65, 0x22, 0xc3, 0x47, 0x45, 0x40, 0xcc, 0x5e, - 0xd3, 0x0d, 0xfd, 0xc8, 0x6d, 0x5e, 0x04, 0x49, 0x2c, 0xb6, 0x05, 0x15, 0x64, 0xab, 0x31, 0xeb, - 0x1c, 0x8c, 0x65, 0x9c, 0x46, 0x2e, 0x6d, 0x0b, 0xe2, 0x02, 0x4d, 0x38, 0x4a, 0x88, 0x73, 0x3f, - 0x22, 0x53, 0x1b, 0x6b, 0x93, 0x81, 0xbb, 0x6b, 0xad, 0x5f, 0x1a, 0x0c, 0xab, 0x49, 0x9e, 0x67, - 0x29, 0xa7, 0x7f, 0x8f, 0xe2, 0x19, 0x0c, 0x39, 0xb1, 0x9b, 0x38, 0xa4, 0xeb, 0xd4, 0x4f, 0xc8, - 0x6c, 0xcb, 0xcf, 0x86, 0x5a, 0xfb, 0xec, 0x27, 0x84, 0x0f, 0x61, 0x20, 0xe2, 0x84, 0xb8, 0xf0, - 0x93, 0xdc, 0xec, 0x8c, 0xb5, 0x49, 0xc7, 0xdd, 0x2f, 0xe0, 0x29, 0xf4, 0x37, 0x19, 0x17, 0x12, - 0xee, 0x4a, 0xf8, 0xae, 0xb7, 0x5e, 0x83, 0xfe, 0x6e, 0xe3, 0xa7, 0x11, 0xe1, 0x09, 0xe8, 0x3c, - 0x2b, 0x58, 0xb8, 0xd3, 0x57, 0x1d, 0x22, 0x74, 0x0b, 0x16, 0x73, 0xb3, 0x3d, 0xee, 0x4c, 0x06, - 0xae, 0xac, 0xad, 0xdf, 0x1a, 0x18, 0x97, 0x7e, 0xba, 0xde, 0xed, 0xf3, 0x01, 0xf4, 0x64, 0x0e, - 0x0a, 0xad, 0x1a, 0x7c, 0x0b, 0x7a, 0x28, 0xff, 0x2d, 0x2d, 0x1b, 0xb3, 0x73, 0xbb, 0x21, 0x3e, - 0xbb, 0xb2, 0xe2, 0x2a, 0x0c, 0x1d, 0xe8, 0x73, 0xc1, 0x7c, 0x41, 0xd1, 0xad, 0x34, 0x7e, 0x6f, - 0xf6, 0xb4, 0xf1, 0x17, 0x9e, 0x02, 0xdc, 0x3b, 0xd4, 0xb2, 0x60, 0x58, 0x99, 0x55, 0x51, 0x23, - 0x74, 0xf9, 0x36, 0x5e, 0x2b, 0xb3, 0xb2, 0xb6, 0xce, 0xa0, 0xe7, 0x30, 0x96, 0xb1, 0xff, 0x1c, - 0xd9, 0x37, 0x38, 0x59, 0xa5, 0x8c, 0xc2, 0x2c, 0x4a, 0xe3, 0x9f, 0xb4, 0xbe, 0x2a, 0x65, 0x2b, - 0xe6, 0x0d, 0xf4, 0xa8, 0x2c, 0x24, 0x61, 0xcc, 0x9e, 0x34, 0x9a, 0x94, 0x98, 0x5b, 0x41, 0xfb, - 0xf0, 0xda, 0xb5, 0xf0, 0x9e, 0xcd, 0xa1, 0xbf, 0xdb, 0x0a, 0x1a, 0x70, 0xf4, 0xde, 0xf9, 0x30, - 0x5f, 0x5d, 0x7e, 0x19, 0xb5, 0x10, 0x40, 0x77, 0x9d, 0xc5, 0xdc, 0x73, 0x46, 0x1a, 0xde, 0x87, - 0x63, 0xef, 0x6a, 0x35, 0xf7, 0x3e, 0x5e, 0xab, 0xa5, 0x36, 0x0e, 0xa0, 0xf7, 0xc9, 0x71, 0x2f, - 0x9c, 0x51, 0x67, 0xf6, 0x47, 0x03, 0xf4, 0xa4, 0xba, 0xf4, 0x7a, 0x51, 0x89, 0x23, 0x41, 0xb7, - 0xbc, 0x79, 0xf8, 0xa2, 0xd1, 0x66, 0xed, 0x2a, 0x9f, 0xbe, 0x3c, 0x70, 0xba, 0xca, 0xd8, 0x6a, - 0x95, 0x32, 0x65, 0xea, 0x07, 0xc8, 0xd4, 0x6e, 0xd2, 0x01, 0x32, 0xf5, 0xa3, 0xb4, 0x5a, 0x8b, - 0x00, 0x1e, 0x87, 0x59, 0xd2, 0x44, 0x2d, 0x86, 0x6a, 0xf7, 0xcb, 0xf2, 0x1d, 0x2f, 0xb5, 0xaf, - 0xcf, 0xa3, 0x58, 0x6c, 0x8a, 0xc0, 0x0e, 0xb3, 0x64, 0x5a, 0xb2, 0xd3, 0x1a, 0x34, 0x55, 0xd0, - 0x54, 0x3e, 0xfa, 0x3c, 0x08, 0x74, 0x59, 0xbc, 0xfa, 0x1b, 0x00, 0x00, 0xff, 0xff, 0x5f, 0x50, - 0x90, 0x90, 0x0e, 0x04, 0x00, 0x00, + 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x8c, 0x93, 0x5d, 0x6f, 0xd3, 0x30, + 0x14, 0x86, 0x9b, 0x7e, 0xad, 0x3d, 0xe9, 0x50, 0xb1, 0x10, 0xaa, 0x2a, 0x24, 0x36, 0x23, 0xb1, + 0xf1, 0x95, 0x4a, 0xe5, 0x16, 0x09, 0xb5, 0x10, 0xc6, 0xc5, 0x40, 0x5d, 0x42, 0x6f, 0xb8, 0x99, + 0x9c, 0xe4, 0x28, 0x8d, 0x44, 0x3e, 0x6a, 0x3b, 0x83, 0x71, 0xcf, 0xaf, 0xe1, 0x5f, 0xf1, 0x4b, + 0x50, 0x6c, 0x77, 0xcd, 0x0d, 0xa4, 0x77, 0xc7, 0xce, 0x79, 0xf2, 0xbe, 0xe7, 0xb5, 0x0d, 0xc7, + 0x31, 0x93, 0xf8, 0x9d, 0xdd, 0x3a, 0x05, 0xcf, 0x65, 0x4e, 0x1e, 0x97, 0x01, 0x72, 0x27, 0xc2, + 0x1b, 0xfc, 0x51, 0x38, 0xa2, 0x0c, 0xd2, 0x44, 0x6e, 0x4b, 0x2c, 0xd1, 0x31, 0x6d, 0xf4, 0x0c, + 0xec, 0x55, 0x92, 0xc5, 0x1e, 0x6e, 0x4b, 0x14, 0x92, 0x4c, 0xe0, 0x28, 0x45, 0x21, 0x58, 0x8c, + 0x13, 0xeb, 0xc4, 0x3a, 0x1f, 0x7a, 0xbb, 0x25, 0xfd, 0x65, 0xc1, 0x48, 0x77, 0x8a, 0x22, 0xcf, + 0x04, 0xfe, 0xbb, 0x95, 0x9c, 0xc2, 0x48, 0x20, 0xbf, 0x49, 0x42, 0xbc, 0xce, 0x58, 0x8a, 0x93, + 0xb6, 0xfa, 0x6c, 0x9b, 0xbd, 0xcf, 0x2c, 0x45, 0xf2, 0x08, 0x86, 0x32, 0x49, 0x51, 0x48, 0x96, + 0x16, 0x93, 0xce, 0x89, 0x75, 0xde, 0xf1, 0xf6, 0x1b, 0x64, 0x0a, 0x83, 0x4d, 0x2e, 0xa4, 0x82, + 0xbb, 0x0a, 0xbe, 0x5b, 0xd3, 0x29, 0xf4, 0xdf, 0x6d, 0x58, 0x16, 0x23, 0x19, 0x43, 0xa7, 0xe4, + 0x89, 0x11, 0xaf, 0x4a, 0xfa, 0xdb, 0x02, 0xfb, 0x92, 0x65, 0xd1, 0x6e, 0x9a, 0x07, 0xd0, 0x53, + 0xd3, 0x9a, 0x1e, 0xbd, 0x20, 0x6f, 0xa1, 0x1f, 0xaa, 0x3f, 0x28, 0x63, 0xf6, 0xfc, 0xcc, 0x69, + 0x08, 0xc9, 0xd1, 0x82, 0x9e, 0xc1, 0x88, 0x0b, 0x03, 0x21, 0x39, 0x93, 0x18, 0xdf, 0x2a, 0x7b, + 0xf7, 0xe6, 0xcf, 0x1a, 0x7f, 0xe1, 0x1b, 0xc0, 0xbb, 0x43, 0x29, 0x85, 0x91, 0x36, 0x6b, 0x02, + 0x25, 0xd0, 0x15, 0xdb, 0x24, 0x32, 0x66, 0x55, 0x4d, 0x4f, 0xa1, 0xe7, 0x72, 0x9e, 0xf3, 0xff, + 0x1c, 0xcc, 0x37, 0x78, 0xb8, 0xce, 0x38, 0x86, 0x79, 0x9c, 0x25, 0x3f, 0x31, 0xba, 0xaa, 0x64, + 0x35, 0xf3, 0x06, 0x7a, 0x58, 0x15, 0x8a, 0xb0, 0xe7, 0x4f, 0x1b, 0x4d, 0x2a, 0xcc, 0xd3, 0xd0, + 0x3e, 0xbc, 0x76, 0x2d, 0xbc, 0xe7, 0x0b, 0x18, 0xec, 0x46, 0x21, 0x36, 0x1c, 0xbd, 0x77, 0x3f, + 0x2c, 0xd6, 0x97, 0x5f, 0xc6, 0x2d, 0x02, 0xd0, 0xf7, 0xdc, 0xe5, 0xc2, 0x77, 0xc7, 0x16, 0xb9, + 0x0f, 0xc7, 0xfe, 0xd5, 0x7a, 0xe1, 0x7f, 0xbc, 0x36, 0x5b, 0x6d, 0x32, 0x84, 0xde, 0x27, 0xd7, + 0xbb, 0x70, 0xc7, 0x9d, 0xf9, 0x1f, 0x0b, 0x88, 0xaf, 0xd4, 0x95, 0xd7, 0x0b, 0x2d, 0x4e, 0x10, + 0xba, 0xd5, 0xfd, 0x22, 0x2f, 0x1b, 0x6d, 0xd6, 0x2e, 0xec, 0xf4, 0xd5, 0x81, 0xdd, 0x3a, 0x63, + 0xda, 0xaa, 0x64, 0xaa, 0xd4, 0x0f, 0x90, 0xa9, 0xdd, 0xa4, 0x03, 0x64, 0xea, 0x47, 0x49, 0x5b, + 0xcb, 0x00, 0x9e, 0x84, 0x79, 0xda, 0x44, 0x2d, 0x47, 0x66, 0xfa, 0x55, 0xf5, 0x5a, 0x57, 0xd6, + 0xd7, 0x17, 0x71, 0x22, 0x37, 0x65, 0xe0, 0x84, 0x79, 0x3a, 0xab, 0xd8, 0x59, 0x0d, 0x9a, 0x19, + 0x68, 0xa6, 0x9e, 0x76, 0x11, 0x04, 0x7d, 0x55, 0xbc, 0xfe, 0x1b, 0x00, 0x00, 0xff, 0xff, 0xf7, + 0x56, 0x39, 0x09, 0xf4, 0x03, 0x00, 0x00, }, } diff --git a/orchestrator/controller/batch/batch_test.go b/orchestrator/controller/batch/batch_test.go index fecea17d..b8d248f6 100644 --- a/orchestrator/controller/batch/batch_test.go +++ b/orchestrator/controller/batch/batch_test.go @@ -57,7 +57,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/456/abc123def"}}, + Change: entity.Change{URI: "github://uber/service/pull/456/abc123def"}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -100,7 +100,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/1/xyz789abc"}}, + Change: entity.Change{URI: "github://uber/service/pull/1/xyz789abc"}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/build/build_test.go b/orchestrator/controller/build/build_test.go index c3d4a1c2..b3269963 100644 --- a/orchestrator/controller/build/build_test.go +++ b/orchestrator/controller/build/build_test.go @@ -57,7 +57,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/456/abc123def"}}, + Change: entity.Change{URI: "github://uber/service/pull/456/abc123def"}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -100,7 +100,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/1/xyz789abc"}}, + Change: entity.Change{URI: "github://uber/service/pull/1/xyz789abc"}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/buildsignal/buildsignal_test.go b/orchestrator/controller/buildsignal/buildsignal_test.go index 9273db1d..ddffbb0d 100644 --- a/orchestrator/controller/buildsignal/buildsignal_test.go +++ b/orchestrator/controller/buildsignal/buildsignal_test.go @@ -42,7 +42,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/456/abc123def"}}, + Change: entity.Change{URI: "github://uber/service/pull/456/abc123def"}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/finalize/finalize_test.go b/orchestrator/controller/finalize/finalize_test.go index 16220c5c..b80448d8 100644 --- a/orchestrator/controller/finalize/finalize_test.go +++ b/orchestrator/controller/finalize/finalize_test.go @@ -42,7 +42,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/456/abc123def"}}, + Change: entity.Change{URI: "github://uber/service/pull/456/abc123def"}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/merge/merge_test.go b/orchestrator/controller/merge/merge_test.go index d7f84050..4f47519a 100644 --- a/orchestrator/controller/merge/merge_test.go +++ b/orchestrator/controller/merge/merge_test.go @@ -57,7 +57,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/456/abc123def"}}, + Change: entity.Change{URI: "github://uber/service/pull/456/abc123def"}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -100,7 +100,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/1/xyz789abc"}}, + Change: entity.Change{URI: "github://uber/service/pull/1/xyz789abc"}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/mergesignal/mergesignal_test.go b/orchestrator/controller/mergesignal/mergesignal_test.go index 42dc733c..228ba878 100644 --- a/orchestrator/controller/mergesignal/mergesignal_test.go +++ b/orchestrator/controller/mergesignal/mergesignal_test.go @@ -42,7 +42,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/456/abc123def"}}, + Change: entity.Change{URI: "github://uber/service/pull/456/abc123def"}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/request/request.go b/orchestrator/controller/request/request.go index 6545f7f2..8f5dcddf 100644 --- a/orchestrator/controller/request/request.go +++ b/orchestrator/controller/request/request.go @@ -69,8 +69,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er "queue", request.Queue, "state", string(request.State), "land_strategy", string(request.LandStrategy), - "change_source", request.Change.Provider, - "change_uris", request.Change.URIs, + "change_uri", request.Change.URI, "version", request.Version, "attempt", delivery.Attempt(), "partition_key", msg.PartitionKey, diff --git a/orchestrator/controller/request/request_test.go b/orchestrator/controller/request/request_test.go index 0c92fe86..20625cea 100644 --- a/orchestrator/controller/request/request_test.go +++ b/orchestrator/controller/request/request_test.go @@ -57,7 +57,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/456/abc123def"}}, + Change: entity.Change{URI: "github://uber/service/pull/456/abc123def"}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -115,7 +115,7 @@ func TestController_Process_AllRequestStates(t *testing.T) { request := entity.Request{ ID: fmt.Sprintf("queue/%s", tt.state), Queue: "test-queue", - Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/1/aaa111bbb"}}, + Change: entity.Change{URI: "github://uber/service/pull/1/aaa111bbb"}, LandStrategy: tt.strategy, State: tt.state, Version: 1, @@ -144,8 +144,7 @@ func TestController_Process_MultipleChanges(t *testing.T) { ID: "queue/999", Queue: "test-queue", Change: entity.Change{ - Provider: "github", - URIs: []string{"github.com/uber/monorepo/1/aaa111", "github.com/uber/monorepo/2/bbb222", "github.com/uber/monorepo/3/ccc333"}, + URI: "github://uber/monorepo/pull/1/aaa111/2/bbb222/3/ccc333", }, LandStrategy: entity.RequestLandStrategySquashRebase, State: entity.RequestStateNew, @@ -172,7 +171,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/1/xyz789abc"}}, + Change: entity.Change{URI: "github://uber/service/pull/1/xyz789abc"}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/speculate/speculate_test.go b/orchestrator/controller/speculate/speculate_test.go index 78c4f55f..0ebf8fc8 100644 --- a/orchestrator/controller/speculate/speculate_test.go +++ b/orchestrator/controller/speculate/speculate_test.go @@ -60,7 +60,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/456/abc123def"}}, + Change: entity.Change{URI: "github://uber/service/pull/456/abc123def"}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -103,7 +103,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{Provider: "github", URIs: []string{"github.com/uber/service/1/xyz789abc"}}, + Change: entity.Change{URI: "github://uber/service/pull/1/xyz789abc"}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/test/e2e/suite_test.go b/test/e2e/suite_test.go index 3eb9676f..261eeb75 100644 --- a/test/e2e/suite_test.go +++ b/test/e2e/suite_test.go @@ -111,16 +111,30 @@ func (s *E2EIntegrationSuite) TestPingOrchestrator() { s.log.Logf("Orchestrator ping: %s", resp.Message) } -func (s *E2EIntegrationSuite) TestLandRequest() { +func (s *E2EIntegrationSuite) TestLandRequest_SinglePR() { req := &gatewaypb.LandRequest{ Queue: "e2e-test-queue", - Change: &gatewaypb.Change{Source: "github", Uris: []string{"github.com/uber/e2e-service/100/aaa100bbb", "github.com/uber/e2e-service/101/ccc101ddd"}}, + Change: &gatewaypb.Change{Uri: "github://uber/e2e-service/pull/123/abc123def"}, Strategy: gatewaypb.Strategy_REBASE, } - s.log.Logf("Sending Land request for queue=%s", req.Queue) + s.log.Logf("Sending Land request (single PR) for queue=%s", req.Queue) resp, err := s.gatewayClient.Land(s.ctx, req) require.NoError(s.T(), err, "Land request failed") require.NotEmpty(s.T(), resp.Sqid, "SQID should not be empty") - s.log.Logf("Land request succeeded: sqid=%s", resp.Sqid) + s.log.Logf("Land request (single PR) succeeded: sqid=%s", resp.Sqid) +} + +func (s *E2EIntegrationSuite) TestLandRequest_StackedPRs() { + req := &gatewaypb.LandRequest{ + Queue: "e2e-test-queue", + Change: &gatewaypb.Change{Uri: "github://uber/e2e-service/pull/100/aaa100bbb/101/ccc101ddd/102/eee102fff"}, + Strategy: gatewaypb.Strategy_SQUASH_REBASE, + } + + s.log.Logf("Sending Land request (stacked PRs) for queue=%s", req.Queue) + resp, err := s.gatewayClient.Land(s.ctx, req) + require.NoError(s.T(), err, "Land request with stacked PRs failed") + require.NotEmpty(s.T(), resp.Sqid, "SQID should not be empty") + s.log.Logf("Land request (stacked PRs) succeeded: sqid=%s", resp.Sqid) } diff --git a/test/integration/extension/storage/suite.go b/test/integration/extension/storage/suite.go index 33ddb248..3290ff40 100644 --- a/test/integration/extension/storage/suite.go +++ b/test/integration/extension/storage/suite.go @@ -46,8 +46,7 @@ func (s *StorageContractSuite) TestStorage_CreateAndGet() { Queue: "test-queue", State: entity.RequestStateNew, Change: entity.Change{ - Provider: "github", - URIs: []string{"github.com/uber/storage-test/123/abc123def"}, + URI: "github://uber/storage-test/pull/123/abc123def", }, LandStrategy: entity.RequestLandStrategyMerge, Version: 1, @@ -65,14 +64,48 @@ func (s *StorageContractSuite) TestStorage_CreateAndGet() { assert.Equal(t, request.ID, retrieved.ID) assert.Equal(t, request.Queue, retrieved.Queue) assert.Equal(t, request.State, retrieved.State) - assert.Equal(t, request.Change.Provider, retrieved.Change.Provider) - assert.Equal(t, request.Change.URIs, retrieved.Change.URIs) + assert.Equal(t, request.Change.URI, retrieved.Change.URI) assert.Equal(t, request.LandStrategy, retrieved.LandStrategy) assert.Equal(t, request.Version, retrieved.Version) s.log.Logf("CreateAndGet test passed: created and retrieved request %s", request.ID) } +// TestStorage_CreateAndGet_StackedPRs tests creating and retrieving a request with stacked PRs +func (s *StorageContractSuite) TestStorage_CreateAndGet_StackedPRs() { + t := s.T() + ctx := s.ctx + + // Stacked PR URI with multiple PRs encoded in the path + stackedURI := "github://uber/monorepo/pull/101/aaa111bbb/102/ccc222ddd/103/eee333fff/104/ggg444hhh" + + request := entity.Request{ + ID: "test/stacked-prs", + Queue: "test-queue", + State: entity.RequestStateNew, + Change: entity.Change{ + URI: stackedURI, + }, + LandStrategy: entity.RequestLandStrategySquashRebase, + Version: 1, + } + + // Create request + err := s.storage.GetRequestStore().Create(ctx, request) + require.NoError(t, err, "failed to create request with stacked PRs") + + // Get request back + retrieved, err := s.storage.GetRequestStore().Get(ctx, request.ID) + require.NoError(t, err, "failed to get request with stacked PRs") + + // Verify the full stacked URI is preserved + assert.Equal(t, stackedURI, retrieved.Change.URI, "stacked PR URI should be preserved exactly") + assert.Equal(t, request.ID, retrieved.ID) + assert.Equal(t, request.LandStrategy, retrieved.LandStrategy) + + s.log.Logf("CreateAndGet_StackedPRs test passed: stacked URI length=%d", len(stackedURI)) +} + // TestStorage_UpdateState tests updating request state func (s *StorageContractSuite) TestStorage_UpdateState() { t := s.T() diff --git a/test/integration/gateway/suite_test.go b/test/integration/gateway/suite_test.go index 133cb541..f27740f7 100644 --- a/test/integration/gateway/suite_test.go +++ b/test/integration/gateway/suite_test.go @@ -111,7 +111,7 @@ func (s *GatewayIntegrationSuite) TestLandAPI() { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Source: "github", Uris: []string{"github.com/uber/integration-test/123/abc123def"}}, + Change: &pb.Change{Uri: "github://uber/integration-test/pull/123/abc123def"}, Strategy: pb.Strategy_REBASE, } @@ -136,3 +136,38 @@ func (s *GatewayIntegrationSuite) TestLandAPI() { s.log.Logf("Land API test passed: request stored and message published") } + +// TestLandAPI_StackedPRs tests the Gateway Land API with stacked PRs +func (s *GatewayIntegrationSuite) TestLandAPI_StackedPRs() { + t := s.T() + + // Stacked PR URI with multiple PRs encoded in the path + stackedURI := "github://uber/integration-test/pull/100/aaa100bbb/101/ccc101ddd/102/eee102fff" + + req := &pb.LandRequest{ + Queue: "test-queue", + Change: &pb.Change{Uri: stackedURI}, + Strategy: pb.Strategy_SQUASH_REBASE, + } + + s.log.Logf("Sending Land request with stacked PRs for queue=%s", req.Queue) + resp, err := s.client.Land(s.ctx, req) + require.NoError(t, err, "Land request with stacked PRs failed") + require.NotEmpty(t, resp.Sqid, "SQID should not be empty") + + s.log.Logf("Land request with stacked PRs succeeded: sqid=%s", resp.Sqid) + + // Verify the full stacked URI is stored correctly in database + var storedURI string + err = s.db.QueryRow("SELECT change_uri FROM request WHERE id = ?", resp.Sqid).Scan(&storedURI) + require.NoError(t, err, "failed to query request from database") + assert.Equal(t, stackedURI, storedURI, "stacked PR URI should be stored exactly as provided") + + // Verify message published to queue + var msgCount int + err = s.queueDB.QueryRow("SELECT COUNT(*) FROM queue_messages WHERE id = ?", resp.Sqid).Scan(&msgCount) + require.NoError(t, err, "failed to query queue messages") + assert.Equal(t, 1, msgCount, "should have 1 message in queue") + + s.log.Logf("Land API stacked PRs test passed: URI length=%d", len(stackedURI)) +} From c92e97cf90a62ed966a17f99c1d97f4bbcd2a816 Mon Sep 17 00:00:00 2001 From: rprithyani Date: Wed, 25 Feb 2026 08:32:25 +0000 Subject: [PATCH 5/5] break uri string into uris list --- entity/request.go | 11 ++-- entity/request_test.go | 14 ++--- extension/changeprovider/BUILD.bazel | 1 + extension/changeprovider/change_provider.go | 17 +++--- extension/storage/mysql/request_store.go | 19 +++++- extension/storage/mysql/schema/request.sql | 2 +- gateway/controller/land.go | 9 +-- gateway/controller/land_test.go | 22 +++---- gateway/proto/gateway.proto | 11 ++-- gateway/protopb/gateway.pb.go | 28 ++++----- gateway/protopb/gateway.pb.yarpc.go | 60 +++++++++---------- orchestrator/controller/batch/batch_test.go | 4 +- orchestrator/controller/build/build_test.go | 4 +- .../buildsignal/buildsignal_test.go | 2 +- .../controller/finalize/finalize_test.go | 2 +- orchestrator/controller/merge/merge_test.go | 4 +- .../mergesignal/mergesignal_test.go | 2 +- orchestrator/controller/request/request.go | 3 +- .../controller/request/request_test.go | 12 ++-- .../controller/speculate/speculate_test.go | 4 +- test/e2e/suite_test.go | 16 +---- test/integration/extension/storage/suite.go | 21 ++++--- test/integration/gateway/suite_test.go | 37 +----------- 23 files changed, 139 insertions(+), 166 deletions(-) diff --git a/entity/request.go b/entity/request.go index 8ab002e5..d73604ca 100644 --- a/entity/request.go +++ b/entity/request.go @@ -33,18 +33,17 @@ const ( RequestStateError RequestState = "error" ) -// Change represents a code change identified by a URI from a code change provider (e.g., GitHub Pull Request, Phabricator Diff). +// Change represents a code change identified by URIs from a code change provider (e.g., GitHub Pull Request, Phabricator Diff). // The provider is extracted from the URI scheme. The object is immutable after creation. type Change struct { - // URI identifies the change(s) to land (RFC 3986 compliant). + // URIs identifies the change(s) to land (RFC 3986 compliant). // The scheme identifies the change provider, and the path contains provider-specific resource identifiers. // - // By default, the GitHub format is supported (though other providers can be added): - // Single PR: "github:////pull//" - // Stacked PRs: "github:////pull/////..." + // GitHub is supported by default (though other providers can be added): + // Template: "github:////pull//" // Example: "github://uber/submitqueue/pull/123/abc123def" // - URI string `json:"uri"` + URIs []string `json:"uris"` } // Request defines a request to land (merge into target branch of the source control repository) a set of code changes. diff --git a/entity/request_test.go b/entity/request_test.go index 8b410529..d7233ce5 100644 --- a/entity/request_test.go +++ b/entity/request_test.go @@ -11,7 +11,7 @@ func TestRequest_ToBytes(t *testing.T) { req := Request{ ID: "test-queue/123", Queue: "test-queue", - Change: Change{URI: "github://uber/submitqueue/pull/456/abc123def/789/def456abc"}, + Change: Change{URIs: []string{"github://uber/submitqueue/pull/456/abc123def", "github://uber/submitqueue/pull/789/def456abc"}}, LandStrategy: RequestLandStrategyRebase, State: RequestStateNew, Version: 1, @@ -24,7 +24,7 @@ func TestRequest_ToBytes(t *testing.T) { // Verify JSON contains expected fields jsonStr := string(data) assert.Contains(t, jsonStr, "test-queue/123") - assert.Contains(t, jsonStr, "github://uber/submitqueue/pull/456/abc123def/789/def456abc") + assert.Contains(t, jsonStr, "github://uber/submitqueue/pull/456/abc123def") assert.Contains(t, jsonStr, "rebase") assert.Contains(t, jsonStr, "new") } @@ -33,7 +33,7 @@ func TestRequestFromBytes(t *testing.T) { original := Request{ ID: "my-queue/999", Queue: "my-queue", - Change: Change{URI: "code.uber.internal.com/D111"}, + Change: Change{URIs: []string{"code.uber.internal.com/D111"}}, LandStrategy: RequestLandStrategyMerge, State: RequestStateProcessing, Version: 3, @@ -50,7 +50,7 @@ func TestRequestFromBytes(t *testing.T) { // Verify all fields match assert.Equal(t, original.ID, deserialized.ID) assert.Equal(t, original.Queue, deserialized.Queue) - assert.Equal(t, original.Change.URI, deserialized.Change.URI) + assert.Equal(t, original.Change.URIs, deserialized.Change.URIs) assert.Equal(t, original.LandStrategy, deserialized.LandStrategy) assert.Equal(t, original.State, deserialized.State) assert.Equal(t, original.Version, deserialized.Version) @@ -87,7 +87,7 @@ func TestRequest_SerializationRoundTrip(t *testing.T) { req: Request{ ID: "queue1/100", Queue: "queue1", - Change: Change{URI: "github://uber/repo-a/pull/101/aaa111/102/bbb222/103/ccc333"}, + Change: Change{URIs: []string{"github://uber/repo-a/pull/101/aaa111", "github://uber/repo-a/pull/102/bbb222", "github://uber/repo-a/pull/103/ccc333"}}, LandStrategy: RequestLandStrategySquashRebase, State: RequestStateLanded, Version: 5, @@ -98,7 +98,7 @@ func TestRequest_SerializationRoundTrip(t *testing.T) { req: Request{ ID: "queue2/200", Queue: "queue2", - Change: Change{URI: "code.uber.internal.com/D12345"}, + Change: Change{URIs: []string{"code.uber.internal.com/D12345"}}, LandStrategy: RequestLandStrategyRebase, State: RequestStateNew, Version: 1, @@ -109,7 +109,7 @@ func TestRequest_SerializationRoundTrip(t *testing.T) { req: Request{ ID: "queue3/300", Queue: "queue3", - Change: Change{URI: "github.uber.com/internal/service/999/deadbeef12"}, + Change: Change{URIs: []string{"github.uber.com/internal/service/999/deadbeef12"}}, LandStrategy: RequestLandStrategyMerge, State: RequestStateError, Version: 10, diff --git a/extension/changeprovider/BUILD.bazel b/extension/changeprovider/BUILD.bazel index a0d94b55..61b2e095 100644 --- a/extension/changeprovider/BUILD.bazel +++ b/extension/changeprovider/BUILD.bazel @@ -5,4 +5,5 @@ go_library( srcs = ["change_provider.go"], importpath = "github.com/uber/submitqueue/extension/changeprovider", visibility = ["//visibility:public"], + deps = ["//entity"], ) diff --git a/extension/changeprovider/change_provider.go b/extension/changeprovider/change_provider.go index c611bfcf..90cfe635 100644 --- a/extension/changeprovider/change_provider.go +++ b/extension/changeprovider/change_provider.go @@ -1,6 +1,10 @@ package changeprovider -import "context" +import ( + "context" + + "github.com/uber/submitqueue/entity" +) // User represents the author of a change. type User struct { @@ -30,19 +34,14 @@ type ChangeInfo struct { ID string // User is the author of the change. User User - // ChangedFiles is the list of files modified in this change. + // ChangedFiles is the list of files modified in this change. Order is unspecified. ChangedFiles []ChangedFile } // ChangeProvider fetches change metadata from external systems // Each implementation is configured for a specific provider (GitHub, GitLab, Phabricator). type ChangeProvider interface { - // Get retrieves change information for the provided URI (RFC 3986 compliant). - // The URI scheme identifies the provider, and the path contains provider-specific resource identifiers. - // - // By default, the GitHub format is supported (though other providers can be added): - // Single PR: "github:////pull//" - // Stacked PRs: "github:////pull/////..." + // Get retrieves change information for the provided Change. // Returns the change info containing metadata and file changes. - Get(ctx context.Context, uri string) (ChangeInfo, error) + Get(ctx context.Context, change entity.Change) (ChangeInfo, error) } diff --git a/extension/storage/mysql/request_store.go b/extension/storage/mysql/request_store.go index 38c81b21..4937066f 100644 --- a/extension/storage/mysql/request_store.go +++ b/extension/storage/mysql/request_store.go @@ -3,6 +3,7 @@ package mysql import ( "context" "database/sql" + "encoding/json" "errors" "fmt" @@ -24,11 +25,12 @@ func NewRequestStore(db *sql.DB) storage.RequestStore { // Get retrieves a land request by ID. Returns ErrNotFound if the request is not found. func (r *requestStore) Get(ctx context.Context, id string) (entity.Request, error) { var req entity.Request + var changeURIsJSON []byte err := r.db.QueryRowContext(ctx, "SELECT id, queue, change_uri, land_strategy, state, version FROM request WHERE id = ?", id, - ).Scan(&req.ID, &req.Queue, &req.Change.URI, &req.LandStrategy, &req.State, &req.Version) + ).Scan(&req.ID, &req.Queue, &changeURIsJSON, &req.LandStrategy, &req.State, &req.Version) if errors.Is(err, sql.ErrNoRows) { return entity.Request{}, storage.WrapNotFound(err) @@ -37,14 +39,25 @@ func (r *requestStore) Get(ctx context.Context, id string) (entity.Request, erro return entity.Request{}, fmt.Errorf("failed to get request entity id=%s from the database: %w", id, err) } + // Unmarshal the change URIs from JSON + if err := json.Unmarshal(changeURIsJSON, &req.Change.URIs); err != nil { + return entity.Request{}, fmt.Errorf("failed to unmarshal change URIs for request id=%s: %w", id, err) + } + return req, nil } // Create creates a new land request. The request must have a unique ID already assigned. Returns ErrAlreadyExists if the request ID already exists. func (r *requestStore) Create(ctx context.Context, request entity.Request) error { - _, err := r.db.ExecContext(ctx, + // Marshal the change URIs to JSON + changeURIsJSON, err := json.Marshal(request.Change.URIs) + if err != nil { + return fmt.Errorf("failed to marshal change URIs for request id=%s: %w", request.ID, err) + } + + _, err = r.db.ExecContext(ctx, "INSERT INTO request (id, queue, change_uri, land_strategy, state, version) VALUES (?, ?, ?, ?, ?, ?)", - request.ID, request.Queue, request.Change.URI, request.LandStrategy, request.State, request.Version, + request.ID, request.Queue, changeURIsJSON, request.LandStrategy, request.State, request.Version, ) if err != nil { var mysqlErr *mysql.MySQLError diff --git a/extension/storage/mysql/schema/request.sql b/extension/storage/mysql/schema/request.sql index 444047d8..e19a575f 100644 --- a/extension/storage/mysql/schema/request.sql +++ b/extension/storage/mysql/schema/request.sql @@ -1,7 +1,7 @@ CREATE TABLE IF NOT EXISTS request ( id VARCHAR(255) NOT NULL, queue VARCHAR(255) NOT NULL, - change_uri TEXT NOT NULL, + change_uri JSON NOT NULL, land_strategy VARCHAR(64) NOT NULL, state VARCHAR(64) NOT NULL, version INT NOT NULL, diff --git a/gateway/controller/land.go b/gateway/controller/land.go index bb624d1d..9e0f70c4 100644 --- a/gateway/controller/land.go +++ b/gateway/controller/land.go @@ -61,12 +61,12 @@ func (c *LandController) Land(ctx context.Context, req *pb.LandRequest) (*pb.Lan if req.Queue == "" { return nil, fmt.Errorf("LandController requires the request to have a queue name specified: %w", ErrInvalidRequest) } - if req.Change == nil || req.Change.Uri == "" { - return nil, fmt.Errorf("LandController requires the request to have a change URI specified: %w", ErrInvalidRequest) + if req.Change == nil || len(req.Change.Uris) == 0 { + return nil, fmt.Errorf("LandController requires the request to have at least one change URI specified: %w", ErrInvalidRequest) } change := entity.Change{ - URI: req.Change.GetUri(), + URIs: req.Change.GetUris(), } // TODO: validate that queue is configured. Return error if not. @@ -100,7 +100,8 @@ func (c *LandController) Land(ctx context.Context, req *pb.LandRequest) (*pb.Lan c.logger.Debugw("land request created", "queue", req.Queue, "sqid", request.ID, - "change_uri", change.URI, + "change_uris", change.URIs, + "change_count", len(change.URIs), "strategy", string(strategy), ) diff --git a/gateway/controller/land_test.go b/gateway/controller/land_test.go index eb948877..c0f425f9 100644 --- a/gateway/controller/land_test.go +++ b/gateway/controller/land_test.go @@ -248,7 +248,7 @@ func TestLand_ReturnsSqid(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Uri: "github://uber/test-repo/pull/123/abc123def"}, + Change: &pb.Change{Uris: []string{"github://uber/test-repo/pull/123/abc123def"}}, } resp, err := controller.Land(ctx, req) @@ -280,7 +280,7 @@ func TestLand_PassesCorrectParametersToStore(t *testing.T) { req := &pb.LandRequest{ Queue: "my-queue", - Change: &pb.Change{Uri: "github://uber/myservice/pull/1/abc111/2/def222"}, + Change: &pb.Change{Uris: []string{"github://uber/myservice/pull/1/abc111", "github://uber/myservice/pull/2/def222"}}, Strategy: pb.Strategy_REBASE, } resp, err := controller.Land(ctx, req) @@ -288,7 +288,7 @@ func TestLand_PassesCorrectParametersToStore(t *testing.T) { require.NoError(t, err) assert.Equal(t, "my-queue/42", capturedRequest.ID) assert.Equal(t, "my-queue", capturedRequest.Queue) - assert.Equal(t, "github://uber/myservice/pull/1/abc111/2/def222", capturedRequest.Change.URI) + assert.Equal(t, []string{"github://uber/myservice/pull/1/abc111", "github://uber/myservice/pull/2/def222"}, capturedRequest.Change.URIs) assert.Equal(t, entity.RequestLandStrategyRebase, capturedRequest.LandStrategy) assert.Equal(t, entity.RequestStateNew, capturedRequest.State) assert.Equal(t, int32(1), capturedRequest.Version) @@ -316,7 +316,7 @@ func TestLand_ReturnsErrorOnStorageFailure(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Uri: "github://uber/test-repo/pull/123/abc123def"}, + Change: &pb.Change{Uris: []string{"github://uber/test-repo/pull/123/abc123def"}}, } _, err := controller.Land(ctx, req) @@ -344,7 +344,7 @@ func TestLand_ReturnsErrorOnCounterFailure(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Uri: "github://uber/test-repo/pull/123/abc123def"}, + Change: &pb.Change{Uris: []string{"github://uber/test-repo/pull/123/abc123def"}}, } _, err := controller.Land(ctx, req) @@ -375,7 +375,7 @@ func TestLand_CounterDomainIncludesQueue(t *testing.T) { req := &pb.LandRequest{ Queue: "my-queue", - Change: &pb.Change{Uri: "github://uber/test-repo/pull/123/abc123def"}, + Change: &pb.Change{Uris: []string{"github://uber/test-repo/pull/123/abc123def"}}, } _, err := controller.Land(ctx, req) @@ -397,7 +397,7 @@ func TestLand_ReturnsErrorOnEmptyQueue(t *testing.T) { req := &pb.LandRequest{ Queue: "", - Change: &pb.Change{Uri: "github://uber/test-repo/pull/123/abc123def"}, + Change: &pb.Change{Uris: []string{"github://uber/test-repo/pull/123/abc123def"}}, } _, err := controller.Land(ctx, req) @@ -419,7 +419,7 @@ func TestLand_ReturnsErrorOnEmptyChangeUri(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Uri: ""}, + Change: &pb.Change{Uris: []string{}}, } _, err := controller.Land(ctx, req) @@ -472,7 +472,7 @@ func TestLand_PublishesToQueue(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Uri: "github://uber/backend/pull/456/fed987cba"}, + Change: &pb.Change{Uris: []string{"github://uber/backend/pull/456/fed987cba"}}, Strategy: pb.Strategy_REBASE, } resp, err := controller.Land(ctx, req) @@ -490,7 +490,7 @@ func TestLand_PublishesToQueue(t *testing.T) { require.NoError(t, err) assert.Equal(t, "test-queue/123", deserializedReq.ID) assert.Equal(t, "test-queue", deserializedReq.Queue) - assert.Equal(t, "github://uber/backend/pull/456/fed987cba", deserializedReq.Change.URI) + assert.Equal(t, []string{"github://uber/backend/pull/456/fed987cba"}, deserializedReq.Change.URIs) assert.Equal(t, entity.RequestLandStrategyRebase, deserializedReq.LandStrategy) assert.Equal(t, entity.RequestStateNew, deserializedReq.State) assert.Equal(t, int32(1), deserializedReq.Version) @@ -514,7 +514,7 @@ func TestLand_ContinuesWhenPublishFails(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Uri: "github://uber/service/pull/1/abc123def"}, + Change: &pb.Change{Uris: []string{"github://uber/service/pull/1/abc123def"}}, } _, err := controller.Land(ctx, req) diff --git a/gateway/proto/gateway.proto b/gateway/proto/gateway.proto index 36fb60ec..e1495193 100644 --- a/gateway/proto/gateway.proto +++ b/gateway/proto/gateway.proto @@ -41,21 +41,20 @@ enum Strategy { MERGE = 3; } -// Change represents a code change identified by a URI from a code change provider (e.g., GitHub Pull Request, Phabricator Diff). +// Change represents a code change identified by URIs from a code change provider (e.g., GitHub Pull Request, Phabricator Diff). // The provider is extracted from the URI scheme. message Change { - // URI identifying the change(s) to land (RFC 3986 compliant). + // URIs identifying the change(s) to land (RFC 3986 compliant). // The scheme identifies the change provider, and the path contains provider-specific resource identifiers. // - // By default, the GitHub format is supported (though other providers can be added): - // Single PR: "github:////pull//" - // Stacked PRs: "github:////pull/////..." + // GitHub is supported by default (though other providers can be added): + // Template: "github:////pull//" // Example: "github://uber/submitqueue/pull/123/abc123def" // // SubmitQueue guarantees changes are landed in order with no other changes in between. // SubmitQueue does not guarantee each change is individually valid, but produces a validity marker on such changes. // The user is responsible for including all changes in a stack in order, starting from the earliest change. - string uri = 1; + repeated string uris = 1; } // LandRequest defines a request to land (merge into target branch of the source control repository) a set of code changes. diff --git a/gateway/protopb/gateway.pb.go b/gateway/protopb/gateway.pb.go index 801ce669..cfc144d5 100644 --- a/gateway/protopb/gateway.pb.go +++ b/gateway/protopb/gateway.pb.go @@ -197,22 +197,22 @@ func (x *PingResponse) GetHostname() string { return "" } -// Change represents a set of code changes identified by a URI from a code change provider like Github Pull Requests. -// The provider is extracted from the URI hostname. +// Change represents a code change identified by URIs from a code change provider (e.g., GitHub Pull Request, Phabricator Diff). +// The provider is extracted from the URI scheme. type Change struct { state protoimpl.MessageState `protogen:"open.v1"` - // URI identifying the change(s) to land. The format is provider-specific. + // URIs identifying the change(s) to land (RFC 3986 compliant). + // The scheme identifies the change provider, and the path contains provider-specific resource identifiers. // - // By default we support GitHub format (though other providers can be added): + // GitHub is supported by default (though other providers can be added): // - // Single diff: "github.com////" - // Stacked diff: "github.com///////..." - // Example: "github.com/uber/submitqueue/123/abc123def" + // Template: "github:////pull//" + // Example: "github://uber/submitqueue/pull/123/abc123def" // // SubmitQueue guarantees changes are landed in order with no other changes in between. // SubmitQueue does not guarantee each change is individually valid, but produces a validity marker on such changes. // The user is responsible for including all changes in a stack in order, starting from the earliest change. - Uri string `protobuf:"bytes,1,opt,name=uri,proto3" json:"uri,omitempty"` + Uris []string `protobuf:"bytes,1,rep,name=uris,proto3" json:"uris,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -247,11 +247,11 @@ func (*Change) Descriptor() ([]byte, []int) { return file_gateway_proto_rawDescGZIP(), []int{2} } -func (x *Change) GetUri() string { +func (x *Change) GetUris() []string { if x != nil { - return x.Uri + return x.Uris } - return "" + return nil } // LandRequest defines a request to land (merge into target branch of the source control repository) a set of code changes. @@ -476,9 +476,9 @@ const file_gateway_proto_rawDesc = "" + "\amessage\x18\x01 \x01(\tR\amessage\x12!\n" + "\fservice_name\x18\x02 \x01(\tR\vserviceName\x12\x1c\n" + "\ttimestamp\x18\x03 \x01(\x03R\ttimestamp\x12\x1a\n" + - "\bhostname\x18\x04 \x01(\tR\bhostname\"\x1a\n" + - "\x06Change\x12\x10\n" + - "\x03uri\x18\x01 \x01(\tR\x03uri\"\xab\x01\n" + + "\bhostname\x18\x04 \x01(\tR\bhostname\"\x1c\n" + + "\x06Change\x12\x12\n" + + "\x04uris\x18\x01 \x03(\tR\x04uris\"\xab\x01\n" + "\vLandRequest\x12\x14\n" + "\x05queue\x18\x01 \x01(\tR\x05queue\x12?\n" + "\x06change\x18\x02 \x01(\v2'.uber.devexp.submitqueue.gateway.ChangeR\x06change\x12E\n" + diff --git a/gateway/protopb/gateway.pb.yarpc.go b/gateway/protopb/gateway.pb.yarpc.go index 69a2eb70..17760cd6 100644 --- a/gateway/protopb/gateway.pb.yarpc.go +++ b/gateway/protopb/gateway.pb.yarpc.go @@ -283,36 +283,36 @@ var ( var yarpcFileDescriptorClosuref1a937782ebbded5 = [][]byte{ // gateway.proto []byte{ - 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x8c, 0x93, 0x5d, 0x6f, 0xd3, 0x30, - 0x14, 0x86, 0x9b, 0x7e, 0xad, 0x3d, 0xe9, 0x50, 0xb1, 0x10, 0xaa, 0x2a, 0x24, 0x36, 0x23, 0xb1, - 0xf1, 0x95, 0x4a, 0xe5, 0x16, 0x09, 0xb5, 0x10, 0xc6, 0xc5, 0x40, 0x5d, 0x42, 0x6f, 0xb8, 0x99, - 0x9c, 0xe4, 0x28, 0x8d, 0x44, 0x3e, 0x6a, 0x3b, 0x83, 0x71, 0xcf, 0xaf, 0xe1, 0x5f, 0xf1, 0x4b, - 0x50, 0x6c, 0x77, 0xcd, 0x0d, 0xa4, 0x77, 0xc7, 0xce, 0x79, 0xf2, 0xbe, 0xe7, 0xb5, 0x0d, 0xc7, - 0x31, 0x93, 0xf8, 0x9d, 0xdd, 0x3a, 0x05, 0xcf, 0x65, 0x4e, 0x1e, 0x97, 0x01, 0x72, 0x27, 0xc2, - 0x1b, 0xfc, 0x51, 0x38, 0xa2, 0x0c, 0xd2, 0x44, 0x6e, 0x4b, 0x2c, 0xd1, 0x31, 0x6d, 0xf4, 0x0c, - 0xec, 0x55, 0x92, 0xc5, 0x1e, 0x6e, 0x4b, 0x14, 0x92, 0x4c, 0xe0, 0x28, 0x45, 0x21, 0x58, 0x8c, - 0x13, 0xeb, 0xc4, 0x3a, 0x1f, 0x7a, 0xbb, 0x25, 0xfd, 0x65, 0xc1, 0x48, 0x77, 0x8a, 0x22, 0xcf, - 0x04, 0xfe, 0xbb, 0x95, 0x9c, 0xc2, 0x48, 0x20, 0xbf, 0x49, 0x42, 0xbc, 0xce, 0x58, 0x8a, 0x93, - 0xb6, 0xfa, 0x6c, 0x9b, 0xbd, 0xcf, 0x2c, 0x45, 0xf2, 0x08, 0x86, 0x32, 0x49, 0x51, 0x48, 0x96, - 0x16, 0x93, 0xce, 0x89, 0x75, 0xde, 0xf1, 0xf6, 0x1b, 0x64, 0x0a, 0x83, 0x4d, 0x2e, 0xa4, 0x82, - 0xbb, 0x0a, 0xbe, 0x5b, 0xd3, 0x29, 0xf4, 0xdf, 0x6d, 0x58, 0x16, 0x23, 0x19, 0x43, 0xa7, 0xe4, - 0x89, 0x11, 0xaf, 0x4a, 0xfa, 0xdb, 0x02, 0xfb, 0x92, 0x65, 0xd1, 0x6e, 0x9a, 0x07, 0xd0, 0x53, - 0xd3, 0x9a, 0x1e, 0xbd, 0x20, 0x6f, 0xa1, 0x1f, 0xaa, 0x3f, 0x28, 0x63, 0xf6, 0xfc, 0xcc, 0x69, - 0x08, 0xc9, 0xd1, 0x82, 0x9e, 0xc1, 0x88, 0x0b, 0x03, 0x21, 0x39, 0x93, 0x18, 0xdf, 0x2a, 0x7b, - 0xf7, 0xe6, 0xcf, 0x1a, 0x7f, 0xe1, 0x1b, 0xc0, 0xbb, 0x43, 0x29, 0x85, 0x91, 0x36, 0x6b, 0x02, - 0x25, 0xd0, 0x15, 0xdb, 0x24, 0x32, 0x66, 0x55, 0x4d, 0x4f, 0xa1, 0xe7, 0x72, 0x9e, 0xf3, 0xff, - 0x1c, 0xcc, 0x37, 0x78, 0xb8, 0xce, 0x38, 0x86, 0x79, 0x9c, 0x25, 0x3f, 0x31, 0xba, 0xaa, 0x64, - 0x35, 0xf3, 0x06, 0x7a, 0x58, 0x15, 0x8a, 0xb0, 0xe7, 0x4f, 0x1b, 0x4d, 0x2a, 0xcc, 0xd3, 0xd0, - 0x3e, 0xbc, 0x76, 0x2d, 0xbc, 0xe7, 0x0b, 0x18, 0xec, 0x46, 0x21, 0x36, 0x1c, 0xbd, 0x77, 0x3f, - 0x2c, 0xd6, 0x97, 0x5f, 0xc6, 0x2d, 0x02, 0xd0, 0xf7, 0xdc, 0xe5, 0xc2, 0x77, 0xc7, 0x16, 0xb9, - 0x0f, 0xc7, 0xfe, 0xd5, 0x7a, 0xe1, 0x7f, 0xbc, 0x36, 0x5b, 0x6d, 0x32, 0x84, 0xde, 0x27, 0xd7, - 0xbb, 0x70, 0xc7, 0x9d, 0xf9, 0x1f, 0x0b, 0x88, 0xaf, 0xd4, 0x95, 0xd7, 0x0b, 0x2d, 0x4e, 0x10, - 0xba, 0xd5, 0xfd, 0x22, 0x2f, 0x1b, 0x6d, 0xd6, 0x2e, 0xec, 0xf4, 0xd5, 0x81, 0xdd, 0x3a, 0x63, - 0xda, 0xaa, 0x64, 0xaa, 0xd4, 0x0f, 0x90, 0xa9, 0xdd, 0xa4, 0x03, 0x64, 0xea, 0x47, 0x49, 0x5b, - 0xcb, 0x00, 0x9e, 0x84, 0x79, 0xda, 0x44, 0x2d, 0x47, 0x66, 0xfa, 0x55, 0xf5, 0x5a, 0x57, 0xd6, - 0xd7, 0x17, 0x71, 0x22, 0x37, 0x65, 0xe0, 0x84, 0x79, 0x3a, 0xab, 0xd8, 0x59, 0x0d, 0x9a, 0x19, - 0x68, 0xa6, 0x9e, 0x76, 0x11, 0x04, 0x7d, 0x55, 0xbc, 0xfe, 0x1b, 0x00, 0x00, 0xff, 0xff, 0xf7, - 0x56, 0x39, 0x09, 0xf4, 0x03, 0x00, 0x00, + 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x8c, 0x93, 0x5d, 0x6f, 0xd3, 0x3e, + 0x14, 0xc6, 0x9b, 0xbe, 0xad, 0x3d, 0xe9, 0xfe, 0xea, 0xdf, 0x42, 0x28, 0x9a, 0x26, 0xd1, 0x19, + 0x89, 0x95, 0xb7, 0x54, 0x2a, 0xb7, 0x48, 0xa8, 0x85, 0x30, 0x2e, 0x06, 0xea, 0x1c, 0x7a, 0xc3, + 0xcd, 0xe4, 0xa4, 0x47, 0x69, 0x24, 0xf2, 0x52, 0xdb, 0x19, 0x8c, 0x7b, 0x3e, 0x0d, 0xdf, 0x8a, + 0x4f, 0x82, 0xe2, 0xb8, 0x5b, 0x6e, 0x20, 0xbd, 0x3b, 0x76, 0xce, 0x2f, 0xcf, 0x73, 0x1e, 0xdb, + 0x70, 0x1c, 0x71, 0x85, 0xdf, 0xf8, 0xad, 0x9b, 0x8b, 0x4c, 0x65, 0xe4, 0x51, 0x11, 0xa0, 0x70, + 0x37, 0x78, 0x83, 0xdf, 0x73, 0x57, 0x16, 0x41, 0x12, 0xab, 0x5d, 0x81, 0x05, 0xba, 0xa6, 0x8d, + 0x9e, 0x83, 0xbd, 0x8a, 0xd3, 0x88, 0xe1, 0xae, 0x40, 0xa9, 0x88, 0x03, 0x47, 0x09, 0x4a, 0xc9, + 0x23, 0x74, 0xac, 0x89, 0x35, 0x1d, 0xb2, 0xfd, 0x92, 0xfe, 0xb4, 0x60, 0x54, 0x75, 0xca, 0x3c, + 0x4b, 0x25, 0xfe, 0xbd, 0x95, 0x9c, 0xc1, 0x48, 0xa2, 0xb8, 0x89, 0x43, 0xbc, 0x4e, 0x79, 0x82, + 0x4e, 0x5b, 0x7f, 0xb6, 0xcd, 0xde, 0x27, 0x9e, 0x20, 0x39, 0x85, 0xa1, 0x8a, 0x13, 0x94, 0x8a, + 0x27, 0xb9, 0xd3, 0x99, 0x58, 0xd3, 0x0e, 0xbb, 0xdf, 0x20, 0x27, 0x30, 0xd8, 0x66, 0x52, 0x69, + 0xb8, 0xab, 0xe1, 0xbb, 0x35, 0x3d, 0x85, 0xfe, 0xdb, 0x2d, 0x4f, 0x23, 0x24, 0x04, 0xba, 0x85, + 0x88, 0xa5, 0x63, 0x4d, 0x3a, 0xd3, 0x21, 0xd3, 0x35, 0xfd, 0x65, 0x81, 0x7d, 0xc9, 0xd3, 0xcd, + 0x7e, 0x9e, 0x07, 0xd0, 0xd3, 0xf3, 0x1a, 0x8b, 0xd5, 0x82, 0xbc, 0x81, 0x7e, 0xa8, 0xff, 0xa1, + 0xad, 0xd9, 0xf3, 0x73, 0xb7, 0x21, 0x26, 0xb7, 0x92, 0x64, 0x06, 0x23, 0x1e, 0x0c, 0xa4, 0x12, + 0x5c, 0x61, 0x74, 0xab, 0x0d, 0xfe, 0x37, 0x7f, 0xda, 0xf8, 0x0b, 0xdf, 0x00, 0xec, 0x0e, 0xa5, + 0x14, 0x46, 0x95, 0x59, 0x13, 0x29, 0x81, 0xae, 0xdc, 0xc5, 0x1b, 0x63, 0x56, 0xd7, 0xf4, 0x0c, + 0x7a, 0x9e, 0x10, 0x99, 0xf8, 0xc7, 0xd1, 0x7c, 0x85, 0x87, 0xeb, 0x54, 0x60, 0x98, 0x45, 0x69, + 0xfc, 0x03, 0x37, 0x57, 0xa5, 0x6c, 0xc5, 0xbc, 0x86, 0x1e, 0x96, 0x85, 0x26, 0xec, 0xf9, 0x93, + 0x46, 0x93, 0x1a, 0x63, 0x15, 0x74, 0x1f, 0x5e, 0xbb, 0x16, 0xde, 0xb3, 0x05, 0x0c, 0xf6, 0xa3, + 0x10, 0x1b, 0x8e, 0xde, 0x79, 0xef, 0x17, 0xeb, 0xcb, 0xcf, 0xe3, 0x16, 0x01, 0xe8, 0x33, 0x6f, + 0xb9, 0xf0, 0xbd, 0xb1, 0x45, 0xfe, 0x87, 0x63, 0xff, 0x6a, 0xbd, 0xf0, 0x3f, 0x5c, 0x9b, 0xad, + 0x36, 0x19, 0x42, 0xef, 0xa3, 0xc7, 0x2e, 0xbc, 0x71, 0x67, 0xfe, 0xdb, 0x02, 0xe2, 0x6b, 0x75, + 0xed, 0xf5, 0xa2, 0x12, 0x27, 0x08, 0xdd, 0xf2, 0x86, 0x91, 0x17, 0x8d, 0x36, 0x6b, 0x57, 0xf6, + 0xe4, 0xe5, 0x81, 0xdd, 0x55, 0xc6, 0xb4, 0x55, 0xca, 0x94, 0xa9, 0x1f, 0x20, 0x53, 0xbb, 0x49, + 0x07, 0xc8, 0xd4, 0x8f, 0x92, 0xb6, 0x96, 0x01, 0x3c, 0x0e, 0xb3, 0xa4, 0x89, 0x5a, 0x8e, 0xcc, + 0xf4, 0xab, 0xf2, 0xbd, 0xae, 0xac, 0x2f, 0xcf, 0xa3, 0x58, 0x6d, 0x8b, 0xc0, 0x0d, 0xb3, 0x64, + 0x56, 0xb2, 0xb3, 0x1a, 0x34, 0x33, 0xd0, 0x4c, 0x3f, 0xee, 0x3c, 0x08, 0xfa, 0xba, 0x78, 0xf5, + 0x27, 0x00, 0x00, 0xff, 0xff, 0xe8, 0x4d, 0xf1, 0x97, 0xf6, 0x03, 0x00, 0x00, }, } diff --git a/orchestrator/controller/batch/batch_test.go b/orchestrator/controller/batch/batch_test.go index b8d248f6..3b35167f 100644 --- a/orchestrator/controller/batch/batch_test.go +++ b/orchestrator/controller/batch/batch_test.go @@ -57,7 +57,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URI: "github://uber/service/pull/456/abc123def"}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/456/abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -100,7 +100,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URI: "github://uber/service/pull/1/xyz789abc"}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/1/xyz789abc"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/build/build_test.go b/orchestrator/controller/build/build_test.go index b3269963..97fbd67e 100644 --- a/orchestrator/controller/build/build_test.go +++ b/orchestrator/controller/build/build_test.go @@ -57,7 +57,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URI: "github://uber/service/pull/456/abc123def"}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/456/abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -100,7 +100,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URI: "github://uber/service/pull/1/xyz789abc"}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/1/xyz789abc"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/buildsignal/buildsignal_test.go b/orchestrator/controller/buildsignal/buildsignal_test.go index ddffbb0d..b8f4cc6b 100644 --- a/orchestrator/controller/buildsignal/buildsignal_test.go +++ b/orchestrator/controller/buildsignal/buildsignal_test.go @@ -42,7 +42,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URI: "github://uber/service/pull/456/abc123def"}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/456/abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/finalize/finalize_test.go b/orchestrator/controller/finalize/finalize_test.go index b80448d8..1f2fe7a7 100644 --- a/orchestrator/controller/finalize/finalize_test.go +++ b/orchestrator/controller/finalize/finalize_test.go @@ -42,7 +42,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URI: "github://uber/service/pull/456/abc123def"}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/456/abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/merge/merge_test.go b/orchestrator/controller/merge/merge_test.go index 4f47519a..1ff56e54 100644 --- a/orchestrator/controller/merge/merge_test.go +++ b/orchestrator/controller/merge/merge_test.go @@ -57,7 +57,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URI: "github://uber/service/pull/456/abc123def"}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/456/abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -100,7 +100,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URI: "github://uber/service/pull/1/xyz789abc"}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/1/xyz789abc"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/mergesignal/mergesignal_test.go b/orchestrator/controller/mergesignal/mergesignal_test.go index 228ba878..a7f2f06e 100644 --- a/orchestrator/controller/mergesignal/mergesignal_test.go +++ b/orchestrator/controller/mergesignal/mergesignal_test.go @@ -42,7 +42,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URI: "github://uber/service/pull/456/abc123def"}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/456/abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/request/request.go b/orchestrator/controller/request/request.go index 8f5dcddf..71749bae 100644 --- a/orchestrator/controller/request/request.go +++ b/orchestrator/controller/request/request.go @@ -69,7 +69,8 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er "queue", request.Queue, "state", string(request.State), "land_strategy", string(request.LandStrategy), - "change_uri", request.Change.URI, + "change_uris", request.Change.URIs, + "change_count", len(request.Change.URIs), "version", request.Version, "attempt", delivery.Attempt(), "partition_key", msg.PartitionKey, diff --git a/orchestrator/controller/request/request_test.go b/orchestrator/controller/request/request_test.go index 20625cea..e08f1c77 100644 --- a/orchestrator/controller/request/request_test.go +++ b/orchestrator/controller/request/request_test.go @@ -57,7 +57,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URI: "github://uber/service/pull/456/abc123def"}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/456/abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -115,7 +115,7 @@ func TestController_Process_AllRequestStates(t *testing.T) { request := entity.Request{ ID: fmt.Sprintf("queue/%s", tt.state), Queue: "test-queue", - Change: entity.Change{URI: "github://uber/service/pull/1/aaa111bbb"}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/1/aaa111bbb"}}, LandStrategy: tt.strategy, State: tt.state, Version: 1, @@ -144,7 +144,11 @@ func TestController_Process_MultipleChanges(t *testing.T) { ID: "queue/999", Queue: "test-queue", Change: entity.Change{ - URI: "github://uber/monorepo/pull/1/aaa111/2/bbb222/3/ccc333", + URIs: []string{ + "github://uber/monorepo/pull/1/aaa111", + "github://uber/monorepo/pull/2/bbb222", + "github://uber/monorepo/pull/3/ccc333", + }, }, LandStrategy: entity.RequestLandStrategySquashRebase, State: entity.RequestStateNew, @@ -171,7 +175,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URI: "github://uber/service/pull/1/xyz789abc"}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/1/xyz789abc"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/orchestrator/controller/speculate/speculate_test.go b/orchestrator/controller/speculate/speculate_test.go index 0ebf8fc8..452ca70a 100644 --- a/orchestrator/controller/speculate/speculate_test.go +++ b/orchestrator/controller/speculate/speculate_test.go @@ -60,7 +60,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URI: "github://uber/service/pull/456/abc123def"}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/456/abc123def"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, @@ -103,7 +103,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URI: "github://uber/service/pull/1/xyz789abc"}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/1/xyz789abc"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateNew, Version: 1, diff --git a/test/e2e/suite_test.go b/test/e2e/suite_test.go index 261eeb75..5cc7035d 100644 --- a/test/e2e/suite_test.go +++ b/test/e2e/suite_test.go @@ -114,7 +114,7 @@ func (s *E2EIntegrationSuite) TestPingOrchestrator() { func (s *E2EIntegrationSuite) TestLandRequest_SinglePR() { req := &gatewaypb.LandRequest{ Queue: "e2e-test-queue", - Change: &gatewaypb.Change{Uri: "github://uber/e2e-service/pull/123/abc123def"}, + Change: &gatewaypb.Change{Uris: []string{"github://uber/e2e-service/pull/123/abc123def"}}, Strategy: gatewaypb.Strategy_REBASE, } @@ -124,17 +124,3 @@ func (s *E2EIntegrationSuite) TestLandRequest_SinglePR() { require.NotEmpty(s.T(), resp.Sqid, "SQID should not be empty") s.log.Logf("Land request (single PR) succeeded: sqid=%s", resp.Sqid) } - -func (s *E2EIntegrationSuite) TestLandRequest_StackedPRs() { - req := &gatewaypb.LandRequest{ - Queue: "e2e-test-queue", - Change: &gatewaypb.Change{Uri: "github://uber/e2e-service/pull/100/aaa100bbb/101/ccc101ddd/102/eee102fff"}, - Strategy: gatewaypb.Strategy_SQUASH_REBASE, - } - - s.log.Logf("Sending Land request (stacked PRs) for queue=%s", req.Queue) - resp, err := s.gatewayClient.Land(s.ctx, req) - require.NoError(s.T(), err, "Land request with stacked PRs failed") - require.NotEmpty(s.T(), resp.Sqid, "SQID should not be empty") - s.log.Logf("Land request (stacked PRs) succeeded: sqid=%s", resp.Sqid) -} diff --git a/test/integration/extension/storage/suite.go b/test/integration/extension/storage/suite.go index 3290ff40..5771b4dc 100644 --- a/test/integration/extension/storage/suite.go +++ b/test/integration/extension/storage/suite.go @@ -46,7 +46,7 @@ func (s *StorageContractSuite) TestStorage_CreateAndGet() { Queue: "test-queue", State: entity.RequestStateNew, Change: entity.Change{ - URI: "github://uber/storage-test/pull/123/abc123def", + URIs: []string{"github://uber/storage-test/pull/123/abc123def"}, }, LandStrategy: entity.RequestLandStrategyMerge, Version: 1, @@ -64,7 +64,7 @@ func (s *StorageContractSuite) TestStorage_CreateAndGet() { assert.Equal(t, request.ID, retrieved.ID) assert.Equal(t, request.Queue, retrieved.Queue) assert.Equal(t, request.State, retrieved.State) - assert.Equal(t, request.Change.URI, retrieved.Change.URI) + assert.Equal(t, request.Change.URIs, retrieved.Change.URIs) assert.Equal(t, request.LandStrategy, retrieved.LandStrategy) assert.Equal(t, request.Version, retrieved.Version) @@ -76,15 +76,20 @@ func (s *StorageContractSuite) TestStorage_CreateAndGet_StackedPRs() { t := s.T() ctx := s.ctx - // Stacked PR URI with multiple PRs encoded in the path - stackedURI := "github://uber/monorepo/pull/101/aaa111bbb/102/ccc222ddd/103/eee333fff/104/ggg444hhh" + // Stacked PRs as separate URIs + stackedURIs := []string{ + "github://uber/monorepo/pull/101/aaa111bbb", + "github://uber/monorepo/pull/102/ccc222ddd", + "github://uber/monorepo/pull/103/eee333fff", + "github://uber/monorepo/pull/104/ggg444hhh", + } request := entity.Request{ ID: "test/stacked-prs", Queue: "test-queue", State: entity.RequestStateNew, Change: entity.Change{ - URI: stackedURI, + URIs: stackedURIs, }, LandStrategy: entity.RequestLandStrategySquashRebase, Version: 1, @@ -98,12 +103,12 @@ func (s *StorageContractSuite) TestStorage_CreateAndGet_StackedPRs() { retrieved, err := s.storage.GetRequestStore().Get(ctx, request.ID) require.NoError(t, err, "failed to get request with stacked PRs") - // Verify the full stacked URI is preserved - assert.Equal(t, stackedURI, retrieved.Change.URI, "stacked PR URI should be preserved exactly") + // Verify the stacked URIs are preserved + assert.Equal(t, stackedURIs, retrieved.Change.URIs, "stacked PR URIs should be preserved exactly") assert.Equal(t, request.ID, retrieved.ID) assert.Equal(t, request.LandStrategy, retrieved.LandStrategy) - s.log.Logf("CreateAndGet_StackedPRs test passed: stacked URI length=%d", len(stackedURI)) + s.log.Logf("CreateAndGet_StackedPRs test passed: %d stacked URIs", len(stackedURIs)) } // TestStorage_UpdateState tests updating request state diff --git a/test/integration/gateway/suite_test.go b/test/integration/gateway/suite_test.go index f27740f7..d7c65238 100644 --- a/test/integration/gateway/suite_test.go +++ b/test/integration/gateway/suite_test.go @@ -111,7 +111,7 @@ func (s *GatewayIntegrationSuite) TestLandAPI() { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Uri: "github://uber/integration-test/pull/123/abc123def"}, + Change: &pb.Change{Uris: []string{"github://uber/integration-test/pull/123/abc123def"}}, Strategy: pb.Strategy_REBASE, } @@ -136,38 +136,3 @@ func (s *GatewayIntegrationSuite) TestLandAPI() { s.log.Logf("Land API test passed: request stored and message published") } - -// TestLandAPI_StackedPRs tests the Gateway Land API with stacked PRs -func (s *GatewayIntegrationSuite) TestLandAPI_StackedPRs() { - t := s.T() - - // Stacked PR URI with multiple PRs encoded in the path - stackedURI := "github://uber/integration-test/pull/100/aaa100bbb/101/ccc101ddd/102/eee102fff" - - req := &pb.LandRequest{ - Queue: "test-queue", - Change: &pb.Change{Uri: stackedURI}, - Strategy: pb.Strategy_SQUASH_REBASE, - } - - s.log.Logf("Sending Land request with stacked PRs for queue=%s", req.Queue) - resp, err := s.client.Land(s.ctx, req) - require.NoError(t, err, "Land request with stacked PRs failed") - require.NotEmpty(t, resp.Sqid, "SQID should not be empty") - - s.log.Logf("Land request with stacked PRs succeeded: sqid=%s", resp.Sqid) - - // Verify the full stacked URI is stored correctly in database - var storedURI string - err = s.db.QueryRow("SELECT change_uri FROM request WHERE id = ?", resp.Sqid).Scan(&storedURI) - require.NoError(t, err, "failed to query request from database") - assert.Equal(t, stackedURI, storedURI, "stacked PR URI should be stored exactly as provided") - - // Verify message published to queue - var msgCount int - err = s.queueDB.QueryRow("SELECT COUNT(*) FROM queue_messages WHERE id = ?", resp.Sqid).Scan(&msgCount) - require.NoError(t, err, "failed to query queue messages") - assert.Equal(t, 1, msgCount, "should have 1 message in queue") - - s.log.Logf("Land API stacked PRs test passed: URI length=%d", len(stackedURI)) -}