Skip to content
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ export function PRStatusGroup({
<DropdownMenuItem asChild className={menuItemClass}>
<a href={pr.url} target="_blank" rel="noopener noreferrer">
<LuArrowUpRight className={menuIconClass} />
<Trans>View on GitHub</Trans>
<Trans>Open in browser</Trans>
</a>
</DropdownMenuItem>
</DropdownMenuContent>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ export function PRDetailCard({ pr, checks, linkState }: PRDetailCardProps) {
className="group flex items-center justify-between border-t border-border/60 px-3 py-2 text-[11px] font-medium text-muted-foreground transition-colors hover:bg-accent hover:text-foreground"
>
<span>
<Trans>View on GitHub</Trans>
<Trans>Open in browser</Trans>

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This makes it work for both GitHub/GitLab and is alredy part of the translated sentences (used in other places of the UI for the same action)

</span>
<LuArrowUpRight
aria-hidden="true"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ function createManager(
db: HostDb,
overrides: {
execGh?: (args: string[]) => Promise<unknown>;
execGlab?: (args: string[]) => Promise<unknown>;
github?: () => Promise<never>;
git?: unknown;
readWorkspaceRefs?: (
Expand All @@ -174,6 +175,7 @@ function createManager(
((async () => {
throw new Error("octokit should not be used");
}) as never),
execGlab: overrides.execGlab as never,
gitWatcher: { onChanged: () => () => {} } as never,
readWorkspaceRefs: overrides.readWorkspaceRefs,
// Seeded worktree paths are fabricated; default the disk gate open so
Expand Down Expand Up @@ -2490,3 +2492,89 @@ describe("PullRequestRuntimeManager GitHub traffic", () => {
expect(getWorkspace(db, "ws")?.pullRequestId).toBeNull();
});
});

describe("GitLab through glab", () => {
function gitlabProject(owner: string, answer: (args: string[]) => unknown) {
const db = createRealDb();
db.insert(schema.projects)
.values({ id: PROJECT_ID, repoPath: "/repo", createdAt: Date.now() })
.run();
seedWorkspace(db, {
id: "ws",
branch: "feat",
upstreamOwner: owner,
upstreamRepo: "app",
upstreamBranch: "feat",
});
const calls: string[][] = [];
const manager = createManager(db, {
git: async () => ({
remote: async () => `git@${owner.replace("/", ":")}/app.git`,
raw: async () => "origin/main\n",
}),
execGlab: async (args) => {
calls.push(args);
return answer(args);
},
});
return { db, calls, refresh: () => projectRefresher(manager)(PROJECT_ID) };
}

test("links a workspace to its MR with the head pipeline as its check", async () => {
const webUrl =
"https://gitlab.example.com/group/sub/app/-/merge_requests/7";
const glab: Record<string, unknown> = {
"config get": "gitlab.example.com",
"mr list": [
{
iid: 7,
web_url: webUrl,
state: "opened",
source_branch: "feat",
sha: "sha-1",
source_project_id: 1,
target_project_id: 1,
},
],
"mr view": {
web_url: webUrl,
target_project_id: 1,
detailed_merge_status: "not_approved",
head_pipeline: { id: 9 },
},
"ci get": { jobs: [{ name: "test", status: "failed" }] },
};
const { db, calls, refresh } = gitlabProject(
"gitlab.example.com/group/sub",
(args) => glab[`${args[0]} ${args[1]}`] ?? {},
);

await refresh();

const pr = getPrById(db, getWorkspace(db, "ws")?.pullRequestId ?? "");
expect(pr).toMatchObject({
repoProvider: "gitlab",
repoOwner: "gitlab.example.com/group/sub",
url: webUrl,
state: "open",
reviewDecision: "pending",
checksStatus: "failure",
});
expect(calls[1]).toContain("https://gitlab.example.com/group/sub/app");
expect(db.select().from(schema.projects).get()?.repoProvider).toBeNull();
});

test("asks glab again about an unknown host and looks up no MR", async () => {
const { db, calls, refresh } = gitlabProject(
"git.example.com/team",
() => ({}),
);

await refresh();
await refresh();

const ask = ["config", "get", "api_host", "--host", "git.example.com"];
expect(calls).toEqual([ask, ask]);
expect(getWorkspace(db, "ws")?.pullRequestId).toBeNull();
});
});
89 changes: 71 additions & 18 deletions packages/host-service/src/runtime/pull-requests/pull-requests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { randomUUID } from "node:crypto";
import { existsSync } from "node:fs";
import type { Octokit } from "@octokit/rest";
import { parseGitHubRemote } from "@superset/shared/github-remote";
import { parseGitLabRemote } from "@superset/shared/gitlab-remote";
import { and, eq, inArray, isNull } from "drizzle-orm";
import type { HostDb } from "../../db";
import {
Expand All @@ -13,6 +14,10 @@ import {
import type { EventBus } from "../../events/event-bus";
import type { GitWatcher } from "../../events/git-watcher";
import type { ExecGh } from "../../trpc/router/workspace-creation/utils/exec-gh";
import {
type ExecGlab,
execGlab,
} from "../../trpc/router/workspace-creation/utils/exec-glab";
import { type GitFactory, resolveDefaultBranchName } from "../git";
import { ConditionalGh } from "./utils/conditional-gh";
import {
Expand All @@ -26,8 +31,10 @@ import {
fetchOpenPullRequestsFromGh,
fetchPullRequestByHead,
fetchPullRequestByHeadFromGh,
fetchPullRequestByHeadFromGlab,
fetchPullRequestChecks,
fetchPullRequestChecksFromGh,
fetchPullRequestDetailsFromGlab,
fetchPullRequestMergeQueueState,
fetchPullRequestMergeQueueStateFromGh,
fetchPullRequestReviewDecision,
Expand Down Expand Up @@ -92,7 +99,7 @@ function upstreamKey(
return `${owner.toLowerCase()}/${repo.toLowerCase()}#${branch}`;
}

type RepoProvider = "github";
type RepoProvider = "github" | "gitlab";

export interface PullRequestStateSnapshot {
url: string;
Expand Down Expand Up @@ -144,6 +151,7 @@ export interface PullRequestRuntimeManagerOptions {
execGh: ExecGh;
git: GitFactory;
github: () => Promise<Octokit>;
execGlab?: ExecGlab;
gitWatcher: GitWatcher;
/** Override to run the per-workspace branch/HEAD/upstream read off the
* event loop (app wiring passes a worker-pool-backed reader). Defaults
Expand Down Expand Up @@ -223,6 +231,8 @@ export class PullRequestRuntimeManager {
private readonly conditionalGh: ConditionalGh;
private readonly git: GitFactory;
private readonly github: () => Promise<Octokit>;
private readonly execGlab: ExecGlab;
private readonly glabHosts = new Set<string>();
private readonly gitWatcher: GitWatcher;
private safetyNetTimer: ReturnType<typeof setInterval> | null = null;
private projectRefreshTimer: ReturnType<typeof setInterval> | null = null;
Expand Down Expand Up @@ -288,6 +298,7 @@ export class PullRequestRuntimeManager {
this.execGh = this.conditionalGh.exec;
this.git = options.git;
this.github = options.github;
this.execGlab = options.execGlab ?? execGlab;
this.gitWatcher = options.gitWatcher;
this.readWorkspaceRefs =
options.readWorkspaceRefs ??
Expand Down Expand Up @@ -1049,6 +1060,13 @@ export class PullRequestRuntimeManager {
}
}

async getGitLabRepository(
projectId: string,
): Promise<NormalizedRepoIdentity | null> {
const repo = await this.getProjectRepository(projectId);
return repo?.provider === "gitlab" ? repo : null;
}

private async getProjectRepository(
Comment on lines +1063 to 1070

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I decided to keep getProjectRepository private to avoid future devs randomly using this instead of the resolveGithubRepo which avoids misuse (stale copy):
For a GitHub remote it saves owner, name and URL on the project row, and from then on it returns that saved copy without reading the remote again.

It felt worth the ... 7 more lines, super easy to read anyway ^^

projectId: string,
): Promise<NormalizedRepoIdentity | null> {
Expand Down Expand Up @@ -1090,7 +1108,13 @@ export class PullRequestRuntimeManager {
}

const parsedRemote = parseGitHubRemote(remoteUrl);
if (!parsedRemote) return null;
if (!parsedRemote) {
// Not persisted: the rest of the app reads projects.repo* as GitHub.
const gitlab = parseGitLabRemote(remoteUrl);
if (!gitlab || !(await this.isGlabHost(gitlab))) return null;
const defaultBranch = await this.resolveDefaultBranch(project.repoPath);
return { ...gitlab, remoteName, defaultBranch };
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved

this.db
.update(projects)
Expand All @@ -1111,6 +1135,23 @@ export class PullRequestRuntimeManager {
return { ...identity, defaultBranch };
}

// glab knows a host when its config has an API host for it (a local read).
// Only known hosts are kept, so a host added later shows at the next refresh.
private async isGlabHost({ owner }: { owner: string }): Promise<boolean> {
const host = owner.slice(0, owner.indexOf("/"));
if (this.glabHosts.has(host)) return true;
const apiHost = await this.execGlab([
"config",
"get",
"api_host",
"--host",
host,
]).catch(() => "");
if (typeof apiHost !== "string" || apiHost === "") return false;
this.glabHosts.add(host);
return true;
}

// Shared origin/HEAD resolver; a repo-open failure disables the guard
// rather than aborting the whole refresh.
private async resolveDefaultBranch(repoPath: string): Promise<string | null> {
Expand Down Expand Up @@ -1415,23 +1456,25 @@ export class PullRequestRuntimeManager {
cacheKey,
options,
() =>
this.fetchFromGitHub(
"PR head lookup",
{ owner: repo.owner, name: repo.name, head },
() =>
fetchPullRequestByHeadFromGh(
this.execGh,
{ owner: repo.owner, name: repo.name },
head,
),
async () =>
fetchPullRequestByHead(
await this.github(),
{ owner: repo.owner, name: repo.name },
head,
repo.provider === "gitlab"
? fetchPullRequestByHeadFromGlab(this.execGlab, repo, head)
: this.fetchFromGitHub(
"PR head lookup",
{ owner: repo.owner, name: repo.name, head },
() =>
fetchPullRequestByHeadFromGh(
this.execGh,
{ owner: repo.owner, name: repo.name },
head,
),
async () =>
fetchPullRequestByHead(
await this.github(),
{ owner: repo.owner, name: repo.name },
head,
),
{ probe: options.bypassCache === true },
Comment on lines -1418 to +1476

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Note to reviewer: I just added

            repo.provider === "gitlab"
					? fetchPullRequestByHeadFromGlab(this.execGlab, repo, head)
					: this.fetchFromGitHub(

All these lines changed are just whitespace formating...

),
{ probe: options.bypassCache === true },
),
);
}

Expand All @@ -1454,6 +1497,13 @@ export class PullRequestRuntimeManager {
cacheKey,
{ ...options, fingerprint: node.headRefOid },
async () => {
if (repo.provider === "gitlab") {
return fetchPullRequestDetailsFromGlab(
this.execGlab,
repo,
node.number,
);
}
const [reviewDecision, checks] = await this.fetchFromGitHub(
"PR review/check lookup",
context,
Expand Down Expand Up @@ -1524,6 +1574,9 @@ export class PullRequestRuntimeManager {
repo: NormalizedRepoIdentity,
options: { bypassCache?: boolean } = {},
): Promise<GitHubPullRequestNode[]> {
// No GitLab sweep yet. GitLab's branch filter is also case-sensitive, so a
// branch whose case drifted from its MR stays unlinked.
if (repo.provider === "gitlab") return [];
Comment on lines +1577 to +1579

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This case-drift sweep did not feel useful to add...
I should note that when a workspace has no MR, it costs one more glab mr list of up to 100 open MRs per project refresh, and glab has nothing like gh's free 304 revalidation. If you want it, here it is:

Suggested change
// No GitLab sweep yet. GitLab's branch filter is also case-sensitive, so a
// branch whose case drifted from its MR stays unlinked.
if (repo.provider === "gitlab") return [];
if (repo.provider === "gitlab") {
return this.cachedGitHubFetch(
this.openPullRequestsCache,
`${repo.owner.toLowerCase()}/${repo.name.toLowerCase()}`,
options,
() => fetchOpenPullRequestsFromGlab(this.execGlab, repo),
);
}

It needs this in github-gitlab-query.ts, exported from github-query/index.ts and imported here:

export async function fetchOpenPullRequestsFromGlab(
	execGlab: ExecGlab,
	repository: GitLabRepository,
): Promise<GitHubPullRequestNode[]> {
	const raw = await execGlab([
		"mr",
		"list",
		"-R",
		repository.url,
		"-o",
		"updated_at",
		"-P",
		"100",
		"-F",
		"json",
	]);
	return asArray(raw)
		.map((item) => normalizeGitLabMergeRequest(item, repository))
		.filter((node): node is GitHubPullRequestNode => node !== null);
}

const cacheKey = `${repo.owner.toLowerCase()}/${repo.name.toLowerCase()}`;
return this.cachedGitHubFetch(
this.openPullRequestsCache,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
fetchPullRequestMergeQueueStateFromGh,
fetchPullRequestReviewDecisionFromGh,
parseMergedAt,
} from "./github-query";
} from "./github-gitlab-query";

interface GhCall {
args: string[];
Expand Down
Loading