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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions submitqueue/extension/changeprovider/change_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,11 @@ import (
// entity.Author, entity.ChangedFile — live in the entity package so the same typed
// facts can be persisted (entity.ChangeRecord) and scored without re-declaration.
type ChangeProvider interface {
// Get retrieves change information for the provided Change.
// Get retrieves change information for the provided request.
// It is handed the request identity and reads request.Change itself.
// For a Change with multiple URIs (e.g., stacked PRs), returns one ChangeInfo per URI.
// Returns a slice of ChangeInfo, one for each change in the stack.
Get(ctx context.Context, change entity.Change) ([]entity.ChangeInfo, error)
Get(ctx context.Context, request entity.Request) ([]entity.ChangeInfo, error)
}

// Config carries the per-queue identity handed to a Factory. The system knows
Expand Down
7 changes: 4 additions & 3 deletions submitqueue/extension/changeprovider/fake/fake.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,10 @@ func New() changeprovider.ChangeProvider {
return provider{}
}

// Get returns one ChangeInfo per URI in the change, unless a recognized marker
// token requests a failure. The "one ChangeInfo per URI" contract is preserved.
func (provider) Get(_ context.Context, change entity.Change) ([]entity.ChangeInfo, error) {
// Get returns one ChangeInfo per URI in the request's change, unless a recognized
// marker token requests a failure. The "one ChangeInfo per URI" contract is preserved.
func (provider) Get(_ context.Context, request entity.Request) ([]entity.ChangeInfo, error) {
change := request.Change
if fakemarker.Token(change.URIs) == tokenError {
return nil, fmt.Errorf("fake: marked provider error")
}
Expand Down
6 changes: 3 additions & 3 deletions submitqueue/extension/changeprovider/fake/fake_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func TestProvider_Get_OnePerURI(t *testing.T) {
p := New()
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
infos, err := p.Get(context.Background(), entity.Change{URIs: tt.uris})
infos, err := p.Get(context.Background(), entity.Request{Change: entity.Change{URIs: tt.uris}})
require.NoError(t, err)
require.Len(t, infos, len(tt.uris))
for i, uri := range tt.uris {
Expand All @@ -59,8 +59,8 @@ func TestProvider_Get_OnePerURI(t *testing.T) {

func TestProvider_Get_ErrorMarker(t *testing.T) {
p := New()
_, err := p.Get(context.Background(), entity.Change{
_, err := p.Get(context.Background(), entity.Request{Change: entity.Change{
URIs: []string{"github://owner/repo/pull/1/abc?sq-fake=provider-error"},
})
}})
require.Error(t, err)
}
6 changes: 4 additions & 2 deletions submitqueue/extension/changeprovider/github/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,14 @@ func NewProvider(params Params) changeprovider.ChangeProvider {
}
}

// Get retrieves change information from GitHub for the provided Change.
// Get retrieves change information from GitHub for the request's change.
// Returns one ChangeInfo per URI (one per PR in stacked changes).
func (p *provider) Get(ctx context.Context, change entity.Change) (_ []entity.ChangeInfo, retErr error) {
func (p *provider) Get(ctx context.Context, request entity.Request) (_ []entity.ChangeInfo, retErr error) {
op := coremetrics.Begin(p.metricsScope, "get")
defer func() { op.Complete(retErr) }()

change := request.Change

// Parse all change IDs
changeIDs := make([]entitygithub.ChangeID, 0, len(change.URIs))
for _, uri := range change.URIs {
Expand Down
14 changes: 7 additions & 7 deletions submitqueue/extension/changeprovider/github/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func TestProvider_Get(t *testing.T) {
}

p := newTestProvider(t, serverURL)
infos, err := p.Get(context.Background(), entity.Change{URIs: tt.uris})
infos, err := p.Get(context.Background(), entity.Request{Change: entity.Change{URIs: tt.uris}})

if tt.wantErr {
require.Error(t, err)
Expand Down Expand Up @@ -147,9 +147,9 @@ func TestProvider_Get_Pagination(t *testing.T) {
defer server.Close()

p := newTestProvider(t, server.URL)
infos, err := p.Get(context.Background(), entity.Change{
infos, err := p.Get(context.Background(), entity.Request{Change: entity.Change{
URIs: []string{"github://uber/submitqueue/pull/456/" + shaXYZ},
})
}})

require.NoError(t, err)
assert.Equal(t, 2, callCount)
Expand All @@ -170,12 +170,12 @@ func TestProvider_Get_MultiplePRs(t *testing.T) {
defer server.Close()

p := newTestProvider(t, server.URL)
infos, err := p.Get(context.Background(), entity.Change{
infos, err := p.Get(context.Background(), entity.Request{Change: entity.Change{
URIs: []string{
"github://uber/submitqueue/pull/123/" + shaA,
"github://uber/submitqueue/pull/456/" + shaB,
},
})
}})

require.NoError(t, err)
assert.Equal(t, 2, callCount)
Expand All @@ -202,12 +202,12 @@ func TestProvider_Get_FetchError_StopsOnFirstFailure(t *testing.T) {
defer server.Close()

p := newTestProvider(t, server.URL)
_, err := p.Get(context.Background(), entity.Change{
_, err := p.Get(context.Background(), entity.Request{Change: entity.Change{
URIs: []string{
"github://uber/submitqueue/pull/123/" + shaA,
"github://uber/submitqueue/pull/456/" + shaB,
},
})
}})

require.Error(t, err)
assert.Equal(t, 2, callCount)
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion submitqueue/orchestrator/controller/validate/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r
coremetrics.NamedCounter(c.metricsScope, "process", "change_provider_errors", 1)
return fmt.Errorf("failed to build change provider for queue %s: %w", request.Queue, err)
}
changeInfos, err := changeProvider.Get(ctx, request.Change)
changeInfos, err := changeProvider.Get(ctx, request)
if err != nil {
coremetrics.NamedCounter(c.metricsScope, "process", "change_provider_errors", 1)
return fmt.Errorf("failed to fetch change information for request %s: %w", request.ID, err)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func requestIDPayload(t *testing.T, id string) []byte {
// mockChangeProvider is a simple mock that returns test data.
type mockChangeProvider struct{}

func (m *mockChangeProvider) Get(ctx context.Context, change entity.Change) ([]entity.ChangeInfo, error) {
func (m *mockChangeProvider) Get(ctx context.Context, request entity.Request) ([]entity.ChangeInfo, error) {
return []entity.ChangeInfo{
{
URI: "github://org/repo/pull/123/abcdef0123456789abcdef0123456789abcdef01",
Expand Down
Loading