initial setup for adding change provider interface#55
Conversation
| @@ -0,0 +1,8 @@ | |||
| # Ignore actual config files, only keep examples | |||
| *.yaml | |||
There was a problem hiding this comment.
I do not think we want this for all of the repo
| } | ||
|
|
||
| githubConfig, err := github.LoadConfigFromFile(githubConfigPath) | ||
| if err != nil { |
There was a problem hiding this comment.
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
| // 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. |
There was a problem hiding this comment.
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.
| type ChangeProvider interface { | ||
| // HasMergeConflicts checks whether the head SHA has merge conflicts with the base SHA. | ||
| // Returns true if conflicts exist. | ||
| HasMergeConflicts(ctx context.Context, baseSHA string, headSHA string, PR string) (bool, error) |
There was a problem hiding this comment.
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.
| func parsePrivateKey(pemData []byte) (*rsa.PrivateKey, error) { | ||
| block, _ := pem.Decode(pemData) | ||
| if block == nil { | ||
| return nil, fmt.Errorf("failed to decode PEM block") |
There was a problem hiding this comment.
it would be hard to find where this error exactly is just by looking at such an error in a log. Go does not have a stack trace.
|
|
||
| key, err := x509.ParsePKCS8PrivateKey(block.Bytes) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse private key: %w", err) |
There was a problem hiding this comment.
you may want to accumulate the previous error as well, so far it is shadowed by the last one
|
|
||
| if resp.StatusCode < 200 || resp.StatusCode >= 300 { | ||
| respBody, _ := io.ReadAll(resp.Body) | ||
| return fmt.Errorf("request failed with status %d: %s", resp.StatusCode, respBody) |
There was a problem hiding this comment.
hopefully it won't include a huge body with such a response... because in theory it can.
| return false, "", err | ||
| } | ||
|
|
||
| // mergeable is null while GitHub is computing merge status |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // mergeable=false means there are conflicts | ||
| return !*result.Mergeable, result.MergeableState, nil |
There was a problem hiding this comment.
Does Github's MergeableState (which is a string) maps to SQv2 model cleanly?
|
|
||
| // 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 { |
There was a problem hiding this comment.
I do not think that ChangeProvider should be responsible for the merge
|
Still need this PR or should we close? |
|
I am closing it to get out of sight out of mind |
2 participants
Summary
initial setup for adding change provider interface
Test Plan
Issues