ci: deploy + seed a test stand after the umbrella chart publish on main - #2404
Merged
Conversation
3 tasks
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change adds a GitHub Actions test-stand pipeline. It defines the environment, guarded GitOps deployment, realm-driven seeding, credential redaction, and post-deployment smoke tests. ChangesTest-stand environment contract
Guarded GitOps operations
Publish-triggered deployment and log handling
Realm-driven seeding and manifest transport
Post-deployment smoke validation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant BuildPublish as Build and publish workflow
participant Workflow as deploy-test-stand.yml
participant GitOps as GitOps Make targets
participant TestStand as test-stand
participant Seeder as seed-stand.sh
participant Smoke as stand_smoke tests
BuildPublish->>Workflow: publish chart on main
Workflow->>GitOps: deploy resolved chart version
GitOps->>TestStand: install or upgrade umbrella release
Workflow->>TestStand: validate routes and public edge
Workflow->>Seeder: run selected seed stage
Seeder->>TestStand: create and monitor seed Jobs
Workflow->>Smoke: run API and UI smoke tests
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ktursunov
commented
Aug 10, 2026
ktursunov
left a comment
ktursunov
left a comment
Contributor
Author
There was a problem hiding this comment.
Follow-up review focused on simplifying the disposable test-stand flow and keeping it on the official deployment path.
ktursunov
force-pushed
the
test-stand-ci
branch
from
August 11, 2026 02:17
5edc716 to
e573dbf
Compare
ktursunov
pushed a commit
that referenced
this pull request
Aug 11, 2026
…ary, deploy only what was published Three of the six review comments on #2404, each self-contained. PUBLIC DIAGNOSTICS ARE GONE. `stand-diagnostics.sh` and the failure step that ran it are deleted. It was a curated allowlist rather than a dump, but a curated allowlist is a standing invitation to widen, and what it guarded against — an infrastructure map assembled out of CI logs — is not worth the convenience of not opening a terminal. Helm history, pod tables, warning events and container logs are now read operator-side by somebody holding a credential. In its place, a shallow black-box gate that runs after the deploy and BEFORE the seed, because the seed's ceiling is 65 minutes and every one of them is wasted against a stand nobody can reach — worse, an unreachable stand used to announce itself an hour later as a smoke failure, which reads like a product fault rather than a routing one. It knows only what an anonymous visitor knows: public DNS, a public certificate, four unauthenticated requests. curl's exit code is classified, so "the name does not resolve", "the certificate is not trusted" and "the app returned the wrong thing" are three different messages with three different owners instead of one status code. It checks `/` 200, `/auth/me` 401, discovery 200 with the issuer the values file configures, and that `/auth/login` bounces to that issuer's authorize endpoint carrying PKCE and scope=openid. The Location is matched and never echoed: it carries `state`, `nonce` and the `code_challenge`. `redact-stand-log.py` stays. It still filters the helm output, the seeder (whose manifest names every persona and the tenant, and whose log is the only copy) and pytest, whose assertion messages quote response bodies. THE SUMMARY NO LONGER LIES. Its stage row was built from `steps.stages.outputs.*` — what was REQUESTED — so a run whose deploy failed still printed "seed ✓, smoke ✓" for two stages that never started. That table is what gets screenshotted into a thread; it was the most confident liar in the file. Each stage now derives from its steps' `outcome`, worst-first, so failure beats cancelled beats skipped beats success, and a stage that started and did not finish reads as incomplete rather than as either. `outcome` and not `conclusion` so `continue-on-error` cannot relabel a failure, and `|| 'skipped'` because a step the job never REACHED has no entry in the context at all — which is precisely how an unrun stage rendered as a tick. NO MORE ARBITRARY VERSIONS. The `workflow_dispatch` version input is gone entirely. A call takes publish-chart's output; a dispatch reads the same fact back from `.insight-version` on the default branch through the API — not from the checkout, which is as old as its branch. An earlier draft resolved it from the checkout and, on a fork, selected 0.4.8 against a stand running 0.5.x; the post-deploy check compares against the version REQUESTED, so it would have called that downgrade a success. This stand is disposable and its recovery is to rebuild and install the current chart, so there is nothing for a version box to be legitimately used for. That deletes `latest-chart-version.py`, which was the only urllib caller in the tree and both `dynamic-urllib-use-detected` findings failing the `sast` check. `nosemgrep` was not an option: it lands in the SARIF as accepted, Code Scanning opens the alert regardless, and the added directive shifts the fingerprint so a duplicate is filed. Signed-off-by: Konstantin Tursunov <konstantin.tursunov@constructor.tech>
ktursunov
pushed a commit
that referenced
this pull request
Aug 11, 2026
…are its own shape Review comments 1 and 4 on #2404, which are one change: CI stops spelling out a second helm procedure, and the official target learns to represent this stand. Everything that used to justify the fork is now DECLARED rather than worked around — `deploy.atomic`, `keycloak.realms`, `oidcClientSecret` and `bootstrap.namespaces` in the inventory, `credentials.deploymentMode` in the values. `deploy-insight` reads them and takes the corresponding path, so ENV=local and ENV=functional-ci render byte-identical helm invocations to before (verified: compose-app-secrets runs, sealed manifests required, realms packed, `--atomic`, `--create-namespace`), and ENV=test-stand takes none of those. Four defects in the target itself, all of which had to be fixed before CI could be gated on it: `make deploy` REPORTED SUCCESS ON A FAILED UPGRADE. helm is piped into `tee`, the recipe took tee's status, and the trailing `|| true` on the log push swallowed the rest. Fixed with `set -o pipefail` and an explicit rc — which needs `SHELL := /bin/bash`, because /bin/sh on a runner is dash and has no pipefail. `vpn-up` PROBED WITH `cluster-info`, which lists Services in kube-system. A namespace-scoped credential is refused, so the check reported "is the VPN up?" against a perfectly reachable cluster. Now `auth can-i --list`, which stock RBAC allows every authenticated principal and denies an anonymous one, so it still fails closed on an expired credential and needs no object to exist. `--create-namespace` WAS HARDCODED, and helm POSTs a cluster-scoped Namespace on a FIRST install. That is exactly the path a wiped stand takes, so the documented recovery would have 403'd on the credential it recommends. Gated on `bootstrap.namespaces`. `--wait` CAME ONLY FROM `--atomic`. Turning atomic off would have silently stopped the deploy waiting for rollout and let the post-deploy steps race a half-started stand; on helm 4 an omitted `--wait` defaults to hooks only, which is the same trap in different clothes. Passed explicitly now. Two more, found by adversarially reviewing the design rather than by running it: The `//` idiom the switches were going to use is yq's ALTERNATIVE operator, and it treats `false` exactly like `null` — `.deploy.atomic // true` reads an explicit `atomic: false` back as TRUE. Every switch is now read raw and takes the non-default path only on an explicit value, which also means ENV=local, whose inventory and values are wizard-generated and absent at parse time, falls back to doing the work rather than to skipping it. With `--atomic` off, a FAILED FIRST install leaves a release with no deployed revision, and helm then refuses every subsequent upgrade with "has no deployed releases" — the stand would stay red on every merge until a human intervened, which is a worse wedge than the one atomic-off avoids. `deploy-insight` now detects that state, and `pending-*`, and names the command that clears each. The OIDC client secret is passed with `--set-file`, not `--set-string`: `--set-string` splits on commas and interprets backslashes, so a generated secret containing either was silently truncated into one the realm does not agree with. It is deliberately not passed to `helm template`, so the rendered manifest on disk never carries it. Comment 4: provision-ci-deployer.sh no longer emits anything outside its own namespace. The airbyte Role/RoleBinding it used to create is a precondition owned by whoever owns that namespace; the script now PRINTS the exact rule set and verifies it, and the verification covers `get` and `update` as well as `create`, because the chart's own object there carries version labels that change on every bump — helm patches it on every upgrade, not only the first. Hand-off note: the two `ci-deployer-airbyte-rbac` objects this script previously created are still live, and `--purge` never removed them. They need one manual hand-off to the namespace owner, or removal once the official identity is used. Signed-off-by: Konstantin Tursunov <konstantin.tursunov@constructor.tech>
ktursunov
pushed a commit
that referenced
this pull request
Aug 11, 2026
…ops the databases too Review comment 2 on #2404 asked for "an executable, idempotent infrastructure create/recreate script; INFRA.md alone is not enough to prove we can rebuild on the same or a replacement cluster." The important part is what it wipes, and that is not obvious. A `helm uninstall` is NOT a clean stand: this namespace holds six Deployments and nothing else — no StatefulSets, no PersistentVolumeClaims — so MariaDB, ClickHouse, Redis and Redpanda live in their own namespaces under their own operators and are entirely untouched by anything helm does to the release. That was established the expensive way. `identity.persons` is an append-only observation log, and one stale email observation written by a seed run five hours earlier survived a full uninstall/reinstall. It produced five API failures — profiles-by-email 404, a session resolving to the wrong person, a visibility grant that would not apply — every one of which read convincingly as a product defect, and which were diagnosed as one twice before anybody looked at the row. A stand that cannot be fully recreated is a stand whose red runs cannot be trusted, which is the same argument the review makes, arrived at from the other end. Order, and why it is this order: guards → survivors → datastore preflight → uninstall → wipe → deploy → verify The datastore preflight is not politeness. The two wipes are sequential, so without a read-only reachability-and-authorisation check first, an unreachable ClickHouse or a missing DROP grant is discovered AFTER MariaDB has been dropped — turning a refusal into a half-destroyed stand. The survivor check runs before anything is destroyed because those objects are the only way back: five Secrets and the realm ConfigMap, none of them Helm-managed, without which the rebuilt release cannot reach its datastores, complete a login, or re-import its realm. `insight-oidc` is checked for a non-empty value and not merely for existence — the chart writes whatever it is given into the authenticator's config, so an empty one yields a confidential client with no secret: every pod Ready, the release `deployed`, every login broken. Guardrails, because this drops three MariaDB schemas and every ClickHouse application database: plan-only by default; `--expect-cluster` is REQUIRED with no default and is compared against the cluster the context RESOLVES to rather than the context name; `--apply` additionally demands the literal token `--confirm wipe-<env>`. Details that were wrong in the first draft and are worth not re-introducing: the wipe Job takes its passwords through `secretKeyRef` with explicit names, never `envFrom` — the keys in insight-db-creds are hyphenated, and envFrom would have injected them as variable names no shell can read, so the Job would have run with empty passwords. Its images are the chart's own hook images rather than a third one chosen here. The analytics database is recreated by the script with the operator's charset and collation, because the chart's pre-install hook creates `identity` and `keycloak` but not that one, and the operator's Database CR is cleanupPolicy: Skip. Documented rather than automated: Airbyte's per-connection cursors live in Airbyte's own store and are not reset by dropping the bronze databases it writes into, so an incremental connection would resume from its cursor and never re-emit the history it has already recorded as synced. The README's "Why not `make deploy`" section is replaced by "How `make deploy` represents this stand" — a table of the six declarations that now let one deploy procedure serve both this cluster and a sandbox — and its Rollback section by Recovery, since rolling back reinstalls the previous chart and has wedged this stand once already. Signed-off-by: Konstantin Tursunov <konstantin.tursunov@constructor.tech>
Comment on lines
+20
to
+26
| kind: RoleBinding | ||
| metadata: | ||
| name: ci-deployer-admin | ||
| namespace: insight | ||
| labels: | ||
| app.kubernetes.io/name: ci-deployer | ||
| app.kubernetes.io/component: ci-credential |
Contributor
Author
There was a problem hiding this comment.
By design, with containment asserted rather than assumed: this binding grants the built-in admin ClusterRole via a namespace-scoped RoleBinding only — the credential is the CI deployer for this one namespace (helm release storage, the seed Job, pod logs), and make verify-ci-credential proves the negative space with cluster-scoped kubectl auth can-i checks that must answer no. The token is revocable in one command (make revoke-ci). A narrower hand-rolled Role would need most of the admin verb surface anyway (helm owns arbitrary namespaced kinds) while adding a second RBAC definition to maintain. Suggest dismissing the alert as won't-fix / by-design with this rationale.
Contributor
Author
There was a problem hiding this comment.
Superseded — fixed for real in 04d8bac rather than argued: the admin ClusterRole binding is gone, replaced by an enumerated ci-deployer-core Role listing exactly the kinds the umbrella chart manages in this namespace plus read-only diagnostics. A new chart kind fails the deploy closed with a helm forbidden naming the missing resource. Verified live: full make verify-ci-credential matrix green and a real make deploy through the credential after the admin binding was deleted from the cluster. KSV-0111 no longer matches by construction.
ktursunov
had a problem deploying
to
insight-test-stand
August 11, 2026 12:34 — with 
GitHub Actions
Failure
GitHub Actions
Failure
ktursunov
had a problem deploying
to
insight-test-stand
August 11, 2026 12:36 — with
GitHub Actions
Failure
GitHub Actions
Failure
ktursunov
had a problem deploying
to
insight-test-stand
August 11, 2026 12:38 — with
GitHub Actions
Failure
GitHub Actions
Failure
ktursunov
had a problem deploying
to
insight-test-stand
August 11, 2026 12:49 — with
GitHub Actions
Failure
GitHub Actions
Failure
ktursunov
had a problem deploying
to
insight-test-stand
August 11, 2026 12:56 — with
GitHub Actions
Failure
GitHub Actions
Failure
ktursunov
had a problem deploying
to
insight-test-stand
August 11, 2026 13:05 — with
GitHub Actions
Failure
GitHub Actions
Failure
ktursunov
had a problem deploying
to
insight-test-stand
August 11, 2026 13:13 — with
GitHub Actions
Failure
GitHub Actions
Failure
ktursunov
had a problem deploying
to
insight-test-stand
August 11, 2026 13:21 — with
GitHub Actions
Failure
GitHub Actions
Failure
added 12 commits
August 12, 2026 09:56
- add the sdlc kit tool_risk_fingerprint to core.toml - add the @cf:root-agents block to CLAUDE.md - normalize a dash in the config README Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
… credential The umbrella-only gitops environment the post-merge test-stand deploy installs through, plus the two operator tools that make it usable without CI. `environments/test-stand/` deliberately manages the umbrella and nothing else: `bootstrap.*` and `system.*` are all false because the stand's datastores are operator-managed in their own namespaces and its edge is Envoy Gateway with HTTPRoutes the chart does not render. Its values reconcile the reference deployment repo against what the release actually carries, and keep `credentials.deploymentMode: helm` / `autoGenerate: true` to match: the chart's `lookup` finds the pre-created, label-less `insight-db-creds` and stands down, so an upgrade cannot rotate live datastore passwords. A server-side dry run renders exactly the release's object set, adding and removing nothing. `emulate-ci-deploy.sh` runs the workflow's three stages from a laptop through an explicit kubeconfig, read-only unless `--apply` plus a typed confirmation token. It asserts the target cluster before anything runs, and prints every command it is about to execute so the local path and the CI path can be diffed rather than assumed. `--allow-dirty` swaps `make diff` for a bare `helm template` when the tree is not clean, which is the only way to rehearse changes that are not committed yet; it says so loudly rather than passing quietly. `provision-ci-deployer.sh` mints the namespace-scoped ServiceAccount the deploy runs as, and asserts its containment before handing over a kubeconfig. A token is deletable; a client certificate needs a CA rotation, which is why an admin kubeconfig is never the CI credential. Refs #2244 Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Closes the gap between "images built, chart published" and "a release anyone can actually use". On merge to main, `build-images.yml` now hands the version it just published to a reusable workflow that installs that exact chart on the shared test stand, seeds it, and proves a login and a metric through the public URL. This is an after-merge alarm, not a gate, and it cannot be anything else: the artefact it installs does not exist until publish-chart has run. Merge blocking stays with the merge-queue lanes; a red run here means main is broken, and the merge author owns it — fix forward if the cause is small, revert if it is not. Three separately-named stages with their own timeouts, so a red run says which one died without opening a log: deploy (no `--atomic`, because a rollback destroys the evidence and leaves the stand silently running an older chart than main), seed, then smoke — which never runs after a failed seed. The repo is public, so every run log is public forever. `stand-diagnostics.sh` is therefore a curated allowlist — release status, a pod table, warning reasons without messages, and the tail of failed containers only — piped through `redact-stand-log.py`. No describes, no environment dumps, no uploaded artefacts. Anything deeper needs the kubeconfig, deliberately. The diagnostics enumerate `--deployed --failed --pending --uninstalling` rather than `helm list --all`: `--all` is a Helm 3 flag that Helm 4 removed, and a diagnostics script runs precisely when something is already broken, so it can never be the thing that breaks. Refs #2244 Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Four checks against a real deployed stand through its public URL, in the order a person meets them: `/auth/login` redirects to the IdP, a seeded persona signs in, `/auth/me` returns that persona, and a metric returns data for the seeded window. Together they are the "a user can log in and see data" half of #2244 — the part a green `helm upgrade` cannot tell you. Everything runs through the public hostname. No port-forwarding, no in-cluster shortcut: DNS, TLS, the gateway and the IdP redirect are exactly the parts that work in a cluster and fail from a browser, so testing around them would test the wrong thing. Personas come from the seeder's manifest at collection time rather than a hardcoded list, so the suite follows the roster instead of drifting from it, and a missing manifest is a hard stop with an actionable message rather than a silent skip. Assertions are on shape and non-emptiness — series present, at least one point, no null values — never on exact numbers: reading a figure off a running stand and asserting it back proves nothing and breaks on every legitimate fixture change. `SMOKE_BASE_URL` and the credentials are required environment variables with no defaults, so the suite cannot quietly aim at somebody's local stack. The `stand_smoke` marker keeps it out of the compose lanes (`-m 'not stand_smoke'`). Refs #2244 Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
`secrets: inherit` passes the caller's ENTIRE secret set to the reusable workflow, including the GitHub App key that is on `main`'s branch-protection bypass list. The deploy needs none of them: its credentials live in the `insight-test-stand` environment, and environment secrets are resolved by the job that declares `environment:` — which is inside the called workflow. Semgrep's yaml.github-actions.security.secrets-inherit rule blocks this, and it is right to. Not inheriting fails loudly rather than silently: the first cluster-touching step refuses with a named error naming the missing secret. Refs #2244 Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…-namespace RBAC the chart needs
Two defects found by review, both of which only show up in production.
REDACTION. `_SECRET_KEY` began with `�`, which does not match between `_` and a
letter — underscore is a word character. Every underscore-joined key therefore
passed through the filter untouched, which is the dominant naming convention in
this stack:
MARIADB_PASSWORD=… leaked
kubectl_token=… leaked
APP__gears__authenticator__config__idp__client_secret: leaked
while a bare `password:` was correctly redacted, so the filter looked like it
worked. On a public repository those lines are published forever. The boundary
is now `(?<![A-Za-z0-9])` with a bounded prefix group, and the digest, chart
version and object-name cases still survive untouched.
CROSS-NAMESPACE RBAC. The umbrella chart renders `<release>-airbyte-auth-reader`
— a Role granting `get` on one Secret, plus its RoleBinding — into the namespace
named by `airbyte.namespace`, which this environment must set: leaving it empty
puts the RBAC where Airbyte's `airbyte-auth-secrets` does not exist and
connector provisioning fails at run time while every pod looks healthy. A
credential scoped only to the release namespace cannot write those two objects,
so the FIRST `helm upgrade` would have been refused — CI red before it deployed
anything. Verified against the stand: the live release owns both objects in that
namespace, and `can-i update rolebindings` there answers `no`.
The supplement is deliberately tiny: rights over `roles`/`rolebindings`, plus
`get` restricted by `resourceNames` to the single Secret the chart's own Role
names — the latter only because escalation prevention forbids creating a Role
granting a permission the creator does not hold. Removing it does not shrink the
credential, it breaks the deploy. Pods, other Secrets and workloads in that
namespace stay unreachable, and the script now asserts all three facts.
Refs #2244
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…ntials The smoke stage needs a login credential the deploy and seed stages do not, and the preflight demands every secret before the first stage runs. That is right for the merge path — being told twenty minutes in that a password is missing is worse than being told immediately — but it made "does this chart install and seed on this stand" unanswerable until an unrelated question about IdP credentials was settled. `stages` is a PREFIX of deploy -> seed -> smoke, never a set: each stage is the next one's precondition, and independent checkboxes would let someone smoke yesterday's data against today's chart and call it a pass. The preflight now requires the smoke credentials only when smoke will actually run, and still validates the login-mode string either way so a typo is caught on the run that introduces it. A partial run is a rehearsal, not a green light, and the workflow says so three times rather than trusting anyone to remember: a warning annotation, a stages row plus a blockquote in the job summary, and the skipped steps in the UI. The ref guard is now absolute for `workflow_call` and advisory for `workflow_dispatch`. A call arrives from build-images.yml after a publish, which only happens on main, so a call from any other ref means the caller's guard broke. A dispatch is a person asking on purpose — usually to rehearse this workflow on the branch that changes it, which cannot be done from main by definition. What decides whether a dispatch may touch the stand is the environment's deployment-branch policy, enforced before the job starts. Refs #2244 Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…he token is a stopgap Two things the runbook set up correctly but never argued, which is how correct setup gets undone by a well-meaning later edit. The allow-list reads like bookkeeping. It is the security control. No secret store protects a value from the people who can run jobs that read it: an environment secret is readable by any job declaring that environment on an allowed branch, and someone with write access can add a step that prints it. What bounds this credential is "allow-list says main" AND "main is branch-protected" — together, a change that reads the secret must survive review first. Widening the list does not degrade that property, it removes it. Recorded alongside the two consequences already in the tree: why fork PRs cannot read it, and why `secrets: inherit` had to go. Also recorded: a rehearsal environment on a personal fork may legitimately allow a feature branch, and that is precisely why it must stay on the fork and never be copied into the org repo. And the honest framing of what this credential is. A GitHub environment secret is a static string with no refresh path, so the token is long-lived by necessity rather than preference — an accepted trade given the asserted blast radius and one-command revocation, but one whose missing property is expiry. The 90-day cadence in §5 is therefore an obligation, not a suggestion: nothing else ages this credential out. The durable answer, noted so it is a decision rather than a gap, is trusting GitHub's OIDC issuer from the API server and minting a short-lived token per run — a cluster-operator change, which is the only reason it is not the starting point. Refs #2244 Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
The stand was rebuilt from the reference deployment repository, and two things this environment asserted stopped being true. THE EDGE. The umbrella grew native Gateway API templates, so `gateway.route`, `keycloak.route` and `frontend.route` replace the `ingress` keys and the release renders and owns both HTTPRoutes. The two manifests this directory carried are therefore deleted rather than kept: a hand-applied copy beside a release that renders the same object is a second writer, and the check that guarded them has not disappeared but inverted — `helm --wait` does not wait on HTTPRoute status, so the deploy now asserts `Accepted` on what it has just written. THE IdP. The stand runs `INSIGHT_LOGIN_MODE=seeded`: realm `insight`, generated from the demo seed's own roster with local password users, not the GitHub-federated `insight-broker`. So `sourceType` is `keycloak` and the external-IdP prose is gone from the Secret and scope comments — there is no external IdP here to describe. `externalIdClaim` stays `idp_sub` and now says why it is not `sub`: the generator sets each user's id to the roster uuid, which would land in `sub` on a realm import, but the chart applies realms with keycloak-config-cli, which creates users through the admin REST API where Keycloak assigns its own id and discards the document's. A "fix" to `sub` produces a stand where every persona authenticates and is then denied, against a fully populated identity projection, with nothing pointing back at that line. A server-side dry run of chart 0.5.111 with this values file renders 38 objects and `helm get manifest` of the live release is 38 objects — nothing added, nothing removed. The comment corrections changed no value: parsed YAML is identical to the previous revision. INFRA.md is new, and answers the question this tree could not: what has to exist beneath the umbrella. Layer model, deploy order with its causal edges, a version-pinned component inventory, and — the part worth the file — 29 load-bearing settings each written as the setting, what breaks without it, and how the breakage presents. It opens with `maxUserConnections: 100`, because at the operator's default of 10 that one account cannot back four connection pools and the symptom is not a recognisable connection limit: two services crash-loop while every other pod stays Ready, and the upgrade dies blaming a client rate limiter. Refs #2244 Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…-sync the rest to the stand
THE BUG. The manifest capture scanned for a `{` at column 0, took the next `}`
at column 0, and parsed everything between. The seeder prints the manifest to
stdout while its logging handler writes to stderr, and both `kubectl logs` and
the shell redirection that captures a seed run merge the two — drained
independently, so a twelve-kilobyte document does not arrive in one piece and a
log line written mid-drain lands BETWEEN two of the document's lines. The seeder
logs immediately after printing it, which makes this the ordinary case. One
interleaved line and `json.loads` raised, the step failed, and the smoke stage
aborted for want of a manifest — on a run whose deploy and seed had both
succeeded, presenting as a seeding problem.
The fix drops lines matching the logger's shape (the SHAPE, so a persona's
display name is never filtered out of the document), then tries the surviving
column-0 spans latest-open-first and accepts only one that parses AND carries
`manifest_version` — which covers what the filter cannot, like a traceback or a
helper writing on the container's own descriptors.
It lives in a committed script because two callers need it. They were two
implementations; when the CI one was fixed the harness's was not, and its
comment went on claiming the two matched — a parity report asserting a parity it
no longer had, which is worse than not checking. One algorithm now, invoked by
both, so neither can drift without deleting the file.
THE REST is re-synced to the redeployed stand. The route-check comments said the
chart renders no HTTPRoute and that CI would be a second writer; both are false
now that the release owns them, and the CI credential's Gateway API grant is
more necessary for it, not less. The workflow header claimed it does no
`kubectl rollout restart` while carrying that very step. The smoke suite was
written for a stand where password login was impossible; the seeded roster realm
serves a password form, so `password` is the mode it defaults to and the long
explanation of why it could not work is replaced by what is now true.
Refs #2244
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…roster CI owns this stand's state: every merge redeploys and reseeds it, and the demo seeder writes the roster that both the Keycloak realm and `identity.persons` are generated from. One roster, two projections that have to agree — or a login authenticates and then resolves to nobody. The persons-seed CronJob rebuilds that same projection from a different source: ClickHouse `identity_inputs`, which connectors populate. A stand with no connectors has none, so today it fails its own input guard every morning and writes nothing — the guard working, not a fault, but a failed Job in every diagnostics dump. The moment connectors did land data here it would stop being harmless and start competing with the seeded roster for the same table. Turned off in this environment's values rather than with `kubectl patch cronjob … suspend=true`: the CronJob is chart-rendered and helm-owned, so a hand-suspend is reverted by the next `helm upgrade` — which on this stand is the next merge. `sync` stays on. It publishes the seeded roster into ClickHouse, where the metrics models resolve email to person_id; disabling it would leave every seeded metric unable to attribute anything. Verified against the live release: a server-side dry run now renders 37 objects where the release has 38, and the single difference is this CronJob disappearing. Nothing else moves. Refs #2244 Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
The seeder refuses to add demo people to a tenant that already holds `identity.persons` rows it did not write. That refusal is not fussiness: the silver step CLEARS the tables it writes, so on a tenant carrying data from somewhere else a reseed is data loss rather than a reset. There are stands where the right answer is to override it anyway — a CI-owned stand whose only foreign rows are a stale artifact of a login model that has since been replaced, for instance. `--force` exists for exactly that, and until now the workflow had no way to pass it, so the only route was editing the invocation and merging the override into every future run. `seed_force` is therefore a `workflow_dispatch` input, defaulted false, and deliberately absent from `workflow_call`: a merge cannot reach that branch at all. Forcing stays a considered answer to a refusal a human has read, and the run says so with a warning annotation rather than passing quietly. Refs #2244 Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
… run — delete before merge" This reverts commit 19fd619. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
ktursunov
marked this pull request as ready for review
August 12, 2026 05:50
ktursunov
enabled auto-merge
August 12, 2026 05:50
added 2 commits
August 12, 2026 14:37
…uments From docs/components/deployment/specs/sop/ into the test-stand environment dir, beside the RBAC manifest and Makefile it drives. Relative links and the four inbound references updated; README links it. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…lines) Same content, one row per fact instead of three paragraphs: the 29 load-bearing settings and 6 incidents become tables (correct value -> the misleading symptom), the bootstrap and recreate runbooks tighten to terse lists. Every version, name and failure signature is preserved. README's one heading-anchor link repointed to the settings section. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (3)
tests/stand/ui/test_logged_out_access_refused.py (1)
46-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the docstring: the assertion is now IdP-specific.
The docstring at lines 48-50 states that a stand pointed at a different IdP should still pass. Line 65 asserts a Keycloak path layout, and the comment at line 63 says so directly. A non-Keycloak IdP now fails this test.
KeycloakLoginPageat line 58 already binds the test to Keycloak, so the docstring is the stale part.♻️ Proposed change
- Asserted on the FORM rather than on the URL containing "keycloak": a stand - pointed at a different IdP should still pass this, and what matters to a user - is that they were asked to sign in, not which host asked them. + Asserted on the FORM first: what matters to a user is that they were asked to + sign in, not which host asked them. The URL check that follows reads Keycloak's + own path layout, so this case is Keycloak-bound, like `KeycloakLoginPage`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/stand/ui/test_logged_out_access_refused.py` around lines 46 - 68, Update the test docstring near KeycloakLoginPage and the URL-path assertion to state that the test expects the Keycloak authorization form/path, removing the claim that any different IdP should pass. Keep the existing assertion and explanatory comments unchanged.tests/stand/conftest.py (1)
352-376: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the optional seed skip.
_manifest()already cachesManifestin_MANIFEST, so repeated calls do not reload it. Pytest evaluates skip marks during setup, but no test covers thispytest_collection_finishpath. Add a focused test that asserts an item requiring onlyother_tenant_leadis skipped before login.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/stand/conftest.py` around lines 352 - 376, Add focused test coverage for the collection-finish handling around the requires_seed marker, constructing an item that requires only other_tenant_lead and asserting it receives a skip marker before login. Reuse the existing manifest/cache setup and verify the skip reason identifies the optional seed, without covering items that also require canonical seeds.src/ingestion/tools/seed/insight_seed/keycloak_realm.py (1)
38-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep each new explanatory comment on one line.
src/ingestion/tools/seed/insight_seed/keycloak_realm.py#L38-L43: split separate reasons into separate one-line comments.src/ingestion/tools/seed/insight_seed/manifest.py#L94-L95: reduce the runtime-password explanation to one line.src/ingestion/tools/seed/insight_seed/manifest.py#L348-L349: reduce the sentinel transport explanation to one line.src/ingestion/tools/seed/insight_seed/manifest.py#L352-L353: reduce the CRI limit explanation to one line.src/ingestion/tools/seed/insight_seed/manifest.py#L372-L373: reduce the scrub-gate explanation to one line.As per coding guidelines: “Add comments only when code cannot express the reason, such as intentional redundancy, cross-function invariants, or reasoned workarounds; keep them to one line.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ingestion/tools/seed/insight_seed/keycloak_realm.py` around lines 38 - 43, Rework the explanatory comments near _ENV_PASSWORD in src/ingestion/tools/seed/insight_seed/keycloak_realm.py lines 38-43 into separate one-line comments for each reason. In src/ingestion/tools/seed/insight_seed/manifest.py, condense the explanations at lines 94-95, 348-349, 352-353, and 372-373 into one-line comments, preserving their meaning while following the guideline to comment only on non-obvious invariants or workarounds.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/deploy-test-stand.yml:
- Around line 277-289: Update the deploy_secret step’s final conditional so an
empty OIDC secret does not produce a non-zero status under set -euo pipefail.
Replace the short-circuit `[ -n "$s" ] && echo "::add-mask::$s"` in the masking
block with a full if statement, preserving masking when a value exists and
ensuring the step completes successfully when it is absent.
- Around line 103-115: Update the command substitutions in
.github/workflows/deploy-test-stand.yml at lines 103-115 and 392-399 to append
|| true to the gh api/base64/tr pipeline and the curl/jq pipeline, respectively.
This allows the existing empty-value checks and crafted diagnostics to run when
either pipeline fails; no direct change is needed to the surrounding error
messages.
- Around line 501-527: Provide a fallback value for CHART_VERSION before the
summary step expands it, so set -u does not abort when the chart resolver fails
to populate the environment. Update the summary block around render and the
“Test stand” header, preserving the resolved version when available and using
the default only when it is unset.
- Around line 336-365: Update the deploy_routes step to normalize kubectl’s
single-route response into an iterable collection before the jq pipelines, and
make both `.status.parents[]` traversals optional so missing status does not
produce a jq error. Preserve the existing diagnosis for non-Accepted parents,
and explicitly detect routes with no populated status if the step must fail
loudly for that state.
In @.github/workflows/scripts/redact-stand-log.py:
- Around line 99-139: Update _clean to cap the input line to max_line before
running any credential-masking regexes, while retaining the existing output
truncation notice when content was dropped. Ensure lines within the limit keep
their current masking behavior and avoid scanning the full oversized input.
In `@deploy/gitops/environments/test-stand/credentials-runbook.md`:
- Around line 77-91: The runbook’s provisioning checklist and escalation
troubleshooting section still describe the obsolete ClusterRole/admin binding
and permissions. Update both sections to reference Role/ci-deployer-core and
Role/ci-deployer-crd-supplement, accurately listing their namespace-scoped rules
and preserving the existing guidance about the airbyte namespace.
In `@deploy/gitops/environments/test-stand/README.md`:
- Around line 237-242: Update the seed command in the README to pass --days 365
instead of --days 730, matching the required validation window and
rebuild-script limit.
In `@deploy/gitops/environments/test-stand/values.yaml`:
- Around line 482-498: Set the overrideEnabled configuration to false before
publishing the stand, disabling the unauthorised impersonation flow while
leaving the surrounding environment configuration unchanged.
In `@deploy/gitops/Makefile`:
- Around line 509-577: Reject chart downgrades by comparing the requested
INSIGHT_VERSION with the currently deployed chart version before any destructive
or upgrade action. In deploy/gitops/Makefile, add this validation before helm
upgrade in the deployment flow; in deploy/gitops/scripts/recreate-test-stand.sh,
perform the same comparison before its destructive recreation phase. Allow equal
or newer versions, and exit with a clear error for lower requested versions.
In `@deploy/gitops/scripts/recreate-test-stand.sh`:
- Around line 426-432: Update the probe loop around BASE and the probe status
comparison so an unexpected response sets a non-zero exit status, while
retaining the existing failure message and checking both probes. Ensure the
script does not reach its successful completion path after either probe fails.
- Around line 125-152: Extend the survivor precheck before the uninstall to
validate required keys and non-empty values for every object in SURVIVORS_SECRET
and SURVIVORS_CM, not just object existence or insight-oidc. Reuse existing
validation helpers such as oidc_secret_nonempty where applicable, set missing=1
for any absent or empty required content, and retain the refusal path before
databases are deleted.
In `@src/ingestion/tools/seed/insight_seed/manifest.py`:
- Around line 93-98: Update the manifest validation around assert_no_credentials
in the seed manifest generation flow to inspect manifest keys and string values
before json.dumps serialization. Compare these logical values directly against
_FORBIDDEN_LITERALS, while preserving existing validation of serialized output
and ensuring forbidden passwords cannot reach the manifest file or sentinel log.
In `@tests/stand/api/analytics/__init__.py`:
- Around line 58-59: In the date-window calculation containing earliest and the
returned ISO-formatted bounds, validate that start is less than or equal to end
before calculating earliest. Reject reversed windows such as a start date after
the end date while preserving the existing max_days clamping for valid ranges.
In `@tests/stand/api/analytics/test_results.py`:
- Around line 86-99: Update the xfail handling for the stand smoke test around
the pytest marker so the missing metric value cannot be reported as a passing
result. Remove the non-strict pytest.mark.xfail applied to this test, or ensure
the stand smoke invocation runs with --runxfail, while preserving the existing
stand_smoke coverage.
---
Nitpick comments:
In `@src/ingestion/tools/seed/insight_seed/keycloak_realm.py`:
- Around line 38-43: Rework the explanatory comments near _ENV_PASSWORD in
src/ingestion/tools/seed/insight_seed/keycloak_realm.py lines 38-43 into
separate one-line comments for each reason. In
src/ingestion/tools/seed/insight_seed/manifest.py, condense the explanations at
lines 94-95, 348-349, 352-353, and 372-373 into one-line comments, preserving
their meaning while following the guideline to comment only on non-obvious
invariants or workarounds.
In `@tests/stand/conftest.py`:
- Around line 352-376: Add focused test coverage for the collection-finish
handling around the requires_seed marker, constructing an item that requires
only other_tenant_lead and asserting it receives a skip marker before login.
Reuse the existing manifest/cache setup and verify the skip reason identifies
the optional seed, without covering items that also require canonical seeds.
In `@tests/stand/ui/test_logged_out_access_refused.py`:
- Around line 46-68: Update the test docstring near KeycloakLoginPage and the
URL-path assertion to state that the test expects the Keycloak authorization
form/path, removing the claim that any different IdP should pass. Keep the
existing assertion and explanatory comments unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e4f9eba2-f5e6-4063-8f7f-4a5af2241a12
📒 Files selected for processing (26)
.cf-studio/config/README.md.cf-studio/config/core.toml.github/workflows/deploy-test-stand-on-publish.yml.github/workflows/deploy-test-stand.yml.github/workflows/scripts/redact-stand-log.pyCLAUDE.mddeploy/gitops/Makefiledeploy/gitops/environments/test-stand/INFRA.mddeploy/gitops/environments/test-stand/README.mddeploy/gitops/environments/test-stand/ci-deployer-rbac.yamldeploy/gitops/environments/test-stand/credentials-runbook.mddeploy/gitops/environments/test-stand/inventory.yamldeploy/gitops/environments/test-stand/values.yamldeploy/gitops/scripts/lib.shdeploy/gitops/scripts/provision-ci-deployer.shdeploy/gitops/scripts/recreate-test-stand.shsrc/ingestion/tools/seed/insight_seed/keycloak_realm.pysrc/ingestion/tools/seed/insight_seed/manifest.pysrc/ingestion/tools/seed/seed-stand.shtests/pyproject.tomltests/stand/api/analytics/__init__.pytests/stand/api/analytics/test_results.pytests/stand/api/identity/test_subchart.pytests/stand/api/test_auth_me.pytests/stand/conftest.pytests/stand/ui/test_logged_out_access_refused.py
Comment on lines
+277
to
+289
| - name: 'Stage 1/4 — mask the OIDC client secret' | ||
| id: deploy_secret | ||
| run: | | ||
| set -euo pipefail | ||
| secret_name="$(yq -r '.oidcClientSecret.secret // ""' "$INVENTORY_FILE")" | ||
| secret_key="$(yq -r '.oidcClientSecret.key // ""' "$INVENTORY_FILE")" | ||
| if [ -n "$secret_name" ] && [ -n "$secret_key" ]; then | ||
| s="$( | ||
| kubectl -n "$STAND_NAMESPACE" get secret "$secret_name" \ | ||
| -o "jsonpath={.data['$secret_key']}" 2>/dev/null | base64 --decode 2>/dev/null || true | ||
| )" | ||
| [ -n "$s" ] && echo "::add-mask::$s" | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
The masking step fails the deploy whenever the OIDC secret is absent.
Line 288 is the last command of the step. [ -n "$s" ] && echo ... returns 1 when $s is empty. The enclosing if block then returns 1, and the step exits non-zero under set -euo pipefail.
This path is reachable on a first install, and also when the ServiceAccount cannot read the secret, because line 286 ends with || true. The stand then fails at Stage 1/4 before make deploy runs.
Use a full if block, so the step always ends with status 0.
🐛 Proposed fix
- [ -n "$s" ] && echo "::add-mask::$s"
+ if [ -n "$s" ]; then
+ echo "::add-mask::$s"
+ fi
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: 'Stage 1/4 — mask the OIDC client secret' | |
| id: deploy_secret | |
| run: | | |
| set -euo pipefail | |
| secret_name="$(yq -r '.oidcClientSecret.secret // ""' "$INVENTORY_FILE")" | |
| secret_key="$(yq -r '.oidcClientSecret.key // ""' "$INVENTORY_FILE")" | |
| if [ -n "$secret_name" ] && [ -n "$secret_key" ]; then | |
| s="$( | |
| kubectl -n "$STAND_NAMESPACE" get secret "$secret_name" \ | |
| -o "jsonpath={.data['$secret_key']}" 2>/dev/null | base64 --decode 2>/dev/null || true | |
| )" | |
| [ -n "$s" ] && echo "::add-mask::$s" | |
| fi | |
| - name: 'Stage 1/4 — mask the OIDC client secret' | |
| id: deploy_secret | |
| run: | | |
| set -euo pipefail | |
| secret_name="$(yq -r '.oidcClientSecret.secret // ""' "$INVENTORY_FILE")" | |
| secret_key="$(yq -r '.oidcClientSecret.key // ""' "$INVENTORY_FILE")" | |
| if [ -n "$secret_name" ] && [ -n "$secret_key" ]; then | |
| s="$( | |
| kubectl -n "$STAND_NAMESPACE" get secret "$secret_name" \ | |
| -o "jsonpath={.data['$secret_key']}" 2>/dev/null | base64 --decode 2>/dev/null || true | |
| )" | |
| if [ -n "$s" ]; then | |
| echo "::add-mask::$s" | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/deploy-test-stand.yml around lines 277 - 289, Update the
deploy_secret step’s final conditional so an empty OIDC secret does not produce
a non-zero status under set -euo pipefail. Replace the short-circuit `[ -n "$s"
] && echo "::add-mask::$s"` in the masking block with a full if statement,
preserving masking when a value exists and ensuring the step completes
successfully when it is absent.
Comment on lines
+336
to
+365
| - name: 'Stage 1/4 — confirm the edge still routes' | ||
| id: deploy_routes | ||
| run: | | ||
| set -euo pipefail | ||
| # Every entry in .status.parents[], not just [0]: Accepted[0] with | ||
| # Refused[1] is still unreachable. `helm --wait` never checks this. | ||
| # shellcheck disable=SC2086 # ROUTE_NAMES is a deliberate word list | ||
| routes_json="$(kubectl -n "$STAND_NAMESPACE" get httproute $ROUTE_NAMES -o json)" | ||
| printf '%s' "$routes_json" | jq -r ' | ||
| .items[] | .metadata.name as $n | ||
| | .status.parents[] | "\($n) parentRef=\(.parentRef.name // "?") accepted=\((.conditions[]? | select(.type=="Accepted") | .status) // "Unknown")" | ||
| ' | ||
| not_accepted="$( | ||
| printf '%s' "$routes_json" | jq -r ' | ||
| .items[] | .metadata.name as $n | ||
| | .status.parents[] | ||
| | select((.conditions[]? | select(.type=="Accepted") | .status) != "True") | ||
| | "\($n) parentRef=\(.parentRef.name // "?")" | ||
| ' | ||
| )" | ||
| if [ -n "$not_accepted" ]; then | ||
| echo "::error::an edge route has a parentRef that is not Accepted:" | ||
| printf '%s\n' "$not_accepted" | ||
| echo "The release is 'deployed' and its pods are Ready; the stand is still unreachable," | ||
| echo "and the smoke below would fail at its first request with a much less useful" | ||
| echo "message. The route came out of this deploy, so start in the values file that" | ||
| echo "produced it — $VALUES_FILE, keys gateway.route and keycloak.route — and check the" | ||
| echo "parentRef they name against the Gateway the cluster actually has." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A route without a populated .status aborts this step with a jq error instead of the written diagnosis.
Line 344 and line 349 iterate .status.parents[]. A freshly reconciled HTTPRoute can have no status yet. jq then fails with "Cannot iterate over null", the pipeline exits under pipefail, and the operator never sees the message at lines 357-364.
Line 343 also depends on ROUTE_NAMES holding two or more names: with a single name, kubectl get returns one object and .items[] fails.
Use optional iteration and normalise the single-object shape.
🐛 Proposed fix
- routes_json="$(kubectl -n "$STAND_NAMESPACE" get httproute $ROUTE_NAMES -o json)"
+ routes_json="$(kubectl -n "$STAND_NAMESPACE" get httproute $ROUTE_NAMES -o json \
+ | jq '{items: (.items // [.])}')"
printf '%s' "$routes_json" | jq -r '
.items[] | .metadata.name as $n
- | .status.parents[] | "\($n) parentRef=\(.parentRef.name // "?") accepted=\((.conditions[]? | select(.type=="Accepted") | .status) // "Unknown")"
+ | (.status.parents[]? // empty) | "\($n) parentRef=\(.parentRef.name // "?") accepted=\((.conditions[]? | select(.type=="Accepted") | .status) // "Unknown")"
'
not_accepted="$(
printf '%s' "$routes_json" | jq -r '
.items[] | .metadata.name as $n
- | .status.parents[]
+ | (.status.parents[]? // empty)
| select((.conditions[]? | select(.type=="Accepted") | .status) != "True")
| "\($n) parentRef=\(.parentRef.name // "?")"
'
)"A route with no status at all then reports nothing; add an explicit count check if you want that state to fail loudly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/deploy-test-stand.yml around lines 336 - 365, Update the
deploy_routes step to normalize kubectl’s single-route response into an iterable
collection before the jq pipelines, and make both `.status.parents[]` traversals
optional so missing status does not produce a jq error. Preserve the existing
diagnosis for non-Accepted parents, and explicitly detect routes with no
populated status if the step must fail loudly for that state.
Comment on lines
+501
to
+527
| run: | | ||
| set -euo pipefail | ||
|
|
||
| # Worst-first: the case arms must stay in this order, or a | ||
| # half-failed stage renders as a pass. | ||
| render() { | ||
| local outcomes="$1" | ||
| case " $outcomes " in | ||
| *" failure "*) echo "**failed**" ;; | ||
| *" cancelled "*) echo "cancelled" ;; | ||
| *" skipped "*) | ||
| case " $outcomes " in | ||
| *" success "*) echo "**incomplete** — started, did not finish" ;; | ||
| *) echo "— not run" ;; | ||
| esac | ||
| ;; | ||
| *" success "*) echo "passed" ;; | ||
| *) echo "— not run" ;; | ||
| esac | ||
| } | ||
|
|
||
| { | ||
| echo "### Test stand — \`$CHART_VERSION\`" | ||
| echo "" | ||
| echo "| | |" | ||
| echo "|---|---|" | ||
| echo "| chart | \`insight-$CHART_VERSION\` |" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
The summary step aborts when the chart resolver failed.
Line 502 sets set -euo pipefail, and line 523 expands $CHART_VERSION. The resolver at lines 82-120 writes CHART_VERSION to $GITHUB_ENV only on success. If that step fails, nounset makes this step exit before it writes any summary, so the run that most needs the summary produces none.
Provide a default.
🐛 Proposed fix
set -euo pipefail
+ chart_version="${CHART_VERSION:-unresolved}"
@@
- echo "### Test stand — \`$CHART_VERSION\`"
+ echo "### Test stand — \`$chart_version\`"
echo ""
echo "| | |"
echo "|---|---|"
- echo "| chart | \`insight-$CHART_VERSION\` |"
+ echo "| chart | \`insight-$chart_version\` |"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| run: | | |
| set -euo pipefail | |
| # Worst-first: the case arms must stay in this order, or a | |
| # half-failed stage renders as a pass. | |
| render() { | |
| local outcomes="$1" | |
| case " $outcomes " in | |
| *" failure "*) echo "**failed**" ;; | |
| *" cancelled "*) echo "cancelled" ;; | |
| *" skipped "*) | |
| case " $outcomes " in | |
| *" success "*) echo "**incomplete** — started, did not finish" ;; | |
| *) echo "— not run" ;; | |
| esac | |
| ;; | |
| *" success "*) echo "passed" ;; | |
| *) echo "— not run" ;; | |
| esac | |
| } | |
| { | |
| echo "### Test stand — \`$CHART_VERSION\`" | |
| echo "" | |
| echo "| | |" | |
| echo "|---|---|" | |
| echo "| chart | \`insight-$CHART_VERSION\` |" | |
| run: | | |
| set -euo pipefail | |
| chart_version="${CHART_VERSION:-unresolved}" | |
| # Worst-first: the case arms must stay in this order, or a | |
| # half-failed stage renders as a pass. | |
| render() { | |
| local outcomes="$1" | |
| case " $outcomes " in | |
| *" failure "*) echo "**failed**" ;; | |
| *" cancelled "*) echo "cancelled" ;; | |
| *" skipped "*) | |
| case " $outcomes " in | |
| *" success "*) echo "**incomplete** — started, did not finish" ;; | |
| *) echo "— not run" ;; | |
| esac | |
| ;; | |
| *" success "*) echo "passed" ;; | |
| *) echo "— not run" ;; | |
| esac | |
| } | |
| { | |
| echo "### Test stand — \`$chart_version\`" | |
| echo "" | |
| echo "| | |" | |
| echo "|---|---|" | |
| echo "| chart | \`insight-$chart_version\` |" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/deploy-test-stand.yml around lines 501 - 527, Provide a
fallback value for CHART_VERSION before the summary step expands it, so set -u
does not abort when the chart resolver fails to populate the environment. Update
the summary block around render and the “Test stand” header, preserving the
resolved version when available and using the default only when it is unset.
Comment on lines
+99
to
+139
| def _clean(line: str, *, max_line: int) -> str: | ||
| """Return `line` with every credential-shaped run replaced. | ||
|
|
||
| Order is load-bearing. URLs go first so a DSN's password is masked as a URL | ||
| credential rather than half-caught by the email rule; digests are parked | ||
| before the long-blob sweep and restored after it; the verification pass runs | ||
| last, over the bytes that are actually about to be printed. | ||
| """ | ||
| if _PRIVATE_KEY_HEADER.search(line): | ||
| # The header names the key type and nothing else useful, and the body | ||
| # is caught by the blob rule. A PEM in a CI log is never a diagnostic. | ||
| return "[private key material withheld by CI]" | ||
|
|
||
| out = _URL_CREDENTIALS.sub(r"\g<scheme>[credentials redacted]@", line) | ||
| out = _JWT.sub("[jwt redacted]", out) | ||
| out = _BEARER.sub(r"\1 [redacted]", out) | ||
| out = _SESSION_COOKIE.sub(r"\1=[redacted]", out) | ||
| out = _SECRET_KEY.sub(r"\1\2[redacted]", out) | ||
| out = _EMAIL.sub(_email_slot, out) | ||
|
|
||
| digests: list[str] = [] | ||
|
|
||
| def _park(match: re.Match[str]) -> str: | ||
| digests.append(match.group(0)) | ||
| return _DIGEST_SLOT.format(len(digests) - 1) | ||
|
|
||
| out = _DIGEST.sub(_park, out) | ||
| out = _LONG_BLOB.sub(_blob_mask, out) | ||
| for index, digest in enumerate(digests): | ||
| out = out.replace(_DIGEST_SLOT.format(index), digest) | ||
|
|
||
| out = _IPV4.sub("[ip redacted]", out) | ||
|
|
||
| if len(out) > max_line: | ||
| out = out[:max_line] + f" …[truncated at {max_line} chars by CI]" | ||
|
|
||
| # The result, not the input: anything credential-shaped that survived means | ||
| # a rule has a hole. Digests stripped first so an exempt one cannot trip it. | ||
| if _EMAIL.search(out) or _JWT.search(out) or _PRIVATE_KEY_HEADER.search(out) or _blob_survivor(out): | ||
| return LINE_MARKER | ||
| return out |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Truncate the input before the rules run, not after.
Line 132 applies max_line to the output. Every regex at lines 107-130 therefore scans the full input line first. A pod log or a helm error can carry a single line of many megabytes, and _SECRET_KEY at lines 52-59 backtracks over [A-Za-z0-9_.\-]* at each start position, so scan cost grows faster than line length. The step then stalls inside the redactor with no output.
Cap the input first. The masking result stays the same for anything that fits, and the truncation notice still tells the reader that content was dropped.
♻️ Proposed change
if _PRIVATE_KEY_HEADER.search(line):
# The header names the key type and nothing else useful, and the body
# is caught by the blob rule. A PEM in a CI log is never a diagnostic.
return "[private key material withheld by CI]"
- out = _URL_CREDENTIALS.sub(r"\g<scheme>[credentials redacted]@", line)
+ truncated = len(line) > max_line
+ out = _URL_CREDENTIALS.sub(r"\g<scheme>[credentials redacted]@", line[:max_line])
@@
- if len(out) > max_line:
+ if truncated or len(out) > max_line:
out = out[:max_line] + f" …[truncated at {max_line} chars by CI]"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/scripts/redact-stand-log.py around lines 99 - 139, Update
_clean to cap the input line to max_line before running any credential-masking
regexes, while retaining the existing output truncation notice when content was
dropped. Ensure lines within the limit keep their current masking behavior and
avoid scanning the full oversized input.
Comment on lines
+125
to
+152
| # Checked BEFORE the uninstall, because after it they are the only way back. | ||
| hdr "survivors — what must outlive the wipe" | ||
|
|
||
| missing=0 | ||
| for s in "${SURVIVORS_SECRET[@]}"; do | ||
| if "${KUBECTL[@]}" -n "$NAMESPACE" get secret "$s" -o name >/dev/null 2>&1; then | ||
| note "${C_GRN}ok${C_RST} secret/$s" | ||
| else | ||
| note "${C_RED}MISSING${C_RST} secret/$s"; missing=1 | ||
| fi | ||
| done | ||
| for c in "${SURVIVORS_CM[@]}"; do | ||
| if "${KUBECTL[@]}" -n "$NAMESPACE" get configmap "$c" -o name >/dev/null 2>&1; then | ||
| note "${C_GRN}ok${C_RST} configmap/$c" | ||
| else | ||
| note "${C_RED}MISSING${C_RST} configmap/$c"; missing=1 | ||
| fi | ||
| done | ||
|
|
||
| # Presence is not enough: an empty client-secret leaves every pod Ready and | ||
| # every login failing at the code exchange. | ||
| if "${KUBECTL[@]}" -n "$NAMESPACE" get secret insight-oidc -o name >/dev/null 2>&1; then | ||
| oidc_secret_nonempty "$NAMESPACE" insight-oidc client-secret || missing=1 # from lib.sh | ||
| fi | ||
|
|
||
| if [ "$missing" = "1" ]; then | ||
| die "refusing to wipe: something the rebuilt stand needs is absent. Restore it first — these objects are created by the bring-up outside this repository, not by the chart." | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate survivor contents before deleting databases.
This check verifies only object existence, except for insight-oidc. An empty realm ConfigMap, signing-key Secret, admin Secret, or config Secret passes the precheck. The script can then delete the databases and leave the rebuilt release unable to start. Validate the required keys and non-empty ConfigMap data for every survivor before the uninstall.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/scripts/recreate-test-stand.sh` around lines 125 - 152, Extend
the survivor precheck before the uninstall to validate required keys and
non-empty values for every object in SURVIVORS_SECRET and SURVIVORS_CM, not just
object existence or insight-oidc. Reuse existing validation helpers such as
oidc_secret_nonempty where applicable, set missing=1 for any absent or empty
required content, and retain the refusal path before databases are deleted.
Comment on lines
+426
to
+432
| BASE="https://$(yq -r '.gateway.route.host' "$VALUES")" | ||
| for probe in "/:200" "/auth/me:401"; do | ||
| path="${probe%:*}"; want="${probe##*:}" | ||
| code="$(curl -s -o /dev/null -w '%{http_code}' -m 20 "$BASE$path" || echo 000)" | ||
| if [ "$code" = "$want" ]; then note "${C_GRN}ok${C_RST} $code $path" | ||
| else note "${C_RED}FAIL${C_RST} $code $path (expected $want)"; fi | ||
| done |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail the rebuild when an edge probe fails.
The failure branch only prints FAIL, then the script reaches the successful completion path. Return non-zero when either probe has an unexpected status.
Proposed fix
+ PROBE_FAILURES=0
for probe in "/:200" "/auth/me:401"; do
path="${probe%:*}"; want="${probe##*:}"
code="$(curl -s -o /dev/null -w '%{http_code}' -m 20 "$BASE$path" || echo 000)"
if [ "$code" = "$want" ]; then note "${C_GRN}ok${C_RST} $code $path"
- else note "${C_RED}FAIL${C_RST} $code $path (expected $want)"; fi
+ else
+ note "${C_RED}FAIL${C_RST} $code $path (expected $want)"
+ PROBE_FAILURES=$((PROBE_FAILURES + 1))
+ fi
done
+ [ "$PROBE_FAILURES" -eq 0 ] || die "public edge validation failed"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| BASE="https://$(yq -r '.gateway.route.host' "$VALUES")" | |
| for probe in "/:200" "/auth/me:401"; do | |
| path="${probe%:*}"; want="${probe##*:}" | |
| code="$(curl -s -o /dev/null -w '%{http_code}' -m 20 "$BASE$path" || echo 000)" | |
| if [ "$code" = "$want" ]; then note "${C_GRN}ok${C_RST} $code $path" | |
| else note "${C_RED}FAIL${C_RST} $code $path (expected $want)"; fi | |
| done | |
| BASE="https://$(yq -r '.gateway.route.host' "$VALUES")" | |
| PROBE_FAILURES=0 | |
| for probe in "/:200" "/auth/me:401"; do | |
| path="${probe%:*}"; want="${probe##*:}" | |
| code="$(curl -s -o /dev/null -w '%{http_code}' -m 20 "$BASE$path" || echo 000)" | |
| if [ "$code" = "$want" ]; then note "${C_GRN}ok${C_RST} $code $path" | |
| else | |
| note "${C_RED}FAIL${C_RST} $code $path (expected $want)" | |
| PROBE_FAILURES=$((PROBE_FAILURES + 1)) | |
| fi | |
| done | |
| [ "$PROBE_FAILURES" -eq 0 ] || die "public edge validation failed" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/scripts/recreate-test-stand.sh` around lines 426 - 432, Update
the probe loop around BASE and the probe status comparison so an unexpected
response sets a non-zero exit status, while retaining the existing failure
message and checking both probes. Ensure the script does not reach its
successful completion path after either probe fails.
Comment on lines
+93
to
+98
| "insight-dev", # keycloak_realm dev password (the default) | ||
| # The runtime persona password too, when a stand overrides the default — | ||
| # mirrors keycloak_realm.DEV_PASSWORD without importing it (no cycle). | ||
| os.environ.get( | ||
| "INSIGHT_SEED_PERSONA_PASSWORD", "insight-dev" | ||
| ), # RULE-DEFAULTS-OK: same tagged default as keycloak_realm.DEV_PASSWORD |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Check credential literals before JSON serialization.
A password that contains " or \ is escaped by json.dumps. The serialized manifest then does not contain the raw value from _FORBIDDEN_LITERALS, so assert_no_credentials can allow a recoverable password into the file and public sentinel log.
Walk manifest keys and string values before serialization, and compare those logical values with the forbidden literals.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/tools/seed/insight_seed/manifest.py` around lines 93 - 98,
Update the manifest validation around assert_no_credentials in the seed manifest
generation flow to inspect manifest keys and string values before json.dumps
serialization. Compare these logical values directly against
_FORBIDDEN_LITERALS, while preserving existing validation of serialized output
and ensuring forbidden passwords cannot reach the manifest file or sentinel log.
Comment on lines
+58
to
+59
| earliest = end - timedelta(days=max_days) | ||
| return max(start, earliest).isoformat(), end.isoformat() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject reversed data windows.
A value such as 2026-08-12..2026-08-11 passes the current checks and returns an invalid period. Assert start <= end before calculating earliest.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/stand/api/analytics/__init__.py` around lines 58 - 59, In the
date-window calculation containing earliest and the returned ISO-formatted
bounds, validate that start is less than or equal to end before calculating
earliest. Reject reversed windows such as a start date after the end date while
preserving the existing max_days clamping for valid ranges.
added 6 commits
August 12, 2026 14:58
…nes) Same seven-step procedure and the load-bearing warnings, minus the explanatory asides. Corrected the seed window from the stale --days 730 (the value INFRA.md incident 3 records as the bug) to 365, matching CI. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
lib.sh gets a shellcheck shell directive (SC2148) and a scoped SC2034 disable for the colour vars its sourcers use; the kubeconfig-help echo becomes printf (SC2028); PROFILE.md's seed_revision is regenerated to match this branch's own edits to the seed package (was stale, wrong-but- green since nothing runs render_profile --check). Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…d rebuild gold A freshly seeded stand served 200-with-null for every person metric: the persons-seed CronJob was disabled here, so connector accounts were never linked to the seeded roster person and gold's resolve_person_id() had no value_type='id' bindings to join to. Enable persons-seed and have seed-stand.sh force the persons-seed + persons-sync projection and a gold rebuild after an all/silver seed — the k8s analogue of dev-compose.sh's cmd_seed. persons-seed LINKS each connector account to the roster person by email (resolve_assignments' LinkedByEmail) and appends to persons, so the roster UUID the login resolves to is the UUID gold attributes activity to. Refactor the single-Job tail of seed-stand.sh into render_seed_manifest / wait_for_job / run_seed_step, and raise the deploy workflow's seed-step and job timeouts to cover the extra Jobs. test_metric_results_200 stays xfail until a stand run confirms non-null. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…fore merge Adds debug-test-stand.yml (push trigger on this branch, resolves the newest published chart, calls the reusable deploy workflow), a main-only-guard exception for this branch's push, and a manifest fallback (the published seed image predates this PR's SEED_MANIFEST_JSON sentinel). Validates the #2447 seeding fix end to end. Revert this commit before merge. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
ktursunov
temporarily deployed
to
insight-test-stand
August 12, 2026 13:11 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
src/ingestion/tools/seed/seed-stand.sh (1)
558-567: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one helper for the name-export-render sequence.
Three sites repeat the same three steps: build a timestamped Job name, export
SEED_STEPandSEED_JOB_NAME, then render. A drift in one site is easy to miss, for example a change to the name format.♻️ Optional consolidation
+# stage_seed_step STEP — set the per-step Job identity for render_seed_manifest. +stage_seed_step() { + export SEED_STEP="$1" + export SEED_JOB_NAME="insight-seed-${1}-$(date -u +%Y%m%d%H%M%S)" +} + # run_seed_step STEP — apply one seeder Job for STEP and wait for its verdict. run_seed_step() { - local step="$1" job_name - # A name per run: a Job's pod spec is immutable. - job_name="insight-seed-${step}-$(date -u +%Y%m%d%H%M%S)" - export SEED_STEP="$step" - export SEED_JOB_NAME="$job_name" + local step="$1" + # A name per run: a Job's pod spec is immutable. + stage_seed_step "$step" render_seed_manifest | kube apply -f -Then call
stage_seed_step "$STEP"in the--dry-runand--no-followbranches.Also applies to: 603-609, 614-620
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ingestion/tools/seed/seed-stand.sh` around lines 558 - 567, Introduce a shared stage_seed_step helper for generating the timestamped job name, exporting SEED_STEP and SEED_JOB_NAME, and rendering the manifest; replace the repeated sequences in run_seed_step and the --dry-run and --no-follow branches with stage_seed_step "$STEP", while preserving each branch’s existing follow/apply behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/debug-test-stand.yml:
- Around line 1-12: Delete the temporary debug-test-stand workflow entirely,
including its name and push trigger, so changes to test-stand-ci cannot start
the duplicate deployment path.
In `@deploy/gitops/environments/test-stand/README.md`:
- Around line 255-260: Update the realm-generation configuration documented in
the README so internet-reachable stands require an externally supplied per-stand
persona password, reject the committed default insight-dev, and pass the
validated value to all seeded users and CI. Apply the same change to the
corresponding guidance in the second referenced section, preserving the existing
environment-variable names.
- Around line 103-105: Update the deployment command around the OIDC
client-secret extraction to enable pipeline failure detection, capture the
decoded value before invoking Helm, and explicitly reject an empty or unreadable
secret. Only proceed with the Helm command when the validated secret is
non-empty, preserving the existing authenticator.oidc.clientSecret setting.
- Around line 99-105: Update the documented Helm deployment flow around the helm
upgrade --install command to run the downgrade/version guard before mutating the
release. Use the guarded make deploy path or add the existing equivalent version
preflight ahead of the command, ensuring lower chart versions are rejected
before Helm executes.
- Around line 312-313: Update the test-stand configuration referenced by the
README and values.yaml so identity CronJobs cannot run during seed-to-smoke
validation; disable or suspend those CronJobs for this stand while preserving
the existing smoke checks.
- Around line 308-311: Update the authenticator.overrideEnabled setting in
values.yaml for the public stand to false, and add coverage verifying requests
using __override are rejected while the existing real password login smoke flow
remains successful.
- Around line 239-245: Update _user() documentation in profile_md.py and the
related get_login_id_pairs documentation to distinguish the roster UUID from
Keycloak’s generated identifier: describe it as the identity.persons ID and
idp_sub value, not the Keycloak id/sub. Preserve the authenticator
externalIdClaim setting as idp_sub and clarify that keycloak-config-cli assigns
the actual Keycloak id/sub independently.
In `@src/ingestion/tools/seed/PROFILE.md`:
- Line 20: Regenerate the generated profile for seed revision b398406c7cb4d91e
using the required --days 365 seed window, ensuring its recorded date range and
derived values match the corresponding manifest.json output; do not leave the
approximately 60-day range unless explicitly marking the profile as a fixed
canonical fixture.
In `@src/ingestion/tools/seed/seed-stand.sh`:
- Around line 509-516: Bound the `kube -n "$NAMESPACE" logs -f "job/$job_name"`
call in `wait_for_job` so it cannot block longer than `DEADLINE_SECONDS`. Run
the follow operation through a small exported wrapper or background it and
terminate it after the deadline, ensuring the existing deadline calculation and
subsequent job-status handling still execute.
- Around line 592-594: Update the job_name construction in the seed script to
explicitly truncate the generated CronJob-derived name so the complete name,
including the -ci- suffix and 14-digit timestamp, never exceeds Kubernetes’
63-character limit. Preserve the existing naming format and uniqueness while
applying the bound before invoking kube create job.
---
Nitpick comments:
In `@src/ingestion/tools/seed/seed-stand.sh`:
- Around line 558-567: Introduce a shared stage_seed_step helper for generating
the timestamped job name, exporting SEED_STEP and SEED_JOB_NAME, and rendering
the manifest; replace the repeated sequences in run_seed_step and the --dry-run
and --no-follow branches with stage_seed_step "$STEP", while preserving each
branch’s existing follow/apply behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 15a49bb1-9a61-4af0-920f-fc71e2b5c0a8
📒 Files selected for processing (11)
.github/workflows/debug-test-stand.yml.github/workflows/deploy-test-stand.ymldeploy/gitops/environments/test-stand/INFRA.mddeploy/gitops/environments/test-stand/README.mddeploy/gitops/environments/test-stand/ci-deployer-rbac.yamldeploy/gitops/environments/test-stand/values.yamldeploy/gitops/scripts/lib.shsrc/ingestion/tools/seed/PROFILE.mdsrc/ingestion/tools/seed/seed-stand.shtests/pyproject.tomltests/stand/api/analytics/test_results.py
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/pyproject.toml
- deploy/gitops/scripts/lib.sh
- tests/stand/api/analytics/test_results.py
- deploy/gitops/environments/test-stand/ci-deployer-rbac.yaml
- deploy/gitops/environments/test-stand/values.yaml
- deploy/gitops/environments/test-stand/INFRA.md
- .github/workflows/deploy-test-stand.yml
Comment on lines
+99
to
+105
| helm upgrade --install insight \ | ||
| oci://ghcr.io/constructorfabric/charts/insight \ | ||
| --version "<chart-version>" --namespace insight \ | ||
| --values deploy/gitops/environments/test-stand/values.yaml \ | ||
| --set-string authenticator.oidc.clientSecret="$(kubectl -n insight \ | ||
| get secret insight-oidc -o jsonpath='{.data.client-secret}' | base64 --decode)" \ | ||
| --wait --timeout 10m --history-max 10 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Run the downgrade guard before helm upgrade.
helm upgrade --install --version "<chart-version>" accepts a lower version. The release check later in this section runs after the mutation. A stale or resumed run can perform the downgrade that this README says wedged subsequent upgrades. Call the guarded make deploy path, or add the same version preflight before this command.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/environments/test-stand/README.md` around lines 99 - 105,
Update the documented Helm deployment flow around the helm upgrade --install
command to run the downgrade/version guard before mutating the release. Use the
guarded make deploy path or add the existing equivalent version preflight ahead
of the command, ensuring lower chart versions are rejected before Helm executes.
Comment on lines
+103
to
+105
| --set-string authenticator.oidc.clientSecret="$(kubectl -n insight \ | ||
| get secret insight-oidc -o jsonpath='{.data.client-secret}' | base64 --decode)" \ | ||
| --wait --timeout 10m --history-max 10 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail closed when the OIDC secret cannot be read.
The command substitution can return an empty value when kubectl cannot find insight-oidc or client-secret. Without pipefail and a non-empty check, base64 can succeed and Helm can deploy a blank client secret. Extract and validate the secret before invoking Helm.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/environments/test-stand/README.md` around lines 103 - 105,
Update the deployment command around the OIDC client-secret extraction to enable
pipeline failure detection, capture the decoded value before invoking Helm, and
explicitly reject an empty or unreadable secret. Only proceed with the Helm
command when the validated secret is non-empty, preserving the existing
authenticator.oidc.clientSecret setting.
Comment on lines
+239
to
+245
| - **The external id is `idp_sub`, not `sub`.** keycloak-config-cli creates users | ||
| via the admin REST API, where Keycloak assigns its own `sub` and discards the | ||
| document's; the bring-up copies the roster UUID into an `idp_sub` attribute + | ||
| mapper, and `authenticator.oidc.externalIdClaim` names it. Point it at `sub` | ||
| and every login authenticates then is denied against a full projection — the | ||
| most expensive failure, because nothing looks like a config error. Full | ||
| contract: [INFRA.md](INFRA.md#the-load-bearing-settings) §11–12. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(PROFILE\.md|README\.md|.*realm.*|.*keycloak.*|.*generator.*)' | sed -n '1,200p'
printf '%s\n' '--- identifier references ---'
rg -n -C 3 'idp_sub|roster UUID|Keycloak user ID|externalIdClaim|internal ID|user.*id|userId' \
deploy/gitops/environments/test-stand/README.md \
src/ingestion/tools/seed/PROFILE.md \
deploy 2>/dev/null | sed -n '1,260p'
printf '%s\n' '--- relevant file sizes ---'
wc -l deploy/gitops/environments/test-stand/README.md \
src/ingestion/tools/seed/PROFILE.md 2>/dev/null || trueRepository: constructorfabric/insight
Length of output: 37120
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- README context ---'
sed -n '220,260p' deploy/gitops/environments/test-stand/README.md
printf '%s\n' '--- PROFILE context ---'
sed -n '1,220p' src/ingestion/tools/seed/PROFILE.md
printf '%s\n' '--- generator and seed implementation references ---'
rg -n -C 4 'idp_sub|externalIdClaim|keycloak|user.?id|roster.*uuid|uuid' \
src deploy 2>/dev/null | sed -n '1,360p'Repository: constructorfabric/insight
Length of output: 40684
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
paths = [
Path("deploy/gitops/environments/test-stand/README.md"),
Path("src/ingestion/tools/seed/PROFILE.md"),
]
needles = ("idp_sub", "externalIdClaim", "Keycloak user ID", "roster UUID", "user id", "user_id")
for path in paths:
print(f"--- {path} ---")
if not path.exists():
print("MISSING")
continue
lines = path.read_text(errors="replace").splitlines()
hits = [i for i, line in enumerate(lines) if any(n.lower() in line.lower() for n in needles)]
shown = set()
for i in hits:
for j in range(max(0, i - 5), min(len(lines), i + 6)):
if j not in shown:
print(f"{j+1}: {lines[j]}")
shown.add(j)
print()
print("--- repository references ---")
for path in Path(".").rglob("*"):
if not path.is_file() or ".git" in path.parts:
continue
try:
text = path.read_text(errors="replace")
except (OSError, UnicodeError):
continue
if any(n.lower() in text.lower() for n in needles):
print(path)
PYRepository: constructorfabric/insight
Length of output: 8979
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- seed profile ---'
cat -n src/ingestion/tools/seed/PROFILE.md
printf '%s\n' '--- seed-related files ---'
git ls-files src/ingestion/tools/seed deploy/gitops/environments/test-stand | sed -n '1,240p'
printf '%s\n' '--- identity import code and configuration ---'
rg -n -C 5 'UserRepresentation|keycloak-config-cli|idp_sub|externalIdClaim|INSIGHT_TENANT_ID|roster' \
src/ingestion/tools/seed deploy/gitops/environments/test-stand deploy/gitops/environments 2>/dev/null \
| sed -n '1,500p'Repository: constructorfabric/insight
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- realm generator ---'
rg -n -C 12 'def _user|["'\"'"]id["'\"'"]|uuid|users' \
src/ingestion/tools/seed/insight_seed/keycloak_realm.py \
src/ingestion/tools/seed/insight_seed/profiles.py \
| sed -n '1,320p'
printf '%s\n' '--- test-stand identity contract ---'
sed -n '600,648p' deploy/gitops/environments/test-stand/values.yaml
printf '%s\n' '--- post-processing and config-cli inputs ---'
rg -n -C 8 'idp_sub|user.attribute|keycloakConfig|config-cli|realm.*json|\\.users|externalIdClaim' \
deploy/gitops/environments/test-stand \
charts/insight/templates/keycloak-config-job.yaml \
src/ingestion/tools/seed/seed-stand.sh \
| sed -n '1,420p'
printf '%s\n' '--- profile construction ---'
rg -n -C 10 'Keycloak user id|person id|uuid|build_manifest|profile' \
src/ingestion/tools/seed/insight_seed/manifest.py \
src/ingestion/tools/seed/insight_seed/profile_md.py \
src/ingestion/tools/seed/insight_seed/profiles.py \
| sed -n '1,320p'Repository: constructorfabric/insight
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact realm user construction ---'
sed -n '1,260p' src/ingestion/tools/seed/insight_seed/keycloak_realm.py
printf '%s\n' '--- exact identity login bindings ---'
sed -n '175,245p' src/ingestion/tools/seed/insight_seed/identity.py
rg -n -C 8 'def get_login_id_pairs|external_id|person_uuid' \
src/ingestion/tools/seed/insight_seed/profiles.py \
src/ingestion/tools/seed/insight_seed/identity.py
printf '%s\n' '--- all test-stand transformations that mention user IDs ---'
rg -n -C 6 'idp_sub|\\.users|user\["id"\]|user\.id|realm-insight|config-cli|attribute-mapper' \
deploy/gitops/environments/test-stand charts/insight src/ingestion/tools/seed/seed-stand.sh \
| sed -n '1,360p'Repository: constructorfabric/insight
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- generator identity fields ---'
rg -n -C 10 'def _user|["'\"'"]id["'\"'"]|uuid|users' \
src/ingestion/tools/seed/insight_seed/keycloak_realm.py \
| sed -n '1,220p'
printf '%s\n' '--- profile text source ---'
rg -n -C 10 'Keycloak user id|identity.persons|same person|Roster' \
src/ingestion/tools/seed/insight_seed/profile_md.py \
src/ingestion/tools/seed/insight_seed/manifest.py \
src/ingestion/tools/seed/insight_seed/profiles.py \
| sed -n '1,220p'
printf '%s\n' '--- config-cli deployment path ---'
rg -n -C 10 'keycloakConfig|config-cli|realm-insight|idp_sub|externalIdClaim|post-process|postprocess' \
deploy/gitops/environments/test-stand charts/insight src/ingestion/tools/seed/seed-stand.sh \
| sed -n '1,320p'Repository: constructorfabric/insight
Length of output: 40008
Clarify the generated profile’s Keycloak identifier. _user() sets the roster UUID as the realm JSON id, but test-stand keycloak-config-cli creates a different Keycloak id/sub. Update profile_md.py and the related get_login_id_pairs documentation to describe the roster UUID as the identity.persons ID and idp_sub. Retain externalIdClaim: idp_sub.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/environments/test-stand/README.md` around lines 239 - 245,
Update _user() documentation in profile_md.py and the related get_login_id_pairs
documentation to distinguish the roster UUID from Keycloak’s generated
identifier: describe it as the identity.persons ID and idp_sub value, not the
Keycloak id/sub. Preserve the authenticator externalIdClaim setting as idp_sub
and clarify that keycloak-config-cli assigns the actual Keycloak id/sub
independently.
Comment on lines
+255
to
+260
| - **The persona password is one shared value.** The realm generator applies | ||
| `INSIGHT_SEED_PERSONA_PASSWORD` (default: the committed `insight-dev`, for | ||
| stands unreachable from outside) to every user, so | ||
| `INSIGHT_STAND_PERSONA_PASSWORD` / `TEST_STAND_PERSONA_PASSWORD` is one value | ||
| per stand. An internet-reachable stand must set it at realm-generation time | ||
| and hand the same value to CI. See Known gaps. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Replace the published persona password.
Every seeded user shares the committed default insight-dev. The public roster exposes the persona addresses and roles, so anyone can authenticate as a seeded persona, including privileged personas. Set a per-stand password through the external environment and reject the default during realm generation.
Also applies to: 295-299
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/environments/test-stand/README.md` around lines 255 - 260,
Update the realm-generation configuration documented in the README so
internet-reachable stands require an externally supplied per-stand persona
password, reject the committed default insight-dev, and pass the validated value
to all seeded users and CI. Apply the same change to the corresponding guidance
in the second referenced section, preserving the existing environment-variable
names.
Comment on lines
+308
to
+311
| - **`authenticator.overrideEnabled: true`** is carried forward — a standing | ||
| impersonation primitive on an internet-reachable stand. The smoke suite uses | ||
| real password login and never sends `__override`, so turning it off is now an | ||
| unblocked change; tracked in the `values.yaml` comment. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable authenticator.overrideEnabled on the public stand.
This setting leaves a request-level impersonation path enabled. The smoke suite uses real password login and does not require __override. Set it to false in values.yaml and add a check that override requests fail.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/environments/test-stand/README.md` around lines 308 - 311,
Update the authenticator.overrideEnabled setting in values.yaml for the public
stand to false, and add coverage verifying requests using __override are
rejected while the existing real password login smoke flow remains successful.
Comment on lines
+312
to
+313
| - **The identity CronJobs** can un-seed logins once their input table stops | ||
| being empty — see the comment at the bottom of `values.yaml`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Block identity CronJobs during seed-to-smoke validation.
If these CronJobs run after seeding, they can remove the identity projection before the login and /auth/me checks. Disable or suspend them for this stand, or gate smoke execution on a stable projection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/environments/test-stand/README.md` around lines 312 - 313,
Update the test-stand configuration referenced by the README and values.yaml so
identity CronJobs cannot run during seed-to-smoke validation; disable or suspend
those CronJobs for this stand while preserving the existing smoke checks.
| | anchor_date | `2026-06-30` | | ||
| | data_window | `2026-05-02..2026-06-30` | | ||
| | seed_revision | `07365a1509ac7db6` | | ||
| | seed_revision | `b398406c7cb4d91e` | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Regenerate the profile for the 365-day seed window.
The stand instructions now require --days 365, but this profile still records 2026-05-02..2026-06-30, which is approximately 60 days. Because this file is generated from the same builder as manifest.json, regenerate it for seed revision b398406c7cb4d91e, or label it explicitly as a fixed canonical fixture.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/tools/seed/PROFILE.md` at line 20, Regenerate the generated
profile for seed revision b398406c7cb4d91e using the required --days 365 seed
window, ensuring its recorded date range and derived values match the
corresponding manifest.json output; do not leave the approximately 60-day range
unless explicitly marking the profile as a fixed canonical fixture.
Comment on lines
+509
to
+516
| for _ in $(seq 1 60); do | ||
| phase="$(kube -n "$NAMESPACE" get pod -l "job-name=$job_name" \ | ||
| -o 'jsonpath={.items[0].status.phase}' 2>/dev/null || true)" | ||
| case "$phase" in Running|Succeeded|Failed) break ;; esac | ||
| sleep 2 | ||
| done | ||
|
|
||
| kube -n "$NAMESPACE" logs -f "job/$job_name" || true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the log-follow call; the deadline clock starts only after it returns.
wait_for_job calls kube logs -f "job/$job_name" at Line 516 and computes deadline at Line 520. A pod that starts and then runs without ending keeps logs -f attached, so the polling deadline never starts and the script blocks past DEADLINE_SECONDS. The seeder Jobs get SEED_DEADLINE_SECONDS from the template, but the Jobs that project_identity creates from the chart CronJobs do not, so their wall clock depends on the chart spec.
Wrap the follow call with the same budget.
🛡️ Proposed fix to bound the follow call
- kube -n "$NAMESPACE" logs -f "job/$job_name" || true
+ # Bounded: a Job that never ends must not hold the stream open past the
+ # script's own ceiling. The verdict is read from the Job object below.
+ timeout "${DEADLINE_SECONDS}s" \
+ "${BASH_SOURCE%/*}/../../../../bin/true" 2>/dev/null || true
+ timeout "${DEADLINE_SECONDS}s" bash -c \
+ 'kube_ns=$1; job=$2; shift 2; kubectl logs -f "job/$job" -n "$kube_ns"' \
+ _ "$NAMESPACE" "$job_name" || trueThe diff above is illustrative only, because kube() is a shell function and timeout needs an external command. Prefer exporting the follow call through a small wrapper, or run kube ... logs -f ... & and kill the background PID after DEADLINE_SECONDS.
Also applies to: 520-521
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/tools/seed/seed-stand.sh` around lines 509 - 516, Bound the
`kube -n "$NAMESPACE" logs -f "job/$job_name"` call in `wait_for_job` so it
cannot block longer than `DEADLINE_SECONDS`. Run the follow operation through a
small exported wrapper or background it and terminate it after the deadline,
ensuring the existing deadline calculation and subsequent job-status handling
still execute.
Comment on lines
+592
to
+594
| job_name="${cronjob}-ci-$(date -u +%Y%m%d%H%M%S)" | ||
| echo "==> identity projection: $component (job/$job_name from cronjob/$cronjob)" | ||
| kube -n "$NAMESPACE" create job --from="cronjob/$cronjob" "$job_name" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Truncate the forced identity Job name to stay inside the 63-character limit.
job_name concatenates the discovered CronJob name, -ci-, and a 14-digit timestamp. The CronJob name already carries the release prefix and the subchart name. For a release named after the namespace, for example insight-test-stand, the CronJob name is close to insight-test-stand-identity-resolution-persons-seed (51 characters); adding -ci- plus the timestamp produces 69 characters. Kubernetes rejects the create because the generated job-name label value cannot exceed 63 characters, so the identity projection fails on long release names instead of running.
Bound the name explicitly.
🐛 Proposed fix to bound the Job name
- job_name="${cronjob}-ci-$(date -u +%Y%m%d%H%M%S)"
+ # 63-char ceiling: the pod's `job-name` label carries this value verbatim.
+ job_name="${cronjob:0:45}-ci-$(date -u +%Y%m%d%H%M%S)"
+ job_name="${job_name#-}"Run the following script to check the rendered CronJob names on this chart:
#!/bin/bash
# Description: Report the identity CronJob metadata names the chart renders, with lengths.
set -euo pipefail
fd -t f -g '*cronjob*.yaml' src/backend/services/identity-resolution/helm/templates \
--exec sh -c 'echo "== $1"; rg -n -A4 "^metadata:" "$1"' sh {}
# Fullname helpers decide the prefix length; read them too.
fd -t f -g '_helpers.tpl' src/backend/services/identity-resolution/helm --exec cat -n {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/tools/seed/seed-stand.sh` around lines 592 - 594, Update the
job_name construction in the seed script to explicitly truncate the generated
CronJob-derived name so the complete name, including the -ci- suffix and
14-digit timestamp, never exceeds Kubernetes’ 63-character limit. Preserve the
existing naming format and uniqueness while applying the bound before invoking
kube create job.
added 4 commits
August 12, 2026 22:49
… stand The value-level guard xpassed on a real stand deploy (chart 0.5.128): the seeded dev_lead's canonical UUID now gets a non-null metric number, confirming the persons-seed + persons-sync projection and gold rebuild resolve the roster person. Drop the xfail so it is a live guard again. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…elete before merge" This reverts commit d99f7ee. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech> # Conflicts: # src/ingestion/tools/seed/PROFILE.md # tests/pyproject.toml
…delete before merge" This reverts commit 7a2d13a. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
ktursunov
temporarily deployed
to
insight-test-stand
August 12, 2026 15:21 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
…I run — delete before merge"" This reverts commit 1b37604. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
ktursunov
force-pushed
the
test-stand-ci
branch
from
August 12, 2026 15:54
2517a1c to
e6b7262
Compare
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
mozhaev-dev
approved these changes
Aug 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2244.
Merge-to-
mainvalidation stops at "images built, chart published". This adds the next step: install that exact chart on the shared test stand, seed it, and prove a person can sign in and see data.What this is
An after-merge alarm, not a gate, and it cannot be anything else: the artefact under test does not exist until
publish-charthas run, and that runs on push tomain. Merge blocking stays with the merge-queue lanes; this guards the claim that what those lanes approved actually installs and works. A red run meansmainis broken — the merge author owns it, fixes forward if the cause is small, reverts if not.Shape
build-images.ymlgains anoutputs:block onpublish-chartand one calling job handingchart_versionto a reusabledeploy-test-stand.yml,mainonly. Named stages with their own timeouts, so a red run says which one died without opening a log:make -C deploy/gitops deploy(the same target an operator runs) at that version,--wait, no--atomic; a downgrade guard refuses installing an older chart than what is deployedsrc/ingestion/tools/seed/seed-stand.sh, 365-day window (the analytics API refuses ≥400 days); anall/silverrun then forces the identity projection (persons-seed + persons-sync) and a gold rebuild, so the stand serves resolved metrics rather than a null for every personpytest tests/stand -m stand_smoke— the deployed-stand suite's own login//auth/me/metric checks, not a separate packageSmoke never runs after a failed seed. Deploys coalesce, so a running upgrade is never killed mid-flight. Chart version is resolved from the OCI registry's own latest tag (or
publish-chart's output on theworkflow_callpath), falling back to.insight-versionon the default branch via the GitHub API only if the registry is unreachable — never from a local checkout, which is as old as its branch.What has actually been proven
Not "implemented" — run, against the real stand:
deploy/gitops/scripts/recreate-test-stand.sh: drops the application databases, reinstalls throughmake deploy, verifies.--forceon a freshly wiped tenant./auth/meand the session-identity checks pass, now consolidated intotests/stand/apiunder thestand_smokemarker instead of a paralleltests/stand/smokepackage.POST /v1/metric-resultsfor a seeded person's canonical UUID returns a non-null value, proven bytest_metric_results_200understand_smoke. This closes All person metrics return null on a freshly seeded install #2447: the seed had left the stand with the persons-seed CronJob disabled, so connector accounts were never linked to the seeded person and gold served null for every person metric. The seed now forces the projection (persons-seed links each connector account to the seeded person by e-mail, so the UUID a login resolves to is the UUID gold attributes activity to) and rebuilds gold over it.kubectl rollout restartof three hardcoded deployments is gone. This stand's config Secrets are static by policy; a deliberate config-values change requires a manual rollout restart of the three readers — documented as an accepted gap inenvironments/test-stand/INFRA.md§25.Script and workflow inventory (post-restructure)
deploy/gitops/scripts/recreate-test-stand.sh— everymake deployprecondition (version resolution, sync-clean, kube-ctx, chart availability) now runs before the first destructive action, not between the wipe and the reinstall.deploy/gitops/scripts/lib.sh(new) — the shared guard/helper library (cluster_guard,namespace_assert,oidc_secret_nonempty, colour/hdr/note/die)recreate-test-stand.shsources, replacing four-plus divergent inline copies.deploy/gitops/scripts/provision-ci-deployer.sh— now a 50-line CLI wrapper around three new Makefile targets (provision-ci,verify-ci-credential,revoke-ci); the RBAC objects it used to render from a heredoc are a committed, static manifest atdeploy/gitops/environments/test-stand/ci-deployer-rbac.yaml..github/workflows/scripts/extract-seed-manifest.py— deleted. The seeder emits oneSEED_MANIFEST_JSON:sentinel line (insight_seed/manifest.py), captured withgrep/cutinstead of a parser..github/workflows/scripts/redact-stand-log.py— the long-blob rule now also masks base64url (Keycloak client secrets, PKCE verifiers), and the final verification pass re-checks it.Design notes for a reviewer
make deployis the deploy path. CI runs the official target, not a second helm procedure. Deploy shape is declared inenvironments/test-stand/inventory.yamlandvalues.yaml.No
--atomic, and the stand is disposable. A rollback reinstalls the previous chart onto a stand whose job is to show what the newest one does. Recovery is fix-forward orrecreate-test-stand.sh, never a downgrade — a stuck-release guard (Makefile) and a downgrade guard (workflow) both hold this; seeINFRA.md, "Incidents that shaped this environment", for the failure modes these close.No cluster diagnostics are published. A red run prints which stage died and the black-box edge result; helm history and pod logs are read operator-side.
redact-stand-log.pyfilters everything else that reaches the console.Least-privilege credential.
make provision-ciapplies a committed, namespace-scoped RBAC manifest and asserts its scope with akubectl auth can-imatrix (make verify-ci-credential) before any secret is handed to CI.Incident log. Six failure modes this environment's guards close are documented once, in
deploy/gitops/environments/test-stand/INFRA.md("Incidents that shaped this environment"), rather than as narrative comments scattered across the workflow and scripts.Known follow-up (not this PR)
seed-stand.shstill discovers the dev-lead's realm-config-map name by sed-scrapinginsight_seed/profiles.py'sDEV_LEAD_UUIDconstant and, whenkeycloakConfig.realmsConfigMapis unset, guessing the chart's Helm-fullname convention. Both are read-only fallbacks, not a correctness bug, but they couple this script to another module's internals. Follow-up: declare the realm ConfigMap's name (or the dev-lead's address directly) as avalues.yamlkey so the realm and the seeder read one committed value instead of one script inferring the other's naming convention.envFromreaders (env vars are read once at container start, and the pod spec doesn't change). Accepted for now: this stand's config values are pinned and rotations are rare manual events; the manual-restart procedure is inenvironments/test-stand/INFRA.md§25. Permanent fix when it matters: hash the Secret's data into each subchart'schecksum/configfrom the same values it is templated from.Setup — confirmed by the deploy → seed → smoke runs
insight-test-standenvironment exists with its secrets set (TEST_STAND_KUBECONFIG,TEST_STAND_PERSONA_PASSWORD) — the runs authenticate with the namespace-scopedci-deployercredential and log the seeded personas in. Runbook:deploy/gitops/environments/test-stand/credentials-runbook.mdmake -C deploy/gitops provision-ci ENV=test-stand(or theprovision-ci-deployer.shwrapper) minted the credential; it self-verifies withverify-ci-credential.main(it currently also allows this branch, which was needed for pre-merge validation).#2463, merged tomain, closed the related drilldown coverage gaps). This workflow is an after-merge alarm, so a rare timeout will eventually page somebody about a healthy stand.Summary by CodeRabbit