OLS-3205: Single UI container image for all OCP versions#2029
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR builds three UI variants into a single container image, tracks pf5 and 4.19 sources as submodules, updates Tekton to clone/prefetch those submodules, switches the Docker runtime to select a variant via OCP_VERSION in entrypoint.sh, and updates docs and test diagnostics. ChangesMulti-Build Deployment Model
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.tekton/integration-tests/lightspeed-console-pre-commit.yaml (1)
61-61: WIP: temporary retarget to 4.19 — restore before merge.Pinning both the provisioned cluster version (
4.19.) andOCP_VERSION=4.19exercises only the/builds/4-19variant. Per the PR description this is intentional for WIP, but it leaves the defaultmainbuild path untested in CI. Please restore (or parameterize across variants) before this leaves WIP.Want me to open a tracking issue so this revert isn't forgotten before merge?
Also applies to: 236-237
🤖 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 @.tekton/integration-tests/lightspeed-console-pre-commit.yaml at line 61, The pipeline temporarily pins the cluster/OCP version to "4.19." which forces the CI to only exercise the /builds/4-19 variant; restore the original version (or make it a parameter) so the default main build path is tested: update the value: "4.19." entries and the OCP_VERSION setting referenced in this file (and the related lines ~236-237) back to the previous/default version or replace them with a pipeline parameter/variable that cycles across variants so CI covers the main build path instead of only /builds/4-19.
🤖 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 @.ai/spec/how/project-structure.md:
- Around line 122-132: Update the "CI/CD pipeline (`.tekton/`)" section to
document the pre-commit Tekton pipeline by adding an entry for
`.tekton/integration-tests/lightspeed-console-pre-commit.yaml`, explaining its
purpose (pre-commit checks) and noting that it also uses `git-clone-oci-ta` with
`SUBMODULES: "true"` and participates in the same Cachi2 `prefetch-input`
scheme; reference the existing push/PR entries and ensure the table now includes
the pre-commit pipeline so the `.tekton/` coverage is complete.
In @.tekton/lightspeed-console-pull-request.yaml:
- Around line 157-158: The SUBMODULES param is incorrectly cased for the
git-clone-oci-ta:0.1 task so submodules may not be fetched; change the pipeline
param name from SUBMODULES to submodules (keep the value 'true') in the task
params block that invokes git-clone-oci-ta:0.1 so the operator recognizes it and
optional submodulePaths will be honored during the subsequent COPY/npm prefetch
steps.
In `@tests/support/global-setup.ts`:
- Around line 269-276: The oc rollout wait is being killed by the global 180s
helper timeout in tests/support/fixtures.ts; update the oc invocation in
tests/support/global-setup.ts (the oc([... 'rollout','status',
`deployment/${deploymentName}`, ...]) call) to use a longer per-call timeout
instead of the default helper timeout—either extend the oc helper to accept an
explicit timeout argument or add a new helper (e.g., ocWithLongTimeout /
ocRolloutStatus) and call that for rollout status so the command's
'--timeout=5m' can complete without being terminated by the 180s process timeout
enforcement in tests/support/fixtures.ts.
---
Nitpick comments:
In @.tekton/integration-tests/lightspeed-console-pre-commit.yaml:
- Line 61: The pipeline temporarily pins the cluster/OCP version to "4.19."
which forces the CI to only exercise the /builds/4-19 variant; restore the
original version (or make it a parameter) so the default main build path is
tested: update the value: "4.19." entries and the OCP_VERSION setting referenced
in this file (and the related lines ~236-237) back to the previous/default
version or replace them with a pipeline parameter/variable that cycles across
variants so CI covers the main build path instead of only /builds/4-19.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8aa55cdd-dd43-40f1-9a4f-b817bb268c6c
📒 Files selected for processing (11)
.ai/spec/how/project-structure.md.ai/spec/what/system-overview.md.gitmodules.tekton/integration-tests/lightspeed-console-pre-commit.yaml.tekton/lightspeed-console-pull-request.yaml.tekton/lightspeed-console-push.yamlDockerfilebranches/4-19branches/pf5entrypoint.shtests/support/global-setup.ts
kyoto
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@kyoto: This pull request references OLS-3206 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
kyoto
changed the title
|
@kyoto: This pull request references OLS-3205 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/support/global-setup.ts`:
- Around line 248-267: The wait loop currently ignores its timeout and may fall
through to oc(['set','env',...]) causing an opaque "deployment not found" error;
update the loop in global-setup.ts to track success with a boolean (e.g.,
foundDeployment) while waiting for deploymentName in namespace OLS_NAMESPACE
using the oc(...) call, and after the loop check that flag—if false, throw or
log a clear error (including deploymentName, OLS_NAMESPACE and the timeout
duration) or skip the env set to avoid the opaque failure; ensure references to
deploymentName, OLS_NAMESPACE and the oc invocation are used so the change is
applied to the correct block.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 63aea6aa-ad98-4df2-82e7-49f57b2f43d0
📒 Files selected for processing (11)
.ai/spec/how/project-structure.md.ai/spec/what/system-overview.md.gitmodules.tekton/integration-tests/lightspeed-console-pre-commit.yaml.tekton/lightspeed-console-pull-request.yaml.tekton/lightspeed-console-push.yamlDockerfilebranches/4-19branches/pf5entrypoint.shtests/support/global-setup.ts
✅ Files skipped from review due to trivial changes (3)
- branches/4-19
- .tekton/integration-tests/lightspeed-console-pre-commit.yaml
- .ai/spec/how/project-structure.md
🚧 Files skipped from review as they are similar to previous changes (7)
- .tekton/lightspeed-console-push.yaml
- .tekton/lightspeed-console-pull-request.yaml
- branches/pf5
- .gitmodules
- entrypoint.sh
- .ai/spec/what/system-overview.md
- Dockerfile
kyoto
force-pushed
the
single-ui-image
branch
6 times, most recently
from
8c5feca to
2e6f941
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kyoto
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| *) BUILD_DIR=/builds/main ;; | ||
| esac | ||
|
|
||
| ln -sfn "$BUILD_DIR" /tmp/nginx/html |
There was a problem hiding this comment.
🟡 Suggestion: Add defensive mkdir -p for /tmp/nginx
The entrypoint relies on /tmp/nginx existing from the Dockerfile's build-time mkdir -p. If a container runtime mounts a tmpfs at /tmp, that directory is wiped and ln -sfn fails immediately (with set -eu the container exits with a cryptic error).
A one-line addition makes the entrypoint self-contained:
| ln -sfn "$BUILD_DIR" /tmp/nginx/html | |
| mkdir -p /tmp/nginx | |
| ln -sfn "$BUILD_DIR" /tmp/nginx/html | |
| exec nginx -g 'daemon off;' -e stderr |
| esac | ||
|
|
||
| ln -sfn "$BUILD_DIR" /tmp/nginx/html | ||
| exec nginx -g 'daemon off;' -e stderr |
There was a problem hiding this comment.
🟡 Suggestion: Log the selected build variant at startup
The entrypoint silently picks a variant with no output. When debugging deployment issues (wrong UI served, missing env var, operator misconfiguration), there's no visibility into what happened. A single echo before the exec would show up in container logs:
| exec nginx -g 'daemon off;' -e stderr | |
| ln -sfn "$BUILD_DIR" /tmp/nginx/html | |
| echo "OCP_VERSION=${OCP_VERSION:-unset} -- serving $BUILD_DIR" | |
| exec nginx -g 'daemon off;' -e stderr |
| 4.16|4.17|4.18) BUILD_DIR=/builds/pf5 ;; | ||
| 4.19|4.20|4.21) BUILD_DIR=/builds/4-19 ;; | ||
| *) BUILD_DIR=/builds/main ;; | ||
| esac |
There was a problem hiding this comment.
🟡 Suggestion: Consider prefix matching for OCP_VERSION
The case requires exact major.minor matches. If the operator ever sets OCP_VERSION to a patch version (4.19.3), prefixed form (v4.19), or pre-release (4.19-rc1), the match silently falls through to main, serving the wrong variant.
Prefix matching would be more resilient:
| esac | |
| 4.16*|4.17*|4.18*) BUILD_DIR=/builds/pf5 ;; | |
| 4.19*|4.20*|4.21*) BUILD_DIR=/builds/4-19 ;; |
If exact matching is intentional (strict contract with the operator), a comment documenting that assumption would help future maintainers.
| | `.tekton/lightspeed-console-pull-request.yaml` | Konflux/Tekton pipeline for pull request events | | ||
| | `.tekton/integration-tests/lightspeed-console-pre-commit.yaml` | Integration test pipeline: provisions an ephemeral cluster, installs the operator, runs lint/unit tests and Playwright e2e tests | | ||
|
|
||
| The push and pull-request pipelines use `git-clone-oci-ta` with `submodules: |
There was a problem hiding this comment.
🟢 Nit: Test directories table references Cypress, but the project uses Playwright
The "Test directories" table at the bottom of this file still lists tests/ as "Cypress e2e test specs" and includes cypress/ and cypress.config.ts, but the project moved to Playwright — there's no Cypress config or directory anywhere in the repo. Since this spec file is being updated in this PR anyway, it would be a good opportunity to fix it:
tests/→ Playwright e2e test specs- Remove the
cypress/andcypress.config.tsrows - Add
playwright.config.tsinstead
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/lgtm |
kyoto
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
3 participants
Summary by CodeRabbit
Documentation
Chores
Tests