Skip to content

feat: add GitLab support#479

Open
harshs-dyte wants to merge 19 commits intoejoffe:masterfrom
harshs-dyte:hshandilya.2026-02-17/gitlab-support
Open

feat: add GitLab support#479
harshs-dyte wants to merge 19 commits intoejoffe:masterfrom
harshs-dyte:hshandilya.2026-02-17/gitlab-support

Conversation

@harshs-dyte
Copy link

Fixes #478

The code is functional and feature-complete as far as my manual testing goes.

I haven't added any GitLab specific tests, opting to let the common forge tests cover the API surface over the githubmock approach taken by github to supplement the tests in spr/spr_test.go. If it's desirable to have this also test things via a mock GitLab API, I can make the change.

Also available as a stack in harshs-dyte#1 if you prefer to review it there.

Replace the GitHub-specific genclient.PullRequestMergeMethod with a new
config.MergeMethod string type and constants (MergeMethodMerge,
MergeMethodSquash, MergeMethodRebase). This decouples the config package
from the GitHub GraphQL client, allowing other forge implementations to
use the same merge method configuration.

- Rename Config.MergeMethod() to Config.ParseMergeMethod()
- Add toGitHubMergeMethod() conversion in githubclient
- Update mock client and tests to use config.MergeMethod

commit-id:e1a2b3c4
Rename RepoConfig fields to be forge-agnostic in preparation for
supporting multiple forge backends:

- GitHubRepoOwner → RepoOwner
- GitHubRepoName  → RepoName
- GitHubHost      → ForgeHost
- GitHubRemote    → Remote
- GitHubBranch    → Branch

The yaml tags are temporarily preserved for backwards compatibility with existing
.spr.yml configuration files.

commit-id:f2b3c4d5
Replace all usages of github.GitHubInterface, github.GitHubInfo,
github.PullRequest, github.RepoAssignee, and github.CheckStatus*
with their forge package equivalents (forge.ForgeInterface,
forge.ForgeInfo, forge.PullRequest, forge.RepoAssignee, forge.CheckStatus*).

The github client now implements forge.ForgeInterface directly,
the template package accepts forge types, and the spr command uses
the forge abstraction layer.

Main changes:
- GitHub client: returns forge.PullRequest with URL field populated
- GitHub client: uses shared forge.BuildPullRequestStack()
- Mock client: implements forge.ForgeInterface
- Templates: accept forge.ForgeInfo and forge.PullRequest
- SPR core: depends on forge.ForgeInterface instead of github.GitHubInterface
- PR String() uses pr.URL directly instead of building URL from config

commit-id:a3b4c5d6
Delete github/interface.go, github/pullrequest.go, and
github/pullrequest_test.go which are superseded by the
forge-agnostic types in the forge/ package.

commit-id:5d31c14b
Introduce gitlab/gitlabclient package with a full implementation of
forge.ForgeInterface backed by the GitLab API (gitlab.com/gitlab-org/api/client-go).

The implementation tries to mimic all the existing GitHub features via
their GitLab-native versions:
- Token discovery from GITLAB_TOKEN env var or glab CLI config
- Merge request CRUD: create, update, merge, close, comment
- Reviewer assignment via GitLab user IDs
- Merge request stack matching using commit-id branch naming
- Approval status and CI pipeline status mapping
- Rebase support for the rebase merge method

The minimum go version is bumped to 1.24.0 as required by the
gitlab client-go dependency. This surfaces a pre-existing go vet
diagnostic in git/mockgit (non-constant format string), which is
fixed in this commit.

commit-id:4b74a3c9
Rename YAML serialization keys from GitHub-specific names to
forge-agnostic names:
- githubRepoOwner -> repoOwner
- githubRepoName  -> repoName
- githubHost      -> forgeHost
- githubRemote    -> remote
- githubBranch    -> branch

Added migrateRepoConfigKeys() in config_parser which automatically
rewrites existing .spr.yml files that still use the old key names,
preserving all values and other keys.

Also:
- Remove default:"github.com" from ForgeHost (no assumed forge)
- Rename NewGitHubRemoteSource -> NewRemoteSource
- Rename regex capture group githubHost -> forgeHost
- Add forge.PullRequestURL() helper for forge-aware PR URL generation
- Update .spr.yml to use new key names

commit-id:a343e3cb
…late/

Move the entire template subsystem (helpers, interface, config_fetcher,
and all four template implementations) out of the GitHub-specific package
into forge/template/ so it is properly shared between forge backends.

All import paths updated; no logic changes.

commit-id:53be3c18
Select GitHub or GitLab client based on cfg.Repo.ForgeHost:
- Host containing "github" -> githubclient
- Host containing "gitlab" -> gitlabclient
- Otherwise -> interactive prompt asking user to choose

The GitHub-specific MaybeStar call is now guarded behind a type
assertion so it only runs when the client implements it.

commit-id:390651d2
Without this the reword gets skipped for commits that mention 'commit-id' in the body without having an actual commit-id

commit-id:636df899
The remote URL regex only matched single-level owner/repo paths, causing
auto-configuration to fail for GitLab repos with nested subgroups like
gitlab.cfdata.org/cloudflare/rt/mobile/core.git.

commit-id:8f7c881f
Replace "github checks pass" with "ci checks pass" since the status
display is shared across GitHub and GitLab forges.

commit-id:b6b4c001
…description

When commit.Body is empty the stack template started its output with
"\n\n---\n...\n---" which GitLab's Markdown parser interprets as YAML
frontmatter delimiters, hiding the stack section entirely. Only emit
the horizontal rule separator when there is actual body content.
GitLab recalculates MR mergeability asynchronously after target branch
changes and rebases. The immediate AcceptMergeRequest call would fail
with 405 Method Not Allowed while this recalculation was still in
progress. Retry with a 2-second backoff for up to 30 seconds.
approvals.Approved is true whenever all configured approval rules are
satisfied, which is vacuously true when no rules apply (e.g. MR does
not target a protected branch). Use len(approvals.ApprovedBy) > 0 to
check whether a human has actually approved the MR.
When spr retargets an MR during the merge flow, GitLab triggers a new
CI pipeline which blocks the merge with 405. Instead of retrying for
30 seconds (far too short for real CI), try a direct merge a few times
for the async mergeability recalculation, then fall back to auto-merge
which queues the MR to merge when the pipeline passes.
@harshs-dyte
Copy link
Author

I've tested this a lot more and discovered that the CI paradigms between GitLab and GitHub might differ entirely too much for spr to cope. When spr tries to merge a stack by changing the target of the top MR/PR, (at least on my company's instance) GitLab will trigger a full CI re-run and invalidate existing approvals which makes the MR unmergeable. I've tried multiple hacky solutions around it that I've also pushed to this branch, but I think this might just not be viable.

@palves
Copy link

palves commented Feb 24, 2026

I've tested this a lot more and discovered that the CI paradigms between GitLab and GitHub might differ entirely too much for spr to cope. When spr tries to merge a stack by changing the target of the top MR/PR, (at least on my company's instance) GitLab will trigger a full CI re-run and invalidate existing approvals which makes the MR unmergeable. I've tried multiple hacky solutions around it that I've also pushed to this branch, but I think this might just not be viable.

Does that actually work on github.com? On my company's repos, github does the same, it drops approvals. I've wondered why the spr documentation doesn't say anything about this, and whether I'm missing some repo or branch setting that makes it work for everyone else. I'd be great if the spr documentation talked about this.

@harshs-dyte
Copy link
Author

I've tested this a lot more and discovered that the CI paradigms between GitLab and GitHub might differ entirely too much for spr to cope. When spr tries to merge a stack by changing the target of the top MR/PR, (at least on my company's instance) GitLab will trigger a full CI re-run and invalidate existing approvals which makes the MR unmergeable. I've tried multiple hacky solutions around it that I've also pushed to this branch, but I think this might just not be viable.

Does that actually work on github.com? On my company's repos, github does the same, it drops approvals. I've wondered why the spr documentation doesn't say anything about this, and whether I'm missing some repo or branch setting that makes it work for everyone else. I'd be great if the spr documentation talked about this.

We had required approvals enabled on GitHub.com and spr served us quite well for the past 6 or so months.

@ejoffe
Copy link
Owner

ejoffe commented Feb 26, 2026

I really admire the effort here! This is a huge change, and it is well designed and thought out.
Having said that I'm not sure we should land this if the merge logic can't work.
Maybe this is better fit for a fork of the repo?
If we do want to move forward with this, here are some notes:

Issues Found

Bugs / Correctness

  1. File handle leak in readGlabCLIConfig (gitlab/gitlabclient/client.go):
    os.Open() is called but the file is never closed. Add defer f.Close().

  2. Fragile commit-ID parsing in GitLab client:
    Uses strings.Split(line, ":")[1] without checking the slice length. A line that is
    exactly "commit-id:" with no value will panic with index out of range. Should use the
    same commitIDRegex pattern used elsewhere in this PR, or at minimum check len(parts) > 1.

  3. Brittle 401 detection in check() helper:
    Uses strings.Contains(msg, "401") to detect auth errors. Any error message containing
    "401" (e.g., in a URL or unrelated number) would trigger the auth error path. Should check
    the actual HTTP status code from the *gitlab.Response object instead.

  4. PullRequest.String() now takes ForgeInterface (forge/pullrequest.go):
    Having a data struct's display method depend on an interface implementation is unusual
    coupling. Consider passing the URL string directly or pre-computing it on the PullRequest
    struct.

Missing Tests

  1. No GitLab-specific unit tests:
    The new 529-line gitlabclient package has zero tests. At minimum,
    matchMergeRequestStack, commit-ID parsing from MR commits, and the merge retry/auto-merge
    logic should be tested. The PR description acknowledges this and offers to add them — I'd
    recommend requiring at least the stack-matching and merge logic be tested.

Performance

  1. N+1 API calls in GitLab GetInfo:
    After fetching MRs, separate API calls are made per-MR for approvals and per-MR for commits
    (two separate loops). For large stacks this adds significant latency. Consider parallelizing
    with goroutines.

  2. No pagination in GitLab MR/member listing:
    ListProjectMergeRequests and ListAllProjectMembers are called without pagination options.
    For projects with many open MRs or members, results may be incomplete.

Naming Inconsistencies

  1. LogGitHubCalls: The GitLab client checks c.config.User.LogGitHubCalls for logging —
    should be renamed to a forge-agnostic name like LogAPICalls.

  2. fetchAndGetGitHubInfo in spr/spr.go: Method name still references GitHub.

  3. githubmock variable in spr/spr_test.go: Should be forgeMock or clientMock.

  4. Mock client lives in github/mockclient/: Since it now implements forge.ForgeInterface,
    it would be less confusing in a shared location.

Other Concerns

  1. Go version bump to 1.24.0: This is a significant jump from 1.21. Verify all CI
    environments support this.

  2. No compile-time interface checks: Neither client has
    var _ forge.ForgeInterface = (*client)(nil) to catch interface drift at compile time.

  3. Draft MR uses title prefix: "Draft: " prefix rather than GitLab's native Draft
    field in CreateMergeRequestOptions. The native API field is more reliable.

  4. os.Exit(-1) for auth errors: Non-standard exit code. Convention is positive integers.

  5. Config migration rewrites .spr.yml silently: migrateRepoConfigKeys reformats the
    file through yaml.Marshal, which may reorder keys and strip comments. Users may be surprised
    to find their config file reformatted.

  6. BuildPullRequestStack panics on invalid branch names: Inherited from the original code,
    but a panic() in a CLI tool could be replaced with a returned error.

@palves
Copy link

palves commented Feb 26, 2026

Does that actually work on github.com? On my company's repos, github does the same, it drops approvals. I've wondered why the spr documentation doesn't say anything about this, and whether I'm missing some repo or branch setting that makes it work for everyone else. I'd be great if the spr documentation talked about this.

We had required approvals enabled on GitHub.com and spr served us quite well for the past 6 or so months.

Thanks for the feedback. But, I thought that the losing approvals when the base changes was hardcoded on github.com. See https://github.com/orgs/community/discussions/57513 for example, where people complain about exactly this to github, stating that this makes stacked PRs not-so-useful. I would really like to find what knob one needs to tweak to make this work.

Edit: moved this to a new issue to avoid polluting this PR further:
#481

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adding support for GitLab

3 participants