-
Notifications
You must be signed in to change notification settings - Fork 0
initial setup for adding change provider interface #55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| # Ignore actual config files, only keep examples | ||
| *.yaml | ||
| *.json | ||
| !*.example.yaml | ||
| !*.example.json | ||
|
|
||
| # Ignore private keys | ||
| *.pem | ||
| 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----- |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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" | ||
|
|
@@ -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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's not suppress errors |
||
| 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{ | ||
|
|
||
| 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"], | ||
| ) |
| 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this check have to be in ChangeProvider? How will PhabChangeProvider understand if there is a merge conflict? |
||
|
|
||
| // Merge merges the head SHA into the base SHA. | ||
| Merge(ctx context.Context, baseSHA string, headSHA string) error | ||
| } | ||
| 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", | ||
| ], | ||
| ) |
| 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) | ||
| } |
| 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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") | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think we want this for all of the repo