diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 2a98be2811..69d7a701ac 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -64567911809a2feb4cb0383153939d25585d1f7b +37221a36157a91b99d86f96b613c8ecdd119e564 diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index d61a49d6b5..609dd420f8 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -337,13 +337,14 @@ pub struct GitRepositorySettings { #[serde(default, skip_serializing_if = "Option::is_none")] pub auto_pull: Option, /// Open a PR when a deploy pushes a `wm_deploy/**` branch of this promotion - /// repo (app-backed only; runs from the deploy callback so it works without + /// repo (needs a credential the server holds — a GitHub App installation or + /// a checked GitLab token; runs from the deploy callback so it works without /// inbound webhooks). Off by default so upgrades don't change behavior. #[serde(default, skip_serializing_if = "is_false")] pub promotion_open_prs: bool, /// Parent-level: open a PR when a fork of this workspace deploys to its - /// `wm-fork/**` branch (app-backed only; the fork's deploy callback reads - /// this from the parent). Off by default. + /// `wm-fork/**` branch (needs a credential the server holds; the fork's + /// deploy callback reads this from the parent). Off by default. #[serde(default, skip_serializing_if = "is_false")] pub fork_open_prs: bool, /// Server-owned: the last failure opening a PR for a deploy branch of this diff --git a/frontend/src/lib/components/GitLabIntegration.svelte b/frontend/src/lib/components/GitLabIntegration.svelte index 25462496eb..7d588067f5 100644 --- a/frontend/src/lib/components/GitLabIntegration.svelte +++ b/frontend/src/lib/components/GitLabIntegration.svelte @@ -89,7 +89,10 @@ // suggested path can collide with another project's, and the field is free // text, so the name alone proves nothing: only the repository the stored // value points at does. - type Occupant = 'free' | 'same-repo' | 'other' + // `checking` exists so a path that has just changed is never treated as the + // previous path's verdict: the answer is asynchronous, and applying against a + // stale one is how an occupied variable gets overwritten anyway. + type Occupant = 'checking' | 'free' | 'same-repo' | 'other' let occupant: Occupant = $state('free') $effect(() => { const path = variablePath @@ -99,6 +102,7 @@ occupant = 'free' return } + occupant = 'checking' let cancelled = false VariableService.existsVariable({ workspace, path }) .then(async (exists) => { @@ -157,17 +161,33 @@ async function apply(close: (_: any) => void) { // The token is cleared once stored, and re-applying without one would - // overwrite the stored credential with an empty password. A path holding - // another repository's remote is never written: that would repoint every - // resource using it, silently, at this project. - if (!ws || !project || !token || occupant === 'other') return + // overwrite the stored credential with an empty password. + const pathIsWritable = occupant === 'free' || occupant === 'same-repo' + if (!ws || !project || !token || !pathIsWritable) return applying = true try { const value = repositoryUrl(project) + const target = repoIdentity(project.http_url_to_repo) const exists = await VariableService.existsVariable({ workspace: ws, path: variablePath }) + // Re-read rather than trust the state the button was enabled from: the + // path can change between the check and the click, and another writer + // can take the path in between. A path holding anything but this same + // repository is never written over — that would repoint every resource + // using it, silently, at this project. + if (exists) { + const current = await VariableService.getVariableValue({ + workspace: ws, + path: variablePath + }).catch(() => undefined) + if (!current || !target || repoIdentity(current) !== target) { + occupant = 'other' + sendUserToast(`${variablePath} holds something else. Choose another path.`, true) + return + } + } if (exists) { await VariableService.updateVariable({ workspace: ws, @@ -277,7 +297,9 @@ size="sm" inputProps={{ oninput: () => (variablePathTouched = true) }} /> - {#if occupant === 'other'} + {#if occupant === 'checking'} +
Checking this path...
+ {:else if occupant === 'other'}
This variable already holds something else. Choose another path, or anything using it would start pointing at this project. @@ -292,7 +314,11 @@