[WIP] OLS-3205: Single UI container image for all OCP versions#2029
[WIP] OLS-3205: Single UI container image for all OCP versions#2029kyoto wants to merge 7 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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
| ### CI/CD pipeline (`.tekton/`) | ||
|
|
||
| | Path | Purpose | | ||
| |---|---| | ||
| | `.tekton/lightspeed-console-push.yaml` | Konflux/Tekton pipeline for push events | | ||
| | `.tekton/lightspeed-console-pull-request.yaml` | Konflux/Tekton pipeline for pull request events | | ||
|
|
||
| Both pipelines use `git-clone-oci-ta` with `SUBMODULES: "true"` to fetch the | ||
| git submodules. The Cachi2 `prefetch-input` includes npm paths for all three | ||
| source directories (`"."`, `"branches/pf5"`, `"branches/4-19"`) plus a single | ||
| RPM path (`"."`). This enables hermetic builds with pre-fetched dependencies. |
There was a problem hiding this comment.
Include the pre-commit Tekton pipeline in the CI/CD section.
This section says “Both pipelines” and lists only push/PR, but this PR also updates .tekton/integration-tests/lightspeed-console-pre-commit.yaml. Please document it here so the .tekton/ coverage is complete.
🤖 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 @.ai/spec/how/project-structure.md around lines 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.
| - name: SUBMODULES | ||
| value: 'true' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Konflux build-definitions git-clone-oci-ta 0.1 task submodules parameter name
💡 Result:
In the Konflux git-clone-oci-ta:0.1 task, the parameter name to specify specific submodule paths to initialize and fetch is submodulePaths [1][2]. To use this parameter, the main submodules parameter must be set to true [1][2]. The submodulePaths parameter accepts a comma-separated list of specific paths; if left as an empty string, all submodules are fetched [1][2].
Citations:
- 1: https://github.com/konflux-ci/build-definitions/blob/main/pipelines/docker-build-oci-ta/README.md
- 2: https://github.com/konflux-ci/build-definitions/blob/main/pipelines/fbc-builder/README.md
Fix SUBMODULES parameter casing for git-clone-oci-ta:0.1.
Konflux git-clone-oci-ta:0.1 declares the parameter as submodules (optionally submodulePaths), so - name: SUBMODULES won’t match and may cause submodule fetching to be skipped. Update the param at .tekton/lightspeed-console-pull-request.yaml (lines 157-158) to use submodules so the later COPY branches/... / npm prefetch for those paths works.
- name: SUBMODULES
value: 'true'🤖 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/lightspeed-console-pull-request.yaml around lines 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.
| oc([ | ||
| 'rollout', | ||
| 'status', | ||
| `deployment/${deploymentName}`, | ||
| '-n', | ||
| OLS_NAMESPACE, | ||
| '--timeout=5m', | ||
| ]); |
There was a problem hiding this comment.
Rollout wait can time out 2 minutes early due to oc helper timeout.
At Line 269, this uses oc(...), but tests/support/fixtures.ts enforces a 180s process timeout. That can kill oc rollout status --timeout=5m before rollout completes, causing flaky setup failures.
Proposed fix
- oc([
- 'rollout',
- 'status',
- `deployment/${deploymentName}`,
- '-n',
- OLS_NAMESPACE,
- '--timeout=5m',
- ]);
+ execFileSync(
+ 'oc',
+ [
+ 'rollout',
+ 'status',
+ `deployment/${deploymentName}`,
+ '-n',
+ OLS_NAMESPACE,
+ '--timeout=5m',
+ '--kubeconfig',
+ KUBECONFIG!,
+ ],
+ { encoding: 'utf-8', timeout: 6 * MINUTE },
+ );🧰 Tools
🪛 ESLint
[error] 269-276: Replace ⏎······'rollout',⏎······'status',⏎······deployment/${deploymentName},⏎······'-n',⏎······OLS_NAMESPACE,⏎······'--timeout=5m',⏎···· with 'rollout',·'status',·deployment/${deploymentName},·'-n',·OLS_NAMESPACE,·'--timeout=5m'
(prettier/prettier)
🤖 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/support/global-setup.ts` around lines 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.
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. |
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>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
| console.log(`Waiting for deployment/${deploymentName} to exist...`); | ||
| const deadline = Date.now() + 5 * MINUTE; | ||
| while (Date.now() < deadline) { | ||
| try { | ||
| oc(['get', `deployment/${deploymentName}`, '-n', OLS_NAMESPACE]); | ||
| break; | ||
| } catch { | ||
| console.log('Waiting for console plugin deployment...'); | ||
| execSync('sleep 10'); | ||
| } | ||
| } | ||
| console.log(`Setting OCP_VERSION=${process.env.OCP_VERSION} on ${deploymentName}...`); | ||
| oc([ | ||
| 'set', | ||
| 'env', | ||
| `deployment/${deploymentName}`, | ||
| '-n', | ||
| OLS_NAMESPACE, | ||
| `OCP_VERSION=${process.env.OCP_VERSION}`, | ||
| ]); |
There was a problem hiding this comment.
Wait loop ignores its own timeout, then fails opaquely.
If the deployment never appears within the 5-minute window, the loop exits silently and execution falls through to oc set env, which throws an unclear "deployment not found" error. Track success and fail (or skip) explicitly so the timeout is meaningful and the failure is diagnosable.
Proposed fix
console.log(`Waiting for deployment/${deploymentName} to exist...`);
const deadline = Date.now() + 5 * MINUTE;
+ let deploymentFound = false;
while (Date.now() < deadline) {
try {
oc(['get', `deployment/${deploymentName}`, '-n', OLS_NAMESPACE]);
+ deploymentFound = true;
break;
} catch {
console.log('Waiting for console plugin deployment...');
execSync('sleep 10');
}
}
+ if (!deploymentFound) {
+ throw new Error(
+ `Timed out waiting for deployment/${deploymentName} in ${OLS_NAMESPACE}`,
+ );
+ }
console.log(`Setting OCP_VERSION=${process.env.OCP_VERSION} on ${deploymentName}...`);📝 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.
| console.log(`Waiting for deployment/${deploymentName} to exist...`); | |
| const deadline = Date.now() + 5 * MINUTE; | |
| while (Date.now() < deadline) { | |
| try { | |
| oc(['get', `deployment/${deploymentName}`, '-n', OLS_NAMESPACE]); | |
| break; | |
| } catch { | |
| console.log('Waiting for console plugin deployment...'); | |
| execSync('sleep 10'); | |
| } | |
| } | |
| console.log(`Setting OCP_VERSION=${process.env.OCP_VERSION} on ${deploymentName}...`); | |
| oc([ | |
| 'set', | |
| 'env', | |
| `deployment/${deploymentName}`, | |
| '-n', | |
| OLS_NAMESPACE, | |
| `OCP_VERSION=${process.env.OCP_VERSION}`, | |
| ]); | |
| console.log(`Waiting for deployment/${deploymentName} to exist...`); | |
| const deadline = Date.now() + 5 * MINUTE; | |
| let deploymentFound = false; | |
| while (Date.now() < deadline) { | |
| try { | |
| oc(['get', `deployment/${deploymentName}`, '-n', OLS_NAMESPACE]); | |
| deploymentFound = true; | |
| break; | |
| } catch { | |
| console.log('Waiting for console plugin deployment...'); | |
| execSync('sleep 10'); | |
| } | |
| } | |
| if (!deploymentFound) { | |
| throw new Error( | |
| `Timed out waiting for deployment/${deploymentName} in ${OLS_NAMESPACE}`, | |
| ); | |
| } | |
| console.log(`Setting OCP_VERSION=${process.env.OCP_VERSION} on ${deploymentName}...`); | |
| oc([ | |
| 'set', | |
| 'env', | |
| `deployment/${deploymentName}`, | |
| '-n', | |
| OLS_NAMESPACE, | |
| `OCP_VERSION=${process.env.OCP_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 `@tests/support/global-setup.ts` around lines 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.
6a6c3cf to
a43ed6e
Compare
Revert to 4.22 before merging Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary by CodeRabbit
Documentation
Chores
Tests