Skip to content
Closed
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
2 changes: 2 additions & 0 deletions MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,12 @@ use_repo(
"com_github_data_dog_go_sqlmock",
"com_github_go_sql_driver_mysql",
"com_github_gogo_protobuf",
"com_github_golang_jwt_jwt_v5",
"com_github_stretchr_testify",
"com_github_testcontainers_testcontainers_go",
"com_github_testcontainers_testcontainers_go_modules_mysql",
"com_github_uber_go_tally_v4",
"in_gopkg_yaml_v3",
"org_golang_google_grpc",
"org_golang_google_protobuf",
"org_uber_go_fx",
Expand Down
8 changes: 8 additions & 0 deletions config/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# Ignore actual config files, only keep examples
*.yaml
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do not think we want this for all of the repo

*.json
!*.example.yaml
!*.example.json

# Ignore private keys
*.pem
10 changes: 10 additions & 0 deletions config/github-app.example.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
app_id: 123456
installation_id: 789012
owner: your-org
repo: your-repo
private_key: |
-----BEGIN RSA PRIVATE KEY-----
MIIEpAIBAAKCAQEA...
YOUR_PRIVATE_KEY_CONTENTS_HERE
...
-----END RSA PRIVATE KEY-----
34 changes: 34 additions & 0 deletions example/server/gateway/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import (

_ "github.com/go-sql-driver/mysql"
"github.com/uber-go/tally/v4"
changeprovider "github.com/uber/submitqueue/extension/change_provider"
"github.com/uber/submitqueue/extension/change_provider/github"
mysqlcounter "github.com/uber/submitqueue/extension/counter/mysql"
queueSQL "github.com/uber/submitqueue/extension/queue/sql"
"github.com/uber/submitqueue/extension/storage/mysql"
Expand Down Expand Up @@ -140,6 +142,38 @@ func run() error {

logger.Info("queue initialized", zap.String("dsn", queueDSN))

// Initialize GitHub change provider (optional)
var changeProvider changeprovider.ChangeProvider
githubConfigPath := os.Getenv("GITHUB_APP_CONFIG_PATH")
if githubConfigPath == "" {
githubConfigPath = "config/github-app.yaml"
}

githubConfig, err := github.LoadConfigFromFile(githubConfigPath)
if err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's not suppress errors
If the logic is to only load the provider is it is configured, a special error, or better a boolean flag, should be returned (and processed) indicating if the provider should be configured or not

logger.Warn("GitHub change provider disabled", zap.String("config_path", githubConfigPath), zap.Error(err))
} else {
httpClient, err := github.NewHTTPClientFromConfig(githubConfig)
if err != nil {
return fmt.Errorf("failed to create GitHub HTTP client: %w", err)
}

changeProvider = github.NewChangeProvider(github.Params{
HTTPClient: httpClient,
Owner: githubConfig.Owner,
Repo: githubConfig.Repo,
})

logger.Info("GitHub change provider initialized",
zap.Int64("app_id", githubConfig.AppID),
zap.String("owner", githubConfig.Owner),
zap.String("repo", githubConfig.Repo),
)
}

// Suppress unused variable warning until changeProvider is used by a controller
_ = changeProvider

// Land controller requires queue publisher
landController := controller.NewLandController(logger.Sugar(), scope, store, cnt, q.Publisher())
gatewayServer := &GatewayServer{
Expand Down
8 changes: 8 additions & 0 deletions extension/change_provider/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
load("@rules_go//go:def.bzl", "go_library")

go_library(
name = "change_provider",
srcs = ["change_provider.go"],
importpath = "github.com/uber/submitqueue/extension/change_provider",
visibility = ["//visibility:public"],
)
14 changes: 14 additions & 0 deletions extension/change_provider/change_provider.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package changeprovider

import "context"

// ChangeProvider integrates with external code review and version control systems
// to check for merge conflicts and perform merges.
type ChangeProvider interface {
// HasMergeConflicts checks whether the head SHA has merge conflicts with the base SHA.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably need a bit more doc'ing on what "merge conflict" is, i.e. if it is a file only conflict, or if automerge won't work, or build target conflict etc.

// Returns true if conflicts exist.
HasMergeConflicts(ctx context.Context, baseSHA string, headSHA string, PR string) (bool, error)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this check have to be in ChangeProvider? How will PhabChangeProvider understand if there is a merge conflict?
I thought the change provider is only to get back a patch to apply.


// Merge merges the head SHA into the base SHA.
Merge(ctx context.Context, baseSHA string, headSHA string) error
}
17 changes: 17 additions & 0 deletions extension/change_provider/github/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
load("@rules_go//go:def.bzl", "go_library")

go_library(
name = "github",
srcs = [
"change_provider.go",
"client.go",
"transport.go",
],
importpath = "github.com/uber/submitqueue/extension/change_provider/github",
visibility = ["//visibility:public"],
deps = [
"//extension/change_provider",
"@com_github_golang_jwt_jwt_v5//:jwt",
"@in_gopkg_yaml_v3//:yaml_v3",
],
)
57 changes: 57 additions & 0 deletions extension/change_provider/github/change_provider.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package github

import (
"context"
"fmt"
"net/http"

changeprovider "github.com/uber/submitqueue/extension/change_provider"
)

// Params holds dependencies for the GitHub change provider.
type Params struct {
// HTTPClient is a pre-configured HTTP client with authentication.
// The caller is responsible for setting up GitHub App or token auth.
HTTPClient *http.Client

// Owner is the repository owner (user or organization).
Owner string

// Repo is the repository name.
Repo string

// BaseURL is the GitHub API base URL. Defaults to https://api.github.com.
BaseURL string
}

type githubChangeProvider struct {
client *githubClient
}

// NewChangeProvider creates a new GitHub-backed ChangeProvider.
func NewChangeProvider(params Params) changeprovider.ChangeProvider {
return &githubChangeProvider{
client: newClient(params),
}
}

// HasMergeConflicts checks whether the head SHA has merge conflicts with the base SHA.
// Returns true if there are conflicts, false otherwise.
// The MergeableState from GitHub (e.g., "dirty", "blocked", "behind") is available
// in the error message when relevant.
func (p *githubChangeProvider) HasMergeConflicts(ctx context.Context, baseSHA string, headSHA string, PR string) (bool, error) {
hasConflicts, mergeableState, err := p.client.hasMergeConflicts(ctx, baseSHA, headSHA, PR)
if err != nil {
// Include mergeableState in error context when available
if mergeableState != "" {
return false, fmt.Errorf("%w (state: %s)", err, mergeableState)
}
return false, err
}
return hasConflicts, nil
}

// Merge merges the head SHA into the base SHA.
func (p *githubChangeProvider) Merge(ctx context.Context, baseSHA string, headSHA string) error {
return p.client.merge(ctx, baseSHA, headSHA)
}
178 changes: 178 additions & 0 deletions extension/change_provider/github/client.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
package github

import (
"bytes"
"context"
"encoding/json"
"errors"
"fmt"
"io"
"net/http"
)

const defaultBaseURL = "https://api.github.com"

// Sentinel errors for GitHub API operations.
var (
ErrMergeConflict = errors.New("merge conflict")
ErrMergeStatusPending = errors.New("merge status pending")
ErrNotFound = errors.New("resource not found")
)

type githubClient struct {
httpClient *http.Client
baseURL string
owner string
repo string
}

func newClient(params Params) *githubClient {
baseURL := params.BaseURL
if baseURL == "" {
baseURL = defaultBaseURL
}

httpClient := params.HTTPClient
if httpClient == nil {
httpClient = http.DefaultClient
}

return &githubClient{
httpClient: httpClient,
baseURL: baseURL,
owner: params.Owner,
repo: params.Repo,
}
}

// doRequest executes an HTTP request and decodes the JSON response.
// If result is nil, the response body is not decoded.
func (c *githubClient) doRequest(ctx context.Context, method, url string, body interface{}, result interface{}) error {
var bodyReader io.Reader
if body != nil {
bodyBytes, err := json.Marshal(body)
if err != nil {
return fmt.Errorf("failed to marshal request body: %w", err)
}
bodyReader = bytes.NewReader(bodyBytes)
}

req, err := http.NewRequestWithContext(ctx, method, url, bodyReader)
if err != nil {
return fmt.Errorf("failed to create HTTP request: %w", err)
}
c.setHeaders(req)
if body != nil {
req.Header.Set("Content-Type", "application/json")
}

resp, err := c.httpClient.Do(req)
if err != nil {
return fmt.Errorf("failed to execute HTTP request: %w", err)
}
defer resp.Body.Close()

if resp.StatusCode == http.StatusNotFound {
return ErrNotFound
}

if resp.StatusCode < 200 || resp.StatusCode >= 300 {
respBody, _ := io.ReadAll(resp.Body)
return fmt.Errorf("request failed with status %d: %s", resp.StatusCode, respBody)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hopefully it won't include a huge body with such a response... because in theory it can.

}

if result != nil {
if err := json.NewDecoder(resp.Body).Decode(result); err != nil {
return fmt.Errorf("failed to decode response: %w", err)
}
}

return nil
}

// PullRequestStatus contains the mergeable status of a PR.
type PullRequestStatus struct {
Mergeable *bool
MergeableState string
}

// hasMergeConflicts checks if the PR has merge conflicts using the GitHub PR API.
// GET /repos/{owner}/{repo}/pulls/{pull_number}
//
// Note: baseSHA and headSHA are currently unused. We use the PR number to get
// mergeability status from GitHub. However, if someone changes the head after
// submitting the request, that state is invalid. Logic to validate head/base SHA
// will be added later in the merge service layer.
func (c *githubClient) hasMergeConflicts(ctx context.Context, baseSHA string, headSHA string, pr string) (bool, string, error) {
url := fmt.Sprintf("%s/repos/%s/%s/pulls/%s", c.baseURL, c.owner, c.repo, pr)

var result struct {
Mergeable *bool `json:"mergeable"`
MergeableState string `json:"mergeable_state"`
}

if err := c.doRequest(ctx, http.MethodGet, url, nil, &result); err != nil {
return false, "", err
}

// mergeable is null while GitHub is computing merge status
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is it the only case? what if it is provided with arbitrary baseSHA and headSHA? I do not believe Github will be computing mergeability for a hash it does not expect.

if result.Mergeable == nil {
return false, result.MergeableState, ErrMergeStatusPending
}

// mergeable=false means there are conflicts
return !*result.Mergeable, result.MergeableState, nil
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does Github's MergeableState (which is a string) maps to SQv2 model cleanly?

}

// mergeRequest represents the request body for the GitHub merge API.
type mergeRequest struct {
Base string `json:"base"`
Head string `json:"head"`
CommitMessage string `json:"commit_message,omitempty"`
}

// merge merges the head SHA into the base SHA using the GitHub merges API.
// POST /repos/{owner}/{repo}/merges
func (c *githubClient) merge(ctx context.Context, baseSHA string, headSHA string) error {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do not think that ChangeProvider should be responsible for the merge

url := fmt.Sprintf("%s/repos/%s/%s/merges", c.baseURL, c.owner, c.repo)

var bodyReader io.Reader
bodyBytes, err := json.Marshal(mergeRequest{
Base: baseSHA,
Head: headSHA,
})
if err != nil {
return fmt.Errorf("failed to marshal merge request: %w", err)
}
bodyReader = bytes.NewReader(bodyBytes)

req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bodyReader)
if err != nil {
return fmt.Errorf("failed to create HTTP request: %w", err)
}
c.setHeaders(req)
req.Header.Set("Content-Type", "application/json")

resp, err := c.httpClient.Do(req)
if err != nil {
return fmt.Errorf("failed to execute merge request: %w", err)
}
defer resp.Body.Close()

switch resp.StatusCode {
case http.StatusCreated, http.StatusNoContent:
return nil
case http.StatusConflict:
return fmt.Errorf("%w: cannot merge %s into %s", ErrMergeConflict, headSHA, baseSHA)
case http.StatusNotFound:
return fmt.Errorf("%w: base or head SHA not found", ErrNotFound)
default:
respBody, _ := io.ReadAll(resp.Body)
return fmt.Errorf("merge request failed with status %d: %s", resp.StatusCode, respBody)
}
}

// setHeaders sets common headers for GitHub API requests.
func (c *githubClient) setHeaders(req *http.Request) {
req.Header.Set("Accept", "application/vnd.github+json")
}
Loading
Loading