From 4cdeae63538ef6fce6a9bac21a7d9e2de548af71 Mon Sep 17 00:00:00 2001 From: Hamish Fagg Date: Fri, 30 Oct 2026 14:42:07 +1300 Subject: [PATCH 1/8] stale-deploy-label: never strip customer envs Co-Authored-By: Claude Fable 5.1 --- .github/workflows/stale-deploy-label.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/stale-deploy-label.yml b/.github/workflows/stale-deploy-label.yml index 425a711..567200d 100644 --- a/.github/workflows/stale-deploy-label.yml +++ b/.github/workflows/stale-deploy-label.yml @@ -4,6 +4,8 @@ # generator filter — removing the label causes ArgoCD to delete the generated # Applications and prune the namespace. # +# PRs carrying a `customer:` label are never stripped; they live until the PR closes. +# # Called as a reusable workflow by per-repo scheduled wrappers. See e.g. # auth/.github/workflows/stale-deploy.yml # mindshub_frontend/.github/workflows/stale-deploy.yml @@ -42,8 +44,8 @@ jobs: mapfile -t STALE_PRS < <( gh pr list --repo "${TARGET_REPO}" --label deploy --state open \ - --json number,updatedAt,title \ - --jq ".[] | select(.updatedAt < \"${THRESHOLD_ISO}\") | .number" + --json number,updatedAt,title,labels \ + --jq ".[] | select(.updatedAt < \"${THRESHOLD_ISO}\") | select(any(.labels[]?.name; startswith(\"customer:\")) | not) | .number" ) if [[ ${#STALE_PRS[@]} -eq 0 ]]; then From 0d9e833557d1b6588b04f0c4fa97d1743f389a54 Mon Sep 17 00:00:00 2001 From: Hamish Fagg Date: Fri, 30 Oct 2026 14:42:33 +1300 Subject: [PATCH 2/8] prune-pr-environments: remind on closed customer PRs Co-Authored-By: Claude Fable 5.1 --- .github/workflows/prune-pr-environments.yml | 3 +- scripts/prune_pr_environments.py | 47 +++++++++++++ tests/test_prune_pr_environments.py | 75 +++++++++++++++++++-- 3 files changed, 118 insertions(+), 7 deletions(-) diff --git a/.github/workflows/prune-pr-environments.yml b/.github/workflows/prune-pr-environments.yml index e2fa80c..cb7bc5d 100644 --- a/.github/workflows/prune-pr-environments.yml +++ b/.github/workflows/prune-pr-environments.yml @@ -46,7 +46,8 @@ jobs: permission-actions: read permission-administration: write permission-environments: write - permission-pull-requests: read + # The customer close reminder is a PR comment. + permission-pull-requests: write - name: Delete stale PR environments env: diff --git a/scripts/prune_pr_environments.py b/scripts/prune_pr_environments.py index f97e04b..eceae8d 100644 --- a/scripts/prune_pr_environments.py +++ b/scripts/prune_pr_environments.py @@ -8,6 +8,9 @@ Environments are listed before PRs, so an environment created mid-run is left for the next run. A repo whose PR list cannot be read is skipped entirely. + +A closed PR labelled ``customer:`` leaves data behind that nothing +deletes, so each one gets a single comment listing it (see ``REMINDER``). """ from __future__ import annotations @@ -21,6 +24,14 @@ Runner = Callable[[Sequence[str]], "subprocess.CompletedProcess[str]"] DEPLOY_LABEL = "deploy" +CUSTOMER_PREFIX = "customer:" +MARKER = "" +REMINDER = MARKER + """ +This customer env is gone, but its data is not. Delete by hand when the engagement is over: +- Files: `s3://mindshub-files-dev/customer-envs/{host_base}/` +- Cowork workspaces: the `pr-{slug}-{number}` directory on the cowork EFS +- Postgres: the retained PersistentVolume for `data-postgres-postgresql-0` in namespace `pr-{slug}-{number}` (`kubectl get pv | grep pr-{slug}-{number}`) and its AWS Backup recovery points +""" def _run(argv: Sequence[str]) -> "subprocess.CompletedProcess[str]": @@ -47,6 +58,40 @@ def _error(result: "subprocess.CompletedProcess[str]") -> str: return result.stderr.strip() or result.stdout.strip() +def customer_reminders(repo: str, runner: Runner = _run, *, dry_run: bool) -> bool: + """Comment once on each recently closed customer PR in ``repo``; False if a gh call failed.""" + slug = repo.split("/", 1)[1].replace("_", "-") + closed = runner([ + "gh", "api", f"repos/{repo}/pulls?state=closed&sort=updated&direction=desc&per_page=100", + "--jq", f'.[] | select(any(.labels[]; .name | startswith("{CUSTOMER_PREFIX}"))) | ' + f'"\\(.number) \\([.labels[].name | select(startswith("{CUSTOMER_PREFIX}"))][0])"', + ]) + if closed.returncode != 0: + print(f"::error title={repo}::could not list closed PRs: {_error(closed)}", flush=True) + return False + ok = True + for line in _lines(closed): + number, label = line.split(" ", 1) + comments = runner(["gh", "api", f"repos/{repo}/issues/{number}/comments", "--jq", ".[].body"]) + if comments.returncode != 0: + print(f"::error title={repo}::could not read comments on #{number}: {_error(comments)}", flush=True) + ok = False + continue + if MARKER in comments.stdout: + continue + if dry_run: + print(f" would remind on {repo}#{number}", flush=True) + continue + body = REMINDER.format(host_base=label.removeprefix(CUSTOMER_PREFIX), slug=slug, number=number) + res = runner(["gh", "api", "-X", "POST", f"repos/{repo}/issues/{number}/comments", "-f", f"body={body}"]) + if res.returncode != 0: + print(f"::error title={repo}::could not comment on #{number}: {_error(res)}", flush=True) + ok = False + else: + print(f" reminded {repo}#{number}", flush=True) + return ok + + def prune(repos: Sequence[str], *, dry_run: bool, runner: Runner = _run) -> int: failed = False for repo in repos: @@ -75,6 +120,8 @@ def prune(repos: Sequence[str], *, dry_run: bool, runner: Runner = _run) -> int: failed = True else: print(f" deleted {name}", flush=True) + if not customer_reminders(repo, runner, dry_run=dry_run): + failed = True return 1 if failed else 0 diff --git a/tests/test_prune_pr_environments.py b/tests/test_prune_pr_environments.py index 7547108..5047469 100644 --- a/tests/test_prune_pr_environments.py +++ b/tests/test_prune_pr_environments.py @@ -22,30 +22,58 @@ def completed(stdout="", stderr="", returncode=0): class FakeGh: """Answers `gh api` calls from per-repo tables and records every call.""" - def __init__(self, envs, prs, fail_prs=(), fail_delete=()): + def __init__(self, envs, prs, fail_prs=(), fail_delete=(), closed=None, comments=None, fail_closed=()): self.envs, self.prs = envs, prs self.fail_prs, self.fail_delete = set(fail_prs), set(fail_delete) + self.closed, self.comments = closed or {}, comments or {} + self.fail_closed = set(fail_closed) self.calls = [] def __call__(self, argv): argv = list(argv) self.calls.append(argv) + path = next(a for a in argv if a.startswith("repos/")) + repo = "/".join(path.split("/")[1:3]) if "-X" in argv: - path = argv[-1] - repo, name = path.removeprefix("repos/").split("/environments/") + if argv[argv.index("-X") + 1] == "POST": + return completed() + name = path.split("/environments/")[1] if name in self.fail_delete: return completed(stderr="HTTP 403", returncode=1) return completed() - path = next(a for a in argv if a.startswith("repos/")) - repo = "/".join(path.split("/")[1:3]) if "/environments" in path: return completed("\n".join(self.envs.get(repo, [])) + "\n") + if "/pulls?state=closed" in path: + if repo in self.fail_closed: + return completed(stderr="HTTP 502", returncode=1) + lines = [] + for pr in self.closed.get(repo, []): + customer = [n for n in pr["labels"] if n.startswith("customer:")] + if customer: + lines.append(f"{pr['number']} {customer[0]}") + return completed("\n".join(lines) + "\n") + if "/comments" in path: + number = int(path.split("/issues/")[1].split("/")[0]) + return completed("\n".join(self.comments.get((repo, number), [])) + "\n") if repo in self.fail_prs: return completed(stderr="HTTP 502", returncode=1) return completed("\n".join(str(n) for n in self.prs.get(repo, [])) + "\n") def deleted(self): - return [a[-1].removeprefix("repos/") for a in self.calls if "-X" in a] + return [a[-1].removeprefix("repos/") for a in self.calls if "DELETE" in a] + + def _posts(self): + return [a for a in self.calls if "POST" in a] + + def posted(self): + out = [] + for a in self._posts(): + path = next(x for x in a if x.startswith("repos/")) + out.append(("/".join(path.split("/")[1:3]), int(path.split("/issues/")[1].split("/")[0]))) + return out + + def posted_bodies(self): + return [next(x for x in a if x.startswith("body=")).removeprefix("body=") for a in self._posts()] class TestStaleEnvironments: @@ -96,3 +124,38 @@ def test_failed_delete_does_not_stop_the_rest(self): gh = FakeGh(envs={"mindsdb/auth": ["pr-auth-1", "pr-auth-2"]}, prs={"mindsdb/auth": []}, fail_delete={"pr-auth-1"}) assert prune.prune(["mindsdb/auth"], dry_run=False, runner=gh) == 1 assert gh.deleted() == ["mindsdb/auth/environments/pr-auth-1", "mindsdb/auth/environments/pr-auth-2"] + + +class TestCustomerReminders: + def test_posts_once_per_closed_customer_pr(self): + gh = FakeGh(envs={}, prs={}, closed={"mindsdb/auth": [{"number": 5, "labels": ["deploy", "customer:acme"]}, {"number": 6, "labels": ["deploy"]}]}, comments={}) + assert prune.customer_reminders("mindsdb/auth", gh, dry_run=False) is True + assert gh.posted() == [("mindsdb/auth", 5)] + body = gh.posted_bodies()[0] + assert "" in body + assert "s3://mindshub-files-dev/customer-envs/acme/" in body + assert "pr-auth-5" in body + + def test_skips_prs_already_reminded(self): + gh = FakeGh(envs={}, prs={}, closed={"mindsdb/auth": [{"number": 5, "labels": ["customer:acme"]}]}, comments={("mindsdb/auth", 5): ["\nolder"]}) + assert prune.customer_reminders("mindsdb/auth", gh, dry_run=False) is True + assert gh.posted() == [] + + def test_dry_run_posts_nothing(self): + gh = FakeGh(envs={}, prs={}, closed={"mindsdb/auth": [{"number": 5, "labels": ["customer:acme"]}]}, comments={}) + assert prune.customer_reminders("mindsdb/auth", gh, dry_run=True) is True + assert gh.posted() == [] + + def test_failed_list_returns_false_and_posts_nothing(self): + gh = FakeGh(envs={}, prs={}, closed={}, comments={}, fail_closed={"mindsdb/auth"}) + assert prune.customer_reminders("mindsdb/auth", gh, dry_run=False) is False + assert gh.posted() == [] + + def test_prune_runs_reminders_and_folds_failure(self): + gh = FakeGh(envs={"mindsdb/auth": []}, prs={"mindsdb/auth": []}, fail_closed={"mindsdb/auth"}) + assert prune.prune(["mindsdb/auth"], dry_run=False, runner=gh) == 1 + + def test_prune_posts_reminder_for_closed_customer_pr(self): + gh = FakeGh(envs={"mindsdb/auth": []}, prs={"mindsdb/auth": []}, closed={"mindsdb/auth": [{"number": 5, "labels": ["customer:acme"]}]}) + assert prune.prune(["mindsdb/auth"], dry_run=False, runner=gh) == 0 + assert gh.posted() == [("mindsdb/auth", 5)] From 5f5ab7fad6929ab068cf985c68ea69a11463352d Mon Sep 17 00:00:00 2001 From: Hamish Fagg Date: Fri, 30 Oct 2026 14:42:50 +1300 Subject: [PATCH 3/8] pr-env-comment: branded hosts for customer envs Co-Authored-By: Claude Fable 5.1 --- .github/workflows/pr-env-comment.yml | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/.github/workflows/pr-env-comment.yml b/.github/workflows/pr-env-comment.yml index 2e2eaec..577c4e5 100644 --- a/.github/workflows/pr-env-comment.yml +++ b/.github/workflows/pr-env-comment.yml @@ -202,14 +202,18 @@ jobs: # stack. Derived from the env name rather than passed in, because it # is identical for every anchor repo. ALL="${LINKS:-[]}" + # A `customer:` label moves the hostnames to ; + # the namespace stays ${ENV_NAME}. + HOST_BASE="$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR}/labels" --jq '[.[].name | select(startswith("customer:")) | ltrimstr("customer:")] | first // empty')" + HOST="${HOST_BASE:-$ENV_NAME}" if [ "$STANDARD" = "true" ]; then - ALL="$(jq -cn --arg e "$ENV_NAME" --arg d "$HOST_SUFFIX" --argjson extra "${LINKS:-[]}" ' - [ {label: "Console", url: "https://\($e).\($d)"}, - {label: "Auth API", url: "https://auth-\($e).\($d)"}, - {label: "Cowork Web", url: "https://cowork-\($e).\($d)"}, - {label: "Keycloak Admin", url: "https://auth-\($e).\($d)/auth/admin/"}, - {label: "Inference API", url: "https://api-\($e).\($d)/v1"}, - {label: "Cowork Server API", url: "https://cowork-\($e).\($d)/api"} ] + ALL="$(jq -cn --arg e "$ENV_NAME" --arg h "$HOST" --arg d "$HOST_SUFFIX" --argjson extra "${LINKS:-[]}" ' + [ {label: "Console", url: "https://\($h).\($d)"}, + {label: "Auth API", url: "https://auth-\($h).\($d)"}, + {label: "Cowork Web", url: "https://cowork-\($h).\($d)"}, + {label: "Keycloak Admin", url: "https://auth-\($h).\($d)/auth/admin/"}, + {label: "Inference API", url: "https://api-\($h).\($d)/v1"}, + {label: "Cowork Server API", url: "https://cowork-\($h).\($d)/api"} ] + $extra + [ {label: "Namespace", url: "`\($e)`"} ]')" fi From 226566c6ee69577eff4d7e4a8eff2275576f6932 Mon Sep 17 00:00:00 2001 From: Hamish Fagg Date: Fri, 30 Oct 2026 14:44:21 +1300 Subject: [PATCH 4/8] prune-pr-environments: paginate the reminder marker lookup Co-Authored-By: Claude Fable 5.1 --- scripts/prune_pr_environments.py | 2 +- tests/test_prune_pr_environments.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/scripts/prune_pr_environments.py b/scripts/prune_pr_environments.py index eceae8d..6d93527 100644 --- a/scripts/prune_pr_environments.py +++ b/scripts/prune_pr_environments.py @@ -72,7 +72,7 @@ def customer_reminders(repo: str, runner: Runner = _run, *, dry_run: bool) -> bo ok = True for line in _lines(closed): number, label = line.split(" ", 1) - comments = runner(["gh", "api", f"repos/{repo}/issues/{number}/comments", "--jq", ".[].body"]) + comments = runner(["gh", "api", "--paginate", f"repos/{repo}/issues/{number}/comments", "--jq", ".[].body"]) if comments.returncode != 0: print(f"::error title={repo}::could not read comments on #{number}: {_error(comments)}", flush=True) ok = False diff --git a/tests/test_prune_pr_environments.py b/tests/test_prune_pr_environments.py index 5047469..2a49285 100644 --- a/tests/test_prune_pr_environments.py +++ b/tests/test_prune_pr_environments.py @@ -54,7 +54,9 @@ def __call__(self, argv): return completed("\n".join(lines) + "\n") if "/comments" in path: number = int(path.split("/issues/")[1].split("/")[0]) - return completed("\n".join(self.comments.get((repo, number), [])) + "\n") + # Page 1 holds no marker; later pages are only reached with --paginate. + bodies = self.comments.get((repo, number), []) if "--paginate" in argv else [] + return completed("\n".join(bodies) + "\n") if repo in self.fail_prs: return completed(stderr="HTTP 502", returncode=1) return completed("\n".join(str(n) for n in self.prs.get(repo, [])) + "\n") From ee2f64598f4dd7d7cebbc37663eadae8ffd9e6a5 Mon Sep 17 00:00:00 2001 From: Hamish Fagg Date: Fri, 30 Oct 2026 14:46:26 +1300 Subject: [PATCH 5/8] argocd-pr-env-deploy: repo#main links, anton worker image, customer label guard Co-Authored-By: Claude Fable 5.1 --- argocd-pr-env-deploy/action.yml | 95 ++++++++++++++++++++++++++------- 1 file changed, 76 insertions(+), 19 deletions(-) diff --git a/argocd-pr-env-deploy/action.yml b/argocd-pr-env-deploy/action.yml index f978ed5..9eba9b1 100644 --- a/argocd-pr-env-deploy/action.yml +++ b/argocd-pr-env-deploy/action.yml @@ -9,6 +9,8 @@ # - `Deploys: repo#staging` — no API lookup; pins that repo to the # `staging` ECR image tag + `staging` git branch. Useful for testing # the anchor PR against non-PR-branch versions of a sibling service. +# - `Deploys: repo#main` — same, pinned to the `production` ECR image tag +# + `main` git branch. # - `Deploys: mindshub_services#N` sets `revisions.mindshub_services` only. # That repo ships a SAM stack, not an image: its pr-env-stacks workflow # reads the revision off the parent Application and deploys it, so there @@ -194,6 +196,9 @@ runs: ECR_CHECKS=on ecr_tag_exists() { local ecr_repo="mindsdb-${1//_/-}" tag="$2" err + # anton's worker image is published under this name, not + # `mindsdb-anton`. + case "$1" in anton) ecr_repo="minds-anton-scratchpad" ;; esac [ "$ECR_CHECKS" = on ] || return 0 if ! command -v aws >/dev/null 2>&1; then ECR_CHECKS=off @@ -222,17 +227,19 @@ runs: # lint below both reuse it. Strip \r so CRLF bodies don't trip the # line-anchored lint pattern. Fail with a diagnosable message rather # than letting `set -e` kill the step mutely on an API blip. - PR_BODY="$(gh_api "/repos/$OWN_REPO/pulls/$PR_NUMBER" | jq -r '.body // ""' | tr -d '\r')" || { + PR_JSON="$(gh_api "/repos/$OWN_REPO/pulls/$PR_NUMBER")" || { echo "::error::Could not fetch PR #${PR_NUMBER} body from the GitHub API (needed to resolve Deploys: links) — transient API failure or bad GH_TOKEN. Re-run the job." exit 1 } + PR_BODY="$(jq -r '.body // ""' <<<"$PR_JSON" | tr -d '\r')" + LABEL_HOST="$(jq -r '[.labels[].name | select(startswith("customer:")) | ltrimstr("customer:")] | first // ""' <<<"$PR_JSON")" # Lint: `Deploys:`-looking lines that don't match the strict form # would otherwise be silently ignored and the env would quietly run # defaults — the classic offender is a pasted PR URL. Warn per line. while IFS= read -r line; do - if ! printf '%s\n' "$line" | grep -qE 'Deploys:[[:space:]]+[^#[:space:]]+#([0-9]+|staging)'; then - echo "::warning::Ignoring unrecognised Deploys line: '${line}'. Accepted forms: 'Deploys: repo#N', 'Deploys: org/repo#N', 'Deploys: repo#staging' (PR URLs are not parsed)." + if ! printf '%s\n' "$line" | grep -qE 'Deploys:[[:space:]]+[^#[:space:]]+#([0-9]+|staging|main)'; then + echo "::warning::Ignoring unrecognised Deploys line: '${line}'. Accepted forms: 'Deploys: repo#N', 'Deploys: org/repo#N', 'Deploys: repo#staging', 'Deploys: repo#main' (PR URLs are not parsed)." fi done < <(printf '%s\n' "$PR_BODY" | grep -iE '^[[:space:]]*deploys?:' || true) @@ -306,11 +313,26 @@ runs: # after the loop, in the main shell where exit actually works. unresolved_links+=("$link") ;; - staging) - set_args+=( - --helm-set "tags.${repo}=staging" - --helm-set "revisions.${repo}=staging" - ) + line) + # `repo#staging` / `repo#main`: the branch field carries which. + case "$branch" in + staging) + set_args+=( + --helm-set "tags.${repo}=staging" + --helm-set "revisions.${repo}=staging" + ) + ;; + main) + if is_revision_only_repo "$repo"; then + set_args+=(--helm-set "revisions.${repo}=main") + else + set_args+=( + --helm-set "tags.${repo}=production" + --helm-set "revisions.${repo}=main" + ) + fi + ;; + esac ;; revision-only) # No image to pin or check; the revision is the whole deploy. @@ -367,7 +389,7 @@ runs: esac done < <( printf '%s\n' "$PR_BODY" \ - | grep -oE 'Deploys:[[:space:]]+[^#[:space:]]+#([0-9]+|staging)' \ + | grep -oE 'Deploys:[[:space:]]+[^#[:space:]]+#([0-9]+|staging|main)' \ | while read -r _kw ref; do full="${ref%%#*}"; num="${ref##*#}" case "$full" in @@ -398,10 +420,10 @@ runs: fi continue fi - # `#staging` — reuse the linked repo's staging pipeline - # outputs: `staging` ECR image tag + `staging` git branch. - if [ "$num" = "staging" ]; then - echo "staging:${full}#staging:${full##*/}::" + # `#staging` / `#main` — reuse the linked repo's release + # pipeline outputs: moving ECR image tag + git branch. + if [ "$num" = "staging" ] || [ "$num" = "main" ]; then + echo "line:${full}#${num}:${full##*/}:::${num}" continue fi # Repos without an image: head SHA only, no ECR lookup. @@ -430,17 +452,22 @@ runs: done ) - # Fail closed on any unresolvable Deploys link, and comment on the - # anchor PR so the cause is visible without digging through the log. - if [ "${#unresolved_links[@]}" -gt 0 ]; then - links="$(printf '%s, ' "${unresolved_links[@]}")"; links="${links%, }" - comment="$(printf '**PR-env deploy blocked.**\n\nThese `Deploys:` link(s) could not be resolved, so the deploy was refused: %s\n\nA linked PR resolves to no `merge_commit_sha` when it is missing, closed, or unmergeable (a conflicted/`dirty` PR has none). Rather than silently deploy that repo from its default branch, this job fails.\n\nFix: resolve the conflicts (or correct the link), then re-run the deploy by pushing a commit or toggling the `deploy` label.' "$links")" - jq -n --arg b "$comment" '{body: $b}' \ + # Post a comment on the anchor PR explaining a refused deploy. + post_blocking_comment() { + jq -n --arg b "$1" '{body: $b}' \ | curl -sfX POST -H "Authorization: Bearer $GH_TOKEN" \ -H "Accept: application/vnd.github+json" \ "https://api.github.com/repos/$OWN_REPO/issues/$PR_NUMBER/comments" \ --data @- >/dev/null \ || echo "::warning::could not post the blocking comment to ${OWN_REPO}#${PR_NUMBER} (does the gh-token allow writing issue comments?)" >&2 + } + + # Fail closed on any unresolvable Deploys link, and comment on the + # anchor PR so the cause is visible without digging through the log. + if [ "${#unresolved_links[@]}" -gt 0 ]; then + links="$(printf '%s, ' "${unresolved_links[@]}")"; links="${links%, }" + comment="$(printf '**PR-env deploy blocked.**\n\nThese `Deploys:` link(s) could not be resolved, so the deploy was refused: %s\n\nA linked PR resolves to no `merge_commit_sha` when it is missing, closed, or unmergeable (a conflicted/`dirty` PR has none). Rather than silently deploy that repo from its default branch, this job fails.\n\nFix: resolve the conflicts (or correct the link), then re-run the deploy by pushing a commit or toggling the `deploy` label.' "$links")" + post_blocking_comment "$comment" echo "::error::Refusing to deploy: unresolvable Deploys link(s): ${links}" exit 1 fi @@ -505,6 +532,36 @@ runs: # ARGOCD_AUTH_TOKEN env vars set above are picked up by every # subsequent argocd command directly. + # The `customer:` label is read only when the env is created. If it + # now disagrees with the hostBase the parent was created with, refuse. + # A parent that does not exist yet is skipped: the ApplicationSet + # creates it from the labels and the `app set` retry below waits for it. + PARENT_JSON="" + for i in $(seq 1 30); do + if get_err="$("$ARGOCD_BIN" app get "$PARENT_APP" "${ARGOCD_FLAGS[@]}" -o json 2>&1)"; then + PARENT_JSON="$get_err" + break + fi + case "$get_err" in + *PermissionDenied*|*NotFound*|*"not found"*|*"permission denied"*) ;; + *) + echo "argocd app get ${PARENT_APP} failed unexpectedly:" >&2 + echo "$get_err" >&2 + exit 1 + ;; + esac + [ "$i" -eq 30 ] || sleep 10 + done + if [ -n "$PARENT_JSON" ]; then + PARENT_HOST="$(jq -r '[.spec.source.helm.parameters[]? | select(.name=="hostBase") | .value] | first // ""' <<<"$PARENT_JSON")" + if [ "$LABEL_HOST" != "$PARENT_HOST" ]; then + comment="**PR-env deploy blocked.** This PR's \`customer:\` label does not match the environment that already exists (label: \`${LABEL_HOST}\`, env: \`${PARENT_HOST}\`). The label is read only when the env is created. Remove the \`deploy\` label, wait for the env to disappear, then add \`customer:\` first and \`deploy\` second." + post_blocking_comment "$comment" + echo "::error::Refusing to deploy: customer label '${LABEL_HOST}' does not match the existing env's hostBase '${PARENT_HOST}'." + exit 1 + fi + fi + # Retry `app set` directly until the parent App is set-able. Three # transient states are tolerated; anything else is fatal: # 1. App doesn't exist yet — ApplicationSet hasn't run its 60s From d7d40afc455ecc6701d9d173e29e89e7972b09d7 Mon Sep 17 00:00:00 2001 From: Hamish Fagg Date: Fri, 30 Oct 2026 14:47:31 +1300 Subject: [PATCH 6/8] argocd-pr-env-deploy: single app get attempt for the label guard Co-Authored-By: Claude Fable 5.1 --- argocd-pr-env-deploy/action.yml | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/argocd-pr-env-deploy/action.yml b/argocd-pr-env-deploy/action.yml index 9eba9b1..89ea6f5 100644 --- a/argocd-pr-env-deploy/action.yml +++ b/argocd-pr-env-deploy/action.yml @@ -532,26 +532,24 @@ runs: # ARGOCD_AUTH_TOKEN env vars set above are picked up by every # subsequent argocd command directly. - # The `customer:` label is read only when the env is created. If it - # now disagrees with the hostBase the parent was created with, refuse. - # A parent that does not exist yet is skipped: the ApplicationSet - # creates it from the labels and the `app set` retry below waits for it. + # The `customer:` label is read only when the env is created. A parent + # that is absent or being deleted has nothing to compare: skip, and the + # `app set` retry below waits for the ApplicationSet to create it. PARENT_JSON="" - for i in $(seq 1 30); do - if get_err="$("$ARGOCD_BIN" app get "$PARENT_APP" "${ARGOCD_FLAGS[@]}" -o json 2>&1)"; then - PARENT_JSON="$get_err" - break - fi + if get_err="$("$ARGOCD_BIN" app get "$PARENT_APP" "${ARGOCD_FLAGS[@]}" -o json 2>&1)"; then + PARENT_JSON="$get_err" + else case "$get_err" in - *PermissionDenied*|*NotFound*|*"not found"*|*"permission denied"*) ;; + *PermissionDenied*|*NotFound*|*"not found"*|*"permission denied"*|*"being deleted"*) + echo "Application ${PARENT_APP} not found or being deleted; skipping the customer label check." + ;; *) echo "argocd app get ${PARENT_APP} failed unexpectedly:" >&2 echo "$get_err" >&2 exit 1 ;; esac - [ "$i" -eq 30 ] || sleep 10 - done + fi if [ -n "$PARENT_JSON" ]; then PARENT_HOST="$(jq -r '[.spec.source.helm.parameters[]? | select(.name=="hostBase") | .value] | first // ""' <<<"$PARENT_JSON")" if [ "$LABEL_HOST" != "$PARENT_HOST" ]; then From 6573c125d3c9e989b77c5e9faf6b25e1fe0d1a43 Mon Sep 17 00:00:00 2001 From: Hamish Fagg Date: Fri, 30 Oct 2026 15:08:46 +1300 Subject: [PATCH 7/8] argocd-pr-env-deploy: keep stderr out of the guard's JSON Co-Authored-By: Claude Fable 5.1 --- argocd-pr-env-deploy/action.yml | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/argocd-pr-env-deploy/action.yml b/argocd-pr-env-deploy/action.yml index 89ea6f5..9934684 100644 --- a/argocd-pr-env-deploy/action.yml +++ b/argocd-pr-env-deploy/action.yml @@ -194,11 +194,17 @@ runs: # remember ECR is unusable, and report every tag as present from # then on. ECR_CHECKS=on - ecr_tag_exists() { - local ecr_repo="mindsdb-${1//_/-}" tag="$2" err + ecr_repo_name() { # anton's worker image is published under this name, not # `mindsdb-anton`. - case "$1" in anton) ecr_repo="minds-anton-scratchpad" ;; esac + case "$1" in + anton) echo "minds-anton-scratchpad" ;; + *) echo "mindsdb-${1//_/-}" ;; + esac + } + ecr_tag_exists() { + local ecr_repo tag="$2" err + ecr_repo="$(ecr_repo_name "$1")" [ "$ECR_CHECKS" = on ] || return 0 if ! command -v aws >/dev/null 2>&1; then ECR_CHECKS=off @@ -277,7 +283,7 @@ runs: else OWN_TAG="development-${OWN_MERGE_SHA}" if ! ecr_tag_exists "$OWN_SHORT" "$OWN_TAG"; then - echo "::error::ECR image mindsdb-${OWN_SLUG}:${OWN_TAG} not found — did the build job in this run succeed?" + echo "::error::ECR image $(ecr_repo_name "$OWN_SHORT"):${OWN_TAG} not found — did the build job in this run succeed?" exit 1 fi set_args=( @@ -378,7 +384,7 @@ runs: if [ "$ECR_CHECKS" = on ] && ecr_tag_exists "$repo" "$head_tag" && [ "$ECR_CHECKS" = on ]; then tag="$head_tag" elif [ "$ECR_CHECKS" = on ] && ! ecr_tag_exists "$repo" "$merge_tag"; then - echo "::error::No ECR image for linked PR ${link}: neither ${head_tag} nor ${merge_tag} exists in mindsdb-${repo//_/-}. The linked PR's base moved after its last build regenerated the merge SHA; re-run the linked repo's pipeline to build a fresh image, then re-run this job." + echo "::error::No ECR image for linked PR ${link}: neither ${head_tag} nor ${merge_tag} exists in $(ecr_repo_name "$repo"). The linked PR's base moved after its last build regenerated the merge SHA; re-run the linked repo's pipeline to build a fresh image, then re-run this job." exit 1 fi set_args+=( @@ -536,9 +542,10 @@ runs: # that is absent or being deleted has nothing to compare: skip, and the # `app set` retry below waits for the ApplicationSet to create it. PARENT_JSON="" - if get_err="$("$ARGOCD_BIN" app get "$PARENT_APP" "${ARGOCD_FLAGS[@]}" -o json 2>&1)"; then - PARENT_JSON="$get_err" - else + get_errfile="$(mktemp)" + if ! PARENT_JSON="$("$ARGOCD_BIN" app get "$PARENT_APP" "${ARGOCD_FLAGS[@]}" -o json 2>"$get_errfile")"; then + PARENT_JSON="" + get_err="$(cat "$get_errfile")" case "$get_err" in *PermissionDenied*|*NotFound*|*"not found"*|*"permission denied"*|*"being deleted"*) echo "Application ${PARENT_APP} not found or being deleted; skipping the customer label check." @@ -546,10 +553,12 @@ runs: *) echo "argocd app get ${PARENT_APP} failed unexpectedly:" >&2 echo "$get_err" >&2 + rm -f "$get_errfile" exit 1 ;; esac fi + rm -f "$get_errfile" if [ -n "$PARENT_JSON" ]; then PARENT_HOST="$(jq -r '[.spec.source.helm.parameters[]? | select(.name=="hostBase") | .value] | first // ""' <<<"$PARENT_JSON")" if [ "$LABEL_HOST" != "$PARENT_HOST" ]; then From d3306cffe811f424f17257075d46328fe75582a0 Mon Sep 17 00:00:00 2001 From: Hamish Fagg Date: Fri, 30 Oct 2026 16:41:10 +1300 Subject: [PATCH 8/8] Address review: ECR-check branch links, paginate closed PRs, drop labels API call - argocd-pr-env-deploy: `repo#staging`/`repo#main` run the ECR existence check before pinning (cowork publishes no `production` tag); revision-only repos get the chart branch alone for both forms; `argocd-envs#staging|main` moves the parent revision instead of failing as a missing PR; the PR fetch guard also covers a non-JSON body. - pr-env-comment: customer host comes from the event payload's labels, not a second API call inside the redirect group. - prune: scan every closed PR (--paginate) and remind only PRs that carried `deploy` as well as `customer:`; the reminder points at argocd-envs `filesBucket` instead of naming the bucket. - tests: deploy-label filter, pagination flag, comments-read failure path. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/pr-env-comment.yml | 10 +++-- argocd-pr-env-deploy/action.yml | 57 ++++++++++++++++------------ scripts/prune_pr_environments.py | 15 +++++--- tests/test_prune_pr_environments.py | 37 ++++++++++++++---- 4 files changed, 76 insertions(+), 43 deletions(-) diff --git a/.github/workflows/pr-env-comment.yml b/.github/workflows/pr-env-comment.yml index 577c4e5..54a8bdd 100644 --- a/.github/workflows/pr-env-comment.yml +++ b/.github/workflows/pr-env-comment.yml @@ -145,6 +145,7 @@ jobs: env: GH_TOKEN: ${{ github.token }} PR: ${{ github.event.pull_request.number }} + PR_LABELS: ${{ toJSON(github.event.pull_request.labels.*.name) }} ENV_NAME: ${{ inputs.env-name }} LINKS: ${{ inputs.links }} STANDARD: ${{ inputs.standard-links }} @@ -202,11 +203,12 @@ jobs: # stack. Derived from the env name rather than passed in, because it # is identical for every anchor repo. ALL="${LINKS:-[]}" - # A `customer:` label moves the hostnames to ; - # the namespace stays ${ENV_NAME}. - HOST_BASE="$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR}/labels" --jq '[.[].name | select(startswith("customer:")) | ltrimstr("customer:")] | first // empty')" - HOST="${HOST_BASE:-$ENV_NAME}" if [ "$STANDARD" = "true" ]; then + # A `customer:` label moves the hostnames to ; + # the namespace stays ${ENV_NAME}. Labels come from the event + # payload, so no extra API call can fail this redirect group. + HOST_BASE="$(jq -r '[.[] | select(startswith("customer:")) | ltrimstr("customer:")] | first // empty' <<< "${PR_LABELS:-[]}")" + HOST="${HOST_BASE:-$ENV_NAME}" ALL="$(jq -cn --arg e "$ENV_NAME" --arg h "$HOST" --arg d "$HOST_SUFFIX" --argjson extra "${LINKS:-[]}" ' [ {label: "Console", url: "https://\($h).\($d)"}, {label: "Auth API", url: "https://auth-\($h).\($d)"}, diff --git a/argocd-pr-env-deploy/action.yml b/argocd-pr-env-deploy/action.yml index 9934684..d9b8617 100644 --- a/argocd-pr-env-deploy/action.yml +++ b/argocd-pr-env-deploy/action.yml @@ -9,7 +9,7 @@ # - `Deploys: repo#staging` — no API lookup; pins that repo to the # `staging` ECR image tag + `staging` git branch. Useful for testing # the anchor PR against non-PR-branch versions of a sibling service. -# - `Deploys: repo#main` — same, pinned to the `production` ECR image tag +# - `Deploys: repo#main`: same, pinned to the `production` ECR image tag # + `main` git branch. # - `Deploys: mindshub_services#N` sets `revisions.mindshub_services` only. # That repo ships a SAM stack, not an image: its pr-env-stacks workflow @@ -233,7 +233,7 @@ runs: # lint below both reuse it. Strip \r so CRLF bodies don't trip the # line-anchored lint pattern. Fail with a diagnosable message rather # than letting `set -e` kill the step mutely on an API blip. - PR_JSON="$(gh_api "/repos/$OWN_REPO/pulls/$PR_NUMBER")" || { + PR_JSON="$(gh_api "/repos/$OWN_REPO/pulls/$PR_NUMBER")" && jq -e 'type == "object"' <<<"$PR_JSON" >/dev/null || { echo "::error::Could not fetch PR #${PR_NUMBER} body from the GitHub API (needed to resolve Deploys: links) — transient API failure or bad GH_TOKEN. Re-run the job." exit 1 } @@ -283,7 +283,7 @@ runs: else OWN_TAG="development-${OWN_MERGE_SHA}" if ! ecr_tag_exists "$OWN_SHORT" "$OWN_TAG"; then - echo "::error::ECR image $(ecr_repo_name "$OWN_SHORT"):${OWN_TAG} not found — did the build job in this run succeed?" + echo "::error::ECR image $(ecr_repo_name "$OWN_SHORT"):${OWN_TAG} not found. Did the build job in this run succeed?" exit 1 fi set_args=( @@ -321,24 +321,26 @@ runs: ;; line) # `repo#staging` / `repo#main`: the branch field carries which. + # Repos without an image take the chart branch alone. + if is_revision_only_repo "$repo"; then + set_args+=(--helm-set "revisions.${repo}=${branch}") + continue + fi case "$branch" in - staging) - set_args+=( - --helm-set "tags.${repo}=staging" - --helm-set "revisions.${repo}=staging" - ) - ;; - main) - if is_revision_only_repo "$repo"; then - set_args+=(--helm-set "revisions.${repo}=main") - else - set_args+=( - --helm-set "tags.${repo}=production" - --helm-set "revisions.${repo}=main" - ) - fi - ;; + staging) tag=staging ;; + main) tag=production ;; esac + # Not every repo publishes both moving tags (cowork has no + # `production`), so check before pinning: an absent tag leaves + # the child in ImagePullBackOff until wait-timeout expires. + if [ "$ECR_CHECKS" = on ] && ! ecr_tag_exists "$repo" "$tag"; then + echo "::error::No ECR image $(ecr_repo_name "$repo"):${tag} for ${link}. That repo does not publish a '${tag}' tag; link a PR number or the other branch instead." + exit 1 + fi + set_args+=( + --helm-set "tags.${repo}=${tag}" + --helm-set "revisions.${repo}=${branch}" + ) ;; revision-only) # No image to pin or check; the revision is the whole deploy. @@ -403,6 +405,17 @@ runs: *) full="${OWN_REPO%%/*}/$full" ;; esac [ "$full" = "$OWN_REPO" ] && continue + # `#staging` / `#main`: reuse the linked repo's release + # pipeline outputs, moving ECR image tag + git branch. For + # argocd-envs the branch is the parent chart revision itself. + if [ "$num" = "staging" ] || [ "$num" = "main" ]; then + if [ "${full##*/}" = "argocd-envs" ]; then + echo "envs:${full}#${num}:${full##*/}:::${num}" + else + echo "line:${full}#${num}:${full##*/}:::${num}" + fi + continue + fi # argocd-envs is the env's own parent chart, not a workload: # no image to tag and no child Application to revision. Emit # the head BRANCH so phase 2 can move the parent Application's @@ -426,12 +439,6 @@ runs: fi continue fi - # `#staging` / `#main` — reuse the linked repo's release - # pipeline outputs: moving ECR image tag + git branch. - if [ "$num" = "staging" ] || [ "$num" = "main" ]; then - echo "line:${full}#${num}:${full##*/}:::${num}" - continue - fi # Repos without an image: head SHA only, no ECR lookup. if is_revision_only_repo "${full##*/}"; then head_sha="$(gh_api "/repos/$full/pulls/$num" | jq -r '.head.sha // ""')" || true diff --git a/scripts/prune_pr_environments.py b/scripts/prune_pr_environments.py index 6d93527..41f2255 100644 --- a/scripts/prune_pr_environments.py +++ b/scripts/prune_pr_environments.py @@ -9,8 +9,10 @@ Environments are listed before PRs, so an environment created mid-run is left for the next run. A repo whose PR list cannot be read is skipped entirely. -A closed PR labelled ``customer:`` leaves data behind that nothing -deletes, so each one gets a single comment listing it (see ``REMINDER``). +A closed PR labelled ``deploy`` and ``customer:`` had a customer env +whose data nothing deletes, so each one gets a single comment listing it (see +``REMINDER``). Every closed PR is scanned, so a PR missed while the job was +failing is still reminded once it recovers. """ from __future__ import annotations @@ -28,7 +30,7 @@ MARKER = "" REMINDER = MARKER + """ This customer env is gone, but its data is not. Delete by hand when the engagement is over: -- Files: `s3://mindshub-files-dev/customer-envs/{host_base}/` +- Files: the `customer-envs/{host_base}/` prefix in the bucket named by `filesBucket` in argocd-envs `parent/values.yaml` - Cowork workspaces: the `pr-{slug}-{number}` directory on the cowork EFS - Postgres: the retained PersistentVolume for `data-postgres-postgresql-0` in namespace `pr-{slug}-{number}` (`kubectl get pv | grep pr-{slug}-{number}`) and its AWS Backup recovery points """ @@ -59,11 +61,12 @@ def _error(result: "subprocess.CompletedProcess[str]") -> str: def customer_reminders(repo: str, runner: Runner = _run, *, dry_run: bool) -> bool: - """Comment once on each recently closed customer PR in ``repo``; False if a gh call failed.""" + """Comment once on each closed PR in ``repo`` that had a customer env; False if a gh call failed.""" slug = repo.split("/", 1)[1].replace("_", "-") closed = runner([ - "gh", "api", f"repos/{repo}/pulls?state=closed&sort=updated&direction=desc&per_page=100", - "--jq", f'.[] | select(any(.labels[]; .name | startswith("{CUSTOMER_PREFIX}"))) | ' + "gh", "api", "--paginate", f"repos/{repo}/pulls?state=closed&per_page=100", + "--jq", f'.[] | select(any(.labels[]; .name == "{DEPLOY_LABEL}")) | ' + f'select(any(.labels[]; .name | startswith("{CUSTOMER_PREFIX}"))) | ' f'"\\(.number) \\([.labels[].name | select(startswith("{CUSTOMER_PREFIX}"))][0])"', ]) if closed.returncode != 0: diff --git a/tests/test_prune_pr_environments.py b/tests/test_prune_pr_environments.py index 2a49285..6954538 100644 --- a/tests/test_prune_pr_environments.py +++ b/tests/test_prune_pr_environments.py @@ -22,11 +22,11 @@ def completed(stdout="", stderr="", returncode=0): class FakeGh: """Answers `gh api` calls from per-repo tables and records every call.""" - def __init__(self, envs, prs, fail_prs=(), fail_delete=(), closed=None, comments=None, fail_closed=()): + def __init__(self, envs, prs, fail_prs=(), fail_delete=(), closed=None, comments=None, fail_closed=(), fail_comments=()): self.envs, self.prs = envs, prs self.fail_prs, self.fail_delete = set(fail_prs), set(fail_delete) self.closed, self.comments = closed or {}, comments or {} - self.fail_closed = set(fail_closed) + self.fail_closed, self.fail_comments = set(fail_closed), set(fail_comments) self.calls = [] def __call__(self, argv): @@ -46,14 +46,17 @@ def __call__(self, argv): if "/pulls?state=closed" in path: if repo in self.fail_closed: return completed(stderr="HTTP 502", returncode=1) + # Mirrors the script's jq: both labels, every page. lines = [] - for pr in self.closed.get(repo, []): + for pr in self.closed.get(repo, []) if "--paginate" in argv else []: customer = [n for n in pr["labels"] if n.startswith("customer:")] - if customer: + if customer and "deploy" in pr["labels"]: lines.append(f"{pr['number']} {customer[0]}") return completed("\n".join(lines) + "\n") if "/comments" in path: number = int(path.split("/issues/")[1].split("/")[0]) + if (repo, number) in self.fail_comments: + return completed(stderr="HTTP 502", returncode=1) # Page 1 holds no marker; later pages are only reached with --paginate. bodies = self.comments.get((repo, number), []) if "--paginate" in argv else [] return completed("\n".join(bodies) + "\n") @@ -135,16 +138,34 @@ def test_posts_once_per_closed_customer_pr(self): assert gh.posted() == [("mindsdb/auth", 5)] body = gh.posted_bodies()[0] assert "" in body - assert "s3://mindshub-files-dev/customer-envs/acme/" in body + assert "`customer-envs/acme/`" in body assert "pr-auth-5" in body + def test_customer_pr_that_never_deployed_is_not_reminded(self): + gh = FakeGh(envs={}, prs={}, closed={"mindsdb/auth": [{"number": 5, "labels": ["customer:acme"]}]}, comments={}) + assert prune.customer_reminders("mindsdb/auth", gh, dry_run=False) is True + assert gh.posted() == [] + + def test_closed_prs_are_paginated(self): + gh = FakeGh(envs={}, prs={}, closed={"mindsdb/auth": [{"number": 5, "labels": ["deploy", "customer:acme"]}]}, comments={}) + prune.customer_reminders("mindsdb/auth", gh, dry_run=False) + listing = next(c for c in gh.calls if any("/pulls?state=closed" in a for a in c)) + assert "--paginate" in listing + assert gh.posted() == [("mindsdb/auth", 5)] + + def test_unreadable_comments_skip_that_pr_and_fail_the_run(self): + closed = {"mindsdb/auth": [{"number": 5, "labels": ["deploy", "customer:acme"]}, {"number": 6, "labels": ["deploy", "customer:beta"]}]} + gh = FakeGh(envs={}, prs={}, closed=closed, comments={}, fail_comments={("mindsdb/auth", 5)}) + assert prune.customer_reminders("mindsdb/auth", gh, dry_run=False) is False + assert gh.posted() == [("mindsdb/auth", 6)] + def test_skips_prs_already_reminded(self): - gh = FakeGh(envs={}, prs={}, closed={"mindsdb/auth": [{"number": 5, "labels": ["customer:acme"]}]}, comments={("mindsdb/auth", 5): ["\nolder"]}) + gh = FakeGh(envs={}, prs={}, closed={"mindsdb/auth": [{"number": 5, "labels": ["deploy", "customer:acme"]}]}, comments={("mindsdb/auth", 5): ["\nolder"]}) assert prune.customer_reminders("mindsdb/auth", gh, dry_run=False) is True assert gh.posted() == [] def test_dry_run_posts_nothing(self): - gh = FakeGh(envs={}, prs={}, closed={"mindsdb/auth": [{"number": 5, "labels": ["customer:acme"]}]}, comments={}) + gh = FakeGh(envs={}, prs={}, closed={"mindsdb/auth": [{"number": 5, "labels": ["deploy", "customer:acme"]}]}, comments={}) assert prune.customer_reminders("mindsdb/auth", gh, dry_run=True) is True assert gh.posted() == [] @@ -158,6 +179,6 @@ def test_prune_runs_reminders_and_folds_failure(self): assert prune.prune(["mindsdb/auth"], dry_run=False, runner=gh) == 1 def test_prune_posts_reminder_for_closed_customer_pr(self): - gh = FakeGh(envs={"mindsdb/auth": []}, prs={"mindsdb/auth": []}, closed={"mindsdb/auth": [{"number": 5, "labels": ["customer:acme"]}]}) + gh = FakeGh(envs={"mindsdb/auth": []}, prs={"mindsdb/auth": []}, closed={"mindsdb/auth": [{"number": 5, "labels": ["deploy", "customer:acme"]}]}) assert prune.prune(["mindsdb/auth"], dry_run=False, runner=gh) == 0 assert gh.posted() == [("mindsdb/auth", 5)]