feat(landprovider): basic github implementation#109
Closed
kevinlnew wants to merge 4 commits into
Closed
Conversation
This was referenced Mar 2, 2026
kevinlnew
force-pushed
the
kevin.new/github-landprovider
branch
from
28f56d9 to
d9f72cd
Compare
kevinlnew
force-pushed
the
kevin.new/github-landprovider
branch
from
d511180 to
32190ca
Compare
kevinlnew
requested review from
a team,
behinddwalls and
sbalabanov
as code owners
behinddwalls
requested changes
Mar 3, 2026
Comment on lines
+21
to
+22
| // (e.g., "https://api.github.com" or "https://ghe.example.com/api/v3"). | ||
| APIURL string |
Collaborator
There was a problem hiding this comment.
do we need it in the input? can this be encapsulated in httpClient?
| // For single-PR landing, idempotency is ensured by checking if the PR is already | ||
| // merged before attempting the merge. Returns ErrAlreadyLanded if so. | ||
| func (l *landProvider) Land(ctx context.Context, queue string, entries []entity.LandEntry) error { | ||
| l.metricsScope.Counter("land_started").Inc(1) |
Collaborator
There was a problem hiding this comment.
can you update to use the core/metrics pkg?
| return fmt.Errorf("unexpected status %d merging PR #%d: %s", resp.StatusCode, cid.PRNumber, string(body)) | ||
| } | ||
|
|
||
| l.logger.Infow("PR merged successfully", |
Comment on lines
+125
to
+126
| default: | ||
| return fmt.Errorf("unexpected status %d merging PR #%d: %s", resp.StatusCode, cid.PRNumber, string(body)) |
Collaborator
There was a problem hiding this comment.
should this be retruned as infra retryable error?
| // isPRMerged checks whether a pull request has already been merged. | ||
| // Uses the dedicated GitHub "check if merged" endpoint which returns | ||
| // 204 if merged, 404 if not merged (empty response body). | ||
| func (l *landProvider) isPRMerged(ctx context.Context, cid entitygithub.ChangeID) (bool, error) { |
Collaborator
There was a problem hiding this comment.
do we have to check for this? can we not just call merge and let it fail to know that PR is already merged?
| // validateSinglePR ensures entries contain exactly one PR. | ||
| // This implementation does not support batch landing because the GitHub merge API | ||
| // operates on individual PRs, making multi-PR landing non-idempotent on retry. | ||
| func validateSinglePR(entries []entity.LandEntry) error { |
Collaborator
There was a problem hiding this comment.
i guess we can let it panic if anyof this is not true? i think we are mostly checking there are some entries in there?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
feat(landprovider): basic github implementation
Why?
We need a default github implementation of the LandProvider extension for "dumb queue".
What?
Adding github implementation of the
LandProvider. It uses the github merge API and wraps errors into theLandRejectedsentinel error based on the HTTP code returned.Test Plan
Unit tests, CI
Issues
Stack