Skip to content
Merged
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
53 changes: 9 additions & 44 deletions apps/desktop/src/lib/forge/forgeFactory.svelte.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { AZURE_DOMAIN, AzureDevOps } from "$lib/forge/azure/azure";
import { BitBucket, BITBUCKET_DOMAIN } from "$lib/forge/bitbucket/bitbucket";
import { AzureDevOps } from "$lib/forge/azure/azure";
import { BitBucket } from "$lib/forge/bitbucket/bitbucket";
import { DefaultForge } from "$lib/forge/default/default";
import { GitHub, GITHUB_DOMAIN } from "$lib/forge/github/github";
import { GitHub } from "$lib/forge/github/github";
import { GitHubClient } from "$lib/forge/github/githubClient";
import { GitLab, GITLAB_DOMAIN, GITLAB_SUB_DOMAIN } from "$lib/forge/gitlab/gitlab";
import { GitLab } from "$lib/forge/gitlab/gitlab";
import { InjectionToken } from "@gitbutler/core/context";
import { deepCompare } from "@gitbutler/shared/compare";
import type { ForgeProvider } from "$lib/baseBranch/baseBranch";
Expand Down Expand Up @@ -113,16 +113,16 @@ export class DefaultForgeFactory implements Reactive<Forge> {
} = config;
this._githubError = githubError;
if (repo && baseBranch) {
this._determinedForgeType = this.determineForgeType(repo, detectedForgeProvider);
const forgeType = forgeOverride ?? detectedForgeProvider ?? "default";
this._determinedForgeType = forgeType;
this._forge = this.build({
repo,
pushRepo,
baseBranch,
forgeType,
Comment thread
mtsgrd marked this conversation as resolved.
githubAuthenticated,
forgeIsLoading,
gitlabAuthenticated,
detectedForgeProvider,
forgeOverride,
});
} else {
this._determinedForgeType = "default";
Expand All @@ -134,25 +134,19 @@ export class DefaultForgeFactory implements Reactive<Forge> {
repo,
pushRepo,
baseBranch,
forgeType,
githubAuthenticated,
forgeIsLoading,
gitlabAuthenticated,
detectedForgeProvider,
forgeOverride,
}: {
repo: RepoInfo;
pushRepo?: RepoInfo;
baseBranch: string;
forgeType: ForgeName;
githubAuthenticated?: boolean;
forgeIsLoading?: boolean;
gitlabAuthenticated?: boolean;
detectedForgeProvider: ForgeProvider | undefined;
forgeOverride: ForgeName | undefined;
}): Forge {
let forgeType = this.determineForgeType(repo, detectedForgeProvider);
if (forgeType === "default" && forgeOverride) {
forgeType = forgeOverride;
}
const forkStr =
pushRepo && pushRepo.hash !== repo.hash ? `${pushRepo.owner}:${pushRepo.name}` : undefined;

Expand Down Expand Up @@ -198,35 +192,6 @@ export class DefaultForgeFactory implements Reactive<Forge> {
return this.default;
}

private determineForgeType(
repo: RepoInfo,
detectedForgeProvider: ForgeProvider | undefined,
): ForgeName {
if (detectedForgeProvider) {
return detectedForgeProvider;
}
const domain = repo.domain;

if (domain.includes(GITHUB_DOMAIN)) {
return "github";
}
if (
domain === GITLAB_DOMAIN ||
domain.startsWith(GITLAB_SUB_DOMAIN + ".") ||
domain.startsWith("xy" + GITLAB_SUB_DOMAIN + ".") // Temporary workaround until we have foerge overrides implemented
) {
return "gitlab";
}
if (domain.includes(BITBUCKET_DOMAIN)) {
return "bitbucket";
}
if (domain.includes(AZURE_DOMAIN)) {
return "azure";
}

return "default";
}

invalidate(tags: TagDescription<ReduxTag>[]) {
const action = this.current.invalidate(tags);
const { dispatch } = this.params;
Expand Down
81 changes: 44 additions & 37 deletions apps/desktop/src/lib/forge/forgeFactory.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,12 @@ describe.concurrent("DefaultforgeFactory", () => {
owner: "test-owner",
},
baseBranch: "some-base",
detectedForgeProvider: undefined,
forgeOverride: undefined,
forgeType: "github",
}),
).instanceOf(GitHub);
});

test("Create self hosted Gitlab service", async () => {
test("Create GitLab service", async () => {
const factory = new DefaultForgeFactory({
gitHubClient,
gitHubApi,
Expand All @@ -71,18 +70,17 @@ describe.concurrent("DefaultforgeFactory", () => {
expect(
factory.build({
repo: {
domain: "gitlab.domain.com",
domain: "gitlab.com",
name: "test-repo",
owner: "test-owner",
},
baseBranch: "some-base",
detectedForgeProvider: undefined,
forgeOverride: undefined,
forgeType: "gitlab",
}),
).instanceOf(GitLab);
});

test("Create Gitlab service", async () => {
test("setConfig uses detectedForgeProvider when present", async () => {
const factory = new DefaultForgeFactory({
gitHubClient,
gitHubApi,
Expand All @@ -92,21 +90,17 @@ describe.concurrent("DefaultforgeFactory", () => {
posthog,
dispatch,
});
expect(
factory.build({
repo: {
domain: "gitlab.com",
name: "test-repo",
owner: "test-owner",
},
baseBranch: "some-base",
detectedForgeProvider: undefined,
forgeOverride: undefined,
}),
).instanceOf(GitLab);
factory.setConfig({
repo: { domain: "github.example.net", name: "test-repo", owner: "test-owner" },
baseBranch: "main",
detectedForgeProvider: "github",
forgeOverride: undefined,
});
expect(factory.current).instanceOf(GitHub);
expect(factory.determinedForgeType).toBe("github");
});

test("Respects detectedForgeProvider: GitHub", async () => {
test("setConfig falls back to detectedForgeProvider when forgeOverride is absent", async () => {
const factory = new DefaultForgeFactory({
gitHubClient,
gitHubApi,
Expand All @@ -116,39 +110,52 @@ describe.concurrent("DefaultforgeFactory", () => {
posthog,
dispatch,
});
const result = factory.build({
repo: {
domain: "gitlab.com",
name: "test-repo",
owner: "test-owner",
},
factory.setConfig({
repo: { domain: "github.example.net", name: "test-repo", owner: "test-owner" },
baseBranch: "main",
detectedForgeProvider: "github",
forgeOverride: undefined,
});
expect(result).instanceOf(GitHub);
expect(factory.current).instanceOf(GitHub);
expect(factory.determinedForgeType).toBe("github");
});
Comment on lines 103 to 121

test("Respects detectedForgeProvider: GitLab", async () => {
test("setConfig resolves to default when both detectedForgeProvider and forgeOverride are absent", async () => {
const factory = new DefaultForgeFactory({
gitHubClient,
gitHubApi,
gitLabClient,
backendApi,
gitLabClient,
gitLabApi,
posthog,
dispatch,
});
const result = factory.build({
repo: {
domain: "github.com",
name: "test-repo",
owner: "test-owner",
},
factory.setConfig({
repo: { domain: "custom.example.com", name: "test-repo", owner: "test-owner" },
baseBranch: "main",
detectedForgeProvider: "gitlab",
detectedForgeProvider: undefined,
forgeOverride: undefined,
});
expect(result).instanceOf(GitLab);
expect(factory.determinedForgeType).toBe("default");
});

test("forgeOverride takes precedence over detectedForgeProvider", async () => {
const factory = new DefaultForgeFactory({
gitHubClient,
gitHubApi,
backendApi,
gitLabClient,
gitLabApi,
posthog,
dispatch,
});
factory.setConfig({
repo: { domain: "github.com", name: "test-repo", owner: "test-owner" },
baseBranch: "main",
detectedForgeProvider: "github",
forgeOverride: "gitlab",
});
expect(factory.current).instanceOf(GitLab);
expect(factory.determinedForgeType).toBe("gitlab");
});
});
22 changes: 21 additions & 1 deletion crates/but-forge/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,8 @@ pub fn get_all_forge_accounts() -> anyhow::Result<Vec<ForgeUser>> {
#[cfg(test)]
mod tests {
use super::{
ForgeName, ForgeUser, match_host_to_accounts_custom_host, normalize_host_for_comparison,
ForgeName, ForgeUser, derive_forge_repo_info, match_host_to_accounts_custom_host,
normalize_host_for_comparison,
};

#[test]
Expand Down Expand Up @@ -280,4 +281,23 @@ mod tests {
"repository.com"
);
}

#[test]
fn derive_forge_detects_github_enterprise_subdomain_ssh() {
let info = derive_forge_repo_info("git@github.ourinternaldomain.net:our-org/my-repo.git")
.expect("should detect GHES from github.* subdomain");
assert_eq!(info.forge, ForgeName::GitHub);
assert_eq!(info.owner, "our-org");
assert_eq!(info.repo, "my-repo");
}

#[test]
fn derive_forge_detects_github_enterprise_subdomain_https() {
let info =
derive_forge_repo_info("https://github.ourinternaldomain.net/our-org/my-repo.git")
.expect("should detect GHES from github.* subdomain");
assert_eq!(info.forge, ForgeName::GitHub);
assert_eq!(info.owner, "our-org");
assert_eq!(info.repo, "my-repo");
}
Comment on lines +284 to +302

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The determination of the forge is now happening mainly in the backend, only to fallback to the frontend system if it can't catch it. So any fixes should mainly go to the rust side, if anything.

I see that we're adding just extra tests (which is nice) but does the functionality need to be fixed in that case as well?

}
Loading