feat(release): add managed source releases - #237
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT. This is a carefully built, well-tested source-release and build-identity change: pinned actions, minimal token scopes, env-wrapped untrusted input, additive tests, and policy checks that all pass locally (make check, release_policy.py, check_image_build_policy.py, the Python unit tests, build-version.sh). No blockers or security regressions found; the main thing that needs a decision is a repo-wide PR-title policy that collides with the team's existing [HYPERSHELL-XXX] titling habit.
What I verified
make checkgates (check-release-policy,check-image-build-policy) and the new Python unit tests pass in this tree.scripts/build-version.sh local|ciproducedev-58b1e9a/v0.0.0-58b1e9aas designed, and degrade todev-unknownwithout a SHA.- Release-file consistency (
VERSION=.release-please-manifest.json.=build-version.envBUILD_PREFIXminusv) is internally consistent (0.0.0/v0.0.0), andbootstrap-shapoints at the currentmaintip. - Web-console changes are additive; the tracing "fail-closed to sampleRatio 0" guarantee is preserved, and no secret crosses the BFF->browser boundary (the
browserRuntimeConfigkey-set test still enforces this). - The
/metadataendpoint is intentionally unauthenticated (security: []+ auth-bypass) but only exposes build version and build time -- no secret exposure. - Modified test assertions are all additive contract widenings (new
buildfield), not silent guarantee removals.
Findings
[Major] New required PR-title gate conflicts with the repo's [HYPERSHELL-XXX] titling convention.
REL-01 + scripts/release_policy.py require every PR title to start with <type>(<scope>)!: <desc>, and the title becomes the squash subject that Release Please parses. Nearly all recent merged commits and open PRs use a [HYPERSHELL-XXX] ... prefix, which this regex rejects. See the Cross-PR section -- this needs a maintainer decision before/at merge.
[Minor] scripts/check_image_build_policy.py couples the policy to an exact Dockerfile substring.
The check matches a literal ${VCS_REF%????...} (exactly 33 ?) and full label strings. A harmless Dockerfile reformat (whitespace, quoting) will fail the gate with a message that doesn't point at the real cause. Consider matching on the semantic pieces (org.opencontainers.image.revision, HYPERSHELL_BUILD_VERSION) rather than the exact expansion literal.
[Minor / informational] Extended ${VAR%pattern} expansion in ENV/LABEL relies on the BuildKit Dockerfile frontend.
It works because each Dockerfile declares # syntax=docker/dockerfile:1, but it silently no-ops under a legacy builder. Fine as-is given the syntax pin; just noting the dependency.
Cross-PR coordination
Merging this PR turns the Conventional-Commit PR-title check (lint-pull-request-title, backed by scripts/release_policy.py check-title and REL-01) into a required gate whose regex requires the title to begin with a lowercase <type>. The prevailing repository practice prefixes titles with a Jira key, e.g. [HYPERSHELL-XXX] feat: ..., which this regex rejects; because the title also becomes the squash-merge subject, such titles would additionally break Release Please's changelog/version parsing on main. Maintainers must decide the convention -- either drop the bracketed prefix going forward, or widen the regex to permit an issue-key affix -- and then coordinate retitling of the in-flight PRs that currently use the prefix so they can pass the new required check once this merges: #235, #232, #227, #210, and #208. #214 and #109 are also affected because their titles are not in conventional form at all.
Findings Summary (ordered by severity, highest first):
- [Major] New required PR-title gate conflicts with the established
[HYPERSHELL-XXX]prefix used across history and open PRs - Cross-PR / Process (release_policy.py L32) - [Minor] Image build policy check pinned to exact Dockerfile substrings is brittle - Maintainability (check_image_build_policy.py L56)
- [Minor]
${VAR%pattern}inENV/LABELdepends on the BuildKit frontend - Portability (components/*/Dockerfile)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (title, VERSION, SHA, build version) | Pass |
| SecurityContext (no pod specs changed) | N/A |
| OpenAPI client regenerated, not hand-edited | Pass |
| Conventional commit messages | Pass |
| Test diff scrutiny (no silent guarantee removal) | Pass |
| Image references consistent across manifests | Pass |
Amber reviewStatus: Stopped The pull request head changed before Amber posted the review. A later job can review the new head. |
|
Amber review is addressed.
Release Please now uses the built-in |
Amber reviewStatus: Stopped The pull request head changed before Amber posted the review. A later job can review the new head. |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This is a well-structured, well-tested release-automation change: Release Please wiring, a conventional PR-title gate, build-identity embedding across images, a public /metadata endpoint, and a web-console version display, all with matching Python/Go/TS tests that pass locally. The code is convention-compliant and I found no blocking defects; my notes are minor, plus one cross-PR coordination item that needs a maintainer decision because this PR introduces a new required PR-title check that conflicts with the title format several open PRs currently use.
I verified make-level policy checks locally: release_policy.py check-files, check_image_build_policy.py, and the three Python unittest suites all pass. The build-identity ldflags target (rh-trex-ai/pkg/api.Version/BuildTime) is consistent across the Makefiles and Dockerfiles, the removed local api.Version/api.BuildTime vars have no remaining references, and the SDK generator correctly excludes the x-sdk-exclude singleton (with a guard test).
Findings
Minor — Security (info disclosure): openapi.yaml:23,91
The /metadata endpoint is made unauthenticated (security: [], added to auth-bypass-paths) and returns version and build_time. Exposing an exact build version and build timestamp to anonymous callers makes version fingerprinting (matching a deployment to known CVEs) easier. This is intentional and the values are not secrets, so it's minor — but consider whether the precise build_time needs to be anonymous, or whether the version string alone suffices for the console/health use case. Confidence: Medium.
Minor — Design: components/web-console/app/features/shell/user-menu.tsx:26
readBrowserRuntimeConfig() is invoked during render, which re-reads the injected meta tag and JSON.parses it on every render of UserMenu. It's cheap and correct, but reading the runtime config once (e.g. a memoized/composition-provided value) would keep the parse off the render path and match the "read runtime config at composition boundary" intent. Confidence: Medium.
Test Diff Scrutiny
The modified assertions in browser-runtime-config.test.ts, bff/test/config.test.ts, and bff/test/app.test.ts are additive: they add the new build field to expected objects while preserving the existing tracing guarantees (invalid ratio still falls back to 0, invalid build version is ignored without disturbing tracing). No pre-existing guarantee was flipped from accepted→rejected, and the optional→required change (HYPERSHELL_BUILD_VERSION validation) is on a brand-new optional field, so no backfill/migration is required.
Cross-PR coordination
This PR adds lint-pull-request-title as a required lint gate and codifies a specific convention (docs/releasing.md, AGENTS.md, CLAUDE.md, scripts/release_policy.py): the PR title must start with a Conventional Commits type and any Jira key must appear after the colon. Because the PR title becomes the squash commit subject that Release Please parses, this is a plan/interface change, not just a lint preference.
Several open pull requests use the opposite title form (Jira key before the type, e.g. [HYPERSHELL-NNN] feat: ..., or no conventional type at all). I confirmed the following titles fail the new checker and would be blocked by the required gate once this merges, until retitled: #235, #232, #227, #210, #208, #214, #109.
Maintainers should decide the rollout/merge order and notify those authors to retitle: either merge #237 and require the affected PRs to update their titles, or coordinate the change so in-flight work isn't blocked unexpectedly. This is a decision about sequencing and communication, not a defect in this PR.
Findings Summary (ordered by severity, highest first)
- [Minor] Unauthenticated
/metadataexposesversion+build_time(version fingerprinting) - Security (openapi.yaml L23, L91) - [Minor]
readBrowserRuntimeConfig()parsed on everyUserMenurender - Design (user-menu.tsx L26)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (build version, VCS ref, PR title) | Pass |
| Image references consistent across manifests | Pass |
| OpenAPI client not manually edited | Pass |
| Conventional commit messages | Pass |
| Test diff scrutiny (no flipped guarantees) | Pass |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT. This is a well-structured, spec-backed source-release feature (Release Please + build identity + API/console version surfacing) with thorough tests and self-enforcing policy checks. I found no blockers or security-critical defects; the notes below are minor, and there are cross-PR coordination items that maintainers should sequence before merge.
What I verified
release_policy.py check-files,check_image_build_policy.py, andrelease_policy.py check-titleon the PR's own title all pass in the PR tree.- Removing
Version/BuildTimefromcomponents/api-server/pkg/api/api.goand retargeting the ldflags torh-trex-ai/pkg/apiis safe: no production code references the old symbols, and the metadata handler/route is supplied by the framework viaMetadataID: "hypershell". - The generated OpenAPI client (
ServiceMetadata,api_default.go, docs) is consistent with the source spec (spec SHA updated), i.e. regenerated rather than hand-edited. - Modified tests are additive (they add the
buildfield / metadata assertions); no pre-existing guarantee was flipped or removed.
Minor findings
-
Public, unauthenticated
/api/hypershell/v1/metadata. The endpoint is nowsecurity: []and added toauth-bypass-pathsin both thedevelopment_oidcenv and the kind/openshift overlays. That exposesversion(release prefix + short Git SHA) andbuild_timeto anonymous callers. This is a reasonable design for a version banner, but it is a deliberate information-disclosure decision (build identity can help correlate a deployment to known CVEs). Please confirm it is intended; if so, consider limiting the response toversiononly. -
Local
devbuild version drops the dirty-tree indicator. The previous Makefile logic appended-modifiedwhen the working tree was dirty;scripts/build-version.sh localnow emits onlydev-<shortsha>. During iterativeswap-component/kind debugging, an image built from uncommitted changes now reports a clean-looking version, which can mislead. Consider re-adding a dirty marker for local mode. -
Obscure revision truncation in the Dockerfiles.
${VCS_REF%?????????????????????????????????}(33?) silently depends onVCS_REFbeing a full 40-char SHA; a non-40-char value (e.g.unknown) passes through unchanged. It is validated bycheck_image_build_policy.py, but a short comment explaining the "strip 33 to keep 7" intent would help future readers.
Cross-PR coordination
The following require maintainer sequencing or a design decision before merge:
-
#150 redesigns the same
scripts/kind/build-images.shbuild invocation, keeping the old--build-arg GIT_VERSIONand the${BUILD_DIR}/components/api-serverbuild context. This PR changes the api-server build context to the repository root and rewritescomponents/api-server/DockerfiletoCOPY components/api-server/go.mod .... The two designs are incompatible: whichever merges second must adopt this PR's repo-root context andBUILD_PREFIX/VCS_REFbuild args, or the api-server image will fail to build / lose its build identity. Decide a merge order and reconcile the build command. -
#217 switches the OpenShift api-server to
API_ENV=development_oidcindeploy/openshift/kustomization.yaml, while this PR edits the same overlay's--auth-bypass-pathspatch (and thedevelopment_oidcenv's ownFlags()) to add/api/hypershell/v1/metadata. That creates two sources of truth for the OpenShift auth-bypass paths. Maintainers should decide which mechanism owns the bypass list and ensure the/metadatapath survives so the console's unauthenticated version fetch works under whichever environment #217 selects. -
#232 introduces a
scripts/cluster/driver abstraction (its ownup.sh/swap.sh) over the kind scripts and separately restructuresdeploy/openshift/kustomization.yamland the.github/workflows/e2e.ymlchange-detection block that this PR also modifies. Its OpenShift/registry build path must propagate this PR's newBUILD_PREFIX/VCS_REFbuild args and repo-root Docker build context (and the shared e2e detection / openshift-overlay edits must be merged coherently), or images built through that driver will not build or will lack build identity. Coordinate ownership of the build/swap tooling and the shared overlay/e2e edits.
Findings Summary (ordered by severity, highest first)
- [Minor] Unauthenticated
/metadataexposes build version/build_time to anonymous callers - Security / API design (openapi.yaml L23, e_development_oidc.go, deploy overlays) - [Minor] Local
devbuild version no longer marks a dirty working tree - Observability / Dev UX (scripts/build-version.sh L16-17) - [Minor] Obscure
${VCS_REF%???...}truncation depends on a full 40-char SHA - Readability (components/*/Dockerfile)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
| Errors wrapped / propagated (Go + Python policy scripts) | Pass |
| No secrets in logs or responses | Pass |
| Input validated (SemVer / SHA / build-version regex) | Pass |
| Image references consistent across manifests | Pass |
| OpenAPI client regenerated, not hand-edited | Pass |
| Conventional commit / PR title | Pass |
| Test Diff Scrutiny (no flipped assertions) | Pass |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This PR adds a well-structured Release Please source-release process, a build-identity pipeline across the three images, and a public API metadata endpoint surfaced in the web console. The implementation is clean and test-backed; my findings are minor plus one cross-PR policy decision that maintainers should settle before merge.
Confidence: High on the structural review, Medium on a couple of the minor robustness notes.
Findings
[Minor] Public, unauthenticated metadata endpoint exposes build identity - Security
components/api-server/openapi/openapi.yaml:23 marks GET /metadata security: [], and the endpoint is added to the auth-bypass lists (e_development_oidc.go, deploy/kind + deploy/openshift overlays). The response now includes the image build version (which embeds the short git SHA) and is reachable by anonymous callers. This is a common and generally acceptable pattern (health/openapi are already public, and a build version is not a secret), but note the web console only reads it after authentication, so the anonymous exposure is broader than the consumer needs. Confirm this is intended; if so, no change required.
[Minor] ldflags inject version into a third-party module package - Maintainability
components/api-server/Makefile:18 and components/api-server/Dockerfile:30 now -X into github.com/openshift-online/rh-trex-ai/pkg/api.Version/.BuildTime rather than a HyperShell-owned symbol (the local pkg/api.Version/BuildTime vars were removed in api.go). This works and is covered by metadata_test.go, but it couples the build to an upstream symbol name; if rh-trex-ai renames or drops those vars, the -X silently no-ops and the binary ships an empty version with no build failure. Consider a build-time assertion or keeping the metadata test in a spot that runs in CI to catch upstream drift.
[Minor] Build-version regex duplicated across hand-maintained files - Config vs. Code
The same version pattern lives in components/web-console/bff/src/config.ts:39 and components/web-console/app/composition/browser-runtime-config.ts:25 (and a sibling form in scripts/build-version.sh). A future change to the versioning scheme (e.g., a new prefix) must be edited in several disconnected places or validation silently diverges. Consider centralizing the pattern.
[Minor] Obscure VCS_REF shortening - Readability/Robustness
components/api-server/Dockerfile:42 (and the control-plane/web-console Dockerfiles) shorten the SHA with ${VCS_REF%?????????????????????????????????} (33 ?). It is correct for a 40-char SHA and check_image_build_policy.py guards the count, but if VCS_REF is not exactly 40 hex chars (e.g., the unknown default) the label/ENV silently keep the full value. The comment helps; a brief note that this relies on a validated 40-char ref would make the intent clearer.
Notes
- Test diffs are additive: the modified
browser-runtime-config.test.ts,bff/test/config.test.ts, andopenapi_embed_test.gohunks extend expectations to the newbuild/ServiceMetadatashape rather than flipping any prior guarantee. No removed contracts. - The api-server Dockerfile build-context change (
components/api-server-> repo root) is consistent with the matching Tektonpath-context: .update and the other two components.
Cross-PR coordination
This PR enforces a new pull-request-title contract: scripts/release_policy.py check-title (wired into .github/workflows/lint.yml as a required gate and documented in CLAUDE.md/AGENTS.md) requires the Conventional Commits type at the start of the title and mandates that any Jira key appear after the colon (feat(release): [HYPERSHELL-123] ...), explicitly forbidding a leading [HYPERSHELL-nnn]. Multiple in-flight PRs use the opposite, currently prevailing ordering (leading [HYPERSHELL-nnn], sometimes with no type: at all) - for example #235, #227, #232, and #214. Once this PR merges, those titles will fail the new lint gate and cannot merge until retitled. Maintainers need to decide which title format is canonical for the repo and, if this PR's rule stands, coordinate retitling of the affected open PRs (and align the historical [HYPERSHELL-nnn]-first convention). This is a shared-policy decision, not a file conflict.
Findings Summary (ordered by severity, highest first):
- [Minor] Public unauthenticated metadata endpoint exposes build identity to anonymous callers - Security (openapi.yaml L23)
- [Minor] Version ldflags inject into third-party
rh-trex-ai/pkg/apisymbol; upstream drift fails silently - Maintainability (Makefile L18, Dockerfile L30) - [Minor] Build-version regex duplicated across hand-maintained files - Config vs. Code (config.ts L39, browser-runtime-config.ts L25)
- [Minor] Obscure 33-
?VCS_REF shortening relies on an unvalidated 40-char ref in the Docker layer - Readability (Dockerfile L42)
Convention Checklist:
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass (N/A - no new Go error paths) |
| No secrets in logs or responses | Pass |
| Input validated | Pass |
| Image references consistent across the stack | Pass |
| OpenAPI client not manually edited | Pass (regenerated; SDK spec SHA updated) |
| Test diff scrutiny (no silently removed guarantees) | Pass |
| Conventional commit messages | Pass |
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
COMMENT - This is a well-structured, thoroughly tested change that introduces Release Please source releases, deterministic image build identities, and surfaces the API/console versions in the UI. I found no blockers; the notes below are minor, plus one cross-PR coordination item that maintainers should sequence before merge.
What I reviewed
- Build identity flow:
scripts/build-version.sh, the threeDockerfiles, rootMakefile,components/api-server/Makefile, and the Kind build/swap scripts. - Release policy tooling:
scripts/release_policy.py,scripts/check_image_build_policy.py, their unit tests,release-please-config.json,.release-please-manifest.json,VERSION,build-version.env, and the new/updated workflows. - API surface:
openapi.yamlServiceMetadataschema + embedded-spec test, generated OpenAPI model,pkg/api/api.go(removal of localVersion/BuildTime), andx-sdk-excludehandling inscripts/sdk-generator/parser.go. - Web console:
api-versionadapter/hook,browser-runtime-config, BFFconfig.ts,user-menu.tsx, i18n messages, and their tests.
I re-ran the shipped Python policy suites and both policy checks locally: release_policy.py check-files, check_image_build_policy.py, and python3 -m unittest for the three test modules all pass. I confirmed no remaining references to the removed hypershell .../pkg/api.Version/.BuildTime symbols, so the ldflags retarget to rh-trex-ai/pkg/api does not break compilation.
Test Diff Scrutiny
The modified assertions in pre-existing tests (browser-runtime-config.test.ts, bff/test/config.test.ts, user-menu.test.tsx) are additive schema-shape updates (build: {} added alongside existing tracing). The fail-closed tracing guarantee is preserved (invalid/missing config still yields sampleRatio: 0), and new negative cases (invalid build version rejected, API-unavailable falls back to "unknown") were added rather than flipping an existing guarantee. No removed guarantees detected.
Findings
[Minor] Unauthenticated metadata endpoint discloses build identity - Security / Info disclosure
/api/hypershell/v1/metadata gains security: [] (components/api-server/openapi/openapi.yaml:23) and is added to auth-bypass-paths in both the dev OIDC env and the Kind/OpenShift kustomizations. This lets any anonymous caller read the API build version (short commit) and build_time. That is a deliberate choice so the BFF can proxy same-origin without a token, and the exposure (short SHA + timestamp) is low risk, but the console only reads it while authenticated - so please confirm the endpoint is intended to be reachable without auth rather than proxied with the caller's session. No secret values are exposed. Confidence: Medium.
[Minor] Build version string is a soft contract across many layers - Maintainability
The build-version grammar (dev-<7hex>[-modified] / v<semver>-<7hex>) is independently re-encoded as a regex in bff/src/config.ts and browser-runtime-config.ts, as a ${VCS_REF%...} (33-char trim) expansion in three Dockerfiles, as SHORT_REVISION_TRIM=33 in check_image_build_policy.py, and in build-version.sh. The image-build-policy check guards the Dockerfiles, but the TS regexes are separate copies. Consider a short comment cross-referencing the canonical definition so a future format change updates every copy. Confidence: High.
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass (n/a - minimal new Go) |
| No secrets in logs or responses | Pass |
| Input validated (VCS_REF / build version) | Pass |
| OpenAPI client not manually edited | Pass (generated model + x-sdk-exclude) |
| Image references consistent across the stack | Pass |
| Conventional commit / PR title | Pass |
| Config separate from code | Pass |
Findings Summary (ordered by severity, highest first)
- [Minor] Unauthenticated
/metadatadiscloses build identity - Security (openapi.yaml L23) - [Minor] Build-version grammar duplicated across layers - Maintainability
Cross-PR coordination
Two other open pull requests rewrite the same local image-build path with a build-arg contract that is incompatible with this PR, so maintainers must decide a merge order and reconcile the interface:
- #150 rewrites
scripts/kind/build-images.sh(andMakefile) to build from the working tree, but keeps the old--build-arg GIT_VERSION=...contract and thecomponents/api-serverbuild context. This PR removesGIT_VERSION, requires a valid 40-characterVCS_REFbuild-arg (the Dockerfile builder now hard-fails without it), and moves the api-server build context to the repository root. Whichever merges second must adopt this PR'sVCS_REF/BUILD_PREFIX/BUILD_SUFFIXbuild args and the new context, or local image builds break. - #232 introduces a new
scripts/cluster/framework whosecomponent_spec()/push_component_image()builds all three images using the old contract (GIT_VERSIONandBUILD_CONTEXT="components/api-server"for api-server; no build args for control-plane and web-console). Under this PR's Dockerfiles those builds fail becauseVCS_REFis required and the api-server context changed. The new OpenShift/cluster build path needs the same build-arg treatment this PR adds to the Kind scripts; maintainers should sequence the two and align on one build-identity interface.
Amber reviewStatus: Complete |
jsell-rh
left a comment
jsell-rh
left a comment
There was a problem hiding this comment.
Verdict
This is a large but disciplined, spec-driven change (specs/platform/source-release.spec.md) that adds Release Please source releases, deterministic image build identity, and API/console version surfacing. The code is well-tested and internally consistent, with no blocking or critical defects; the main action items are cross-PR merge coordination and a small maintainability note.
I reviewed against CLAUDE.md, the security spec, and the control-plane conventions. The unauthenticated /metadata endpoint (security: [], added to auth-bypass-paths) is intentional and explicitly mandated by REL-09, and it exposes only non-secret build identity — not a finding. Test-diff scrutiny: the modified assertions in browser-runtime-config.test.ts, bff/test/config.test.ts, and bff/test/app.test.ts are additive (they thread the new build field through existing expectations) and preserve the prior fail-closed tracing guarantees; no contract was silently removed.
Highlights
- Build identity is validated at multiple layers:
VCS_REFis required and checked as a 40-char lowercase SHA in all three Dockerfiles,build-version.shenforces the format, andcheck-image-build-policy/check-release-policyare wired intomake check. - Removing the local
api.Version/api.BuildTimevars in favor of the rh-trex-ai globals is clean, andmetadata_test.goplus thelinked_metadata-tagged test prove the ldflags actually land in the binary. - SDK exclusion via
x-sdk-excludeis correctly enforced in the generator and covered byTestParseSpecExcludesSingletonMetadata.
Minor
- The REL-07 build-version regex is duplicated verbatim in
components/web-console/bff/src/config.tsandcomponents/web-console/app/composition/browser-runtime-config.ts. This is defensible given the BFF/browser boundary, but the two copies must be kept in lockstep with the spec if the format ever changes. Consider a shared constant or a comment cross-referencing the sibling copy. (Confidence: High)
Cross-PR coordination
- #232 — This PR replaces the
GIT_VERSIONbuild-arg contract with required, validatedVCS_REF/BUILD_PREFIX/BUILD_SUFFIXargs across all three Dockerfiles and addscheck-image-build-policytomake check. #232 adds a new component build path that still passes--build-arg GIT_VERSION=.... Whichever merges second will break: #232's image builds will fail against the new Dockerfiles, and its build call sites must be updated to the new build-arg contract. Both PRs also editdeploy/openshift/kustomization.yaml's api-server env/auth block. Maintainers need to decide merge order and update the trailing PR to the new build-arg contract. - #150 — #150 independently rewrites
scripts/kind/build-images.shand the Makefile build variables (the same build path this PR restructures), keeping the oldbuild_version/GIT_VERSIONmodel. The two rewrites are incompatible; the trailing PR must be rebased onto the newVCS_REF/BUILD_PREFIX/BUILD_SUFFIXflow and its "working-tree by default" behavior reconciled withbuild-version.sh's dirty-state handling. A merge-order decision and integration are required. - #217 — #217 switches the OpenShift overlay to run the api-server under
API_ENV=development_oidcand replaces the whole env array. This PR depends on auth-bypass configuration (bothe_development_oidc.goand the overlay's--auth-bypass-paths) to keep the newly unauthenticated/metadataendpoint reachable. Coordinate so the metadata bypass survives the env switch and the two edits to the same overlay are reconciled.
Findings Summary (ordered by severity, highest first)
- [Minor] REL-07 build-version regex duplicated across the BFF and browser layers; keep the two copies in sync with the spec - Maintainability (bff
config.tsL40,browser-runtime-config.tsL26)
Convention Checklist
| Convention | Result |
|---|---|
No panic() in production code |
Pass |
Errors wrapped with fmt.Errorf context |
Pass |
| No secrets in logs or responses | Pass |
| Input validated (VCS_REF SHA, build-version format) | Pass |
| Unauthenticated endpoint scope (build identity only) | Pass |
| OpenAPI client not manually edited (regenerated) | Pass |
| Reconcile / update-or-create patterns | N/A |
| Image references consistent across manifests | Pass |
| Conventional commit messages | Pass |
| Test diff scrutiny (no silently removed guarantees) | Pass |
|
|
||
| // REL-07 in specs/platform/source-release.spec.md defines this format. | ||
| const buildVersionPattern = | ||
| /^(?:dev-[0-9a-f]{7}(?:-modified)?|v(?:0|[1-9][0-9]*)\.(?:0|[1-9][0-9]*)\.(?:0|[1-9][0-9]*)-[0-9a-f]{7})$/u; |
There was a problem hiding this comment.
[Minor] Maintainability — This REL-07 build-version regex is a verbatim copy of the one in components/web-console/bff/src/config.ts (L40). The duplication is defensible across the BFF/browser boundary, but both must be updated together if the build-version format in specs/platform/source-release.spec.md changes. Consider a shared constant or a cross-reference comment so the two copies do not drift.
1 participant
Summary
Validation
make checkpnpm check:webRelease PR checks
Release Please uses the built-in
GITHUB_TOKEN. No app, repository variable, or repository secret is required. GitHub holds workflows for an automation-created Release PR until a user with write access selects Approve workflows to run. Approve the workflows for the current revision before merge.