step-registry/telco5g-ptp-tests: refactor for readability - #82741
Conversation
|
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:
WalkthroughThe Telco5G PTP workflow centralizes release and repository settings, separates image-build logic, adds explicit execution stages, supports Go 1.25, and updates the telco runner image with a stable OpenShift client URL. ChangesTelco5G PTP test workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 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 |
jzding
changed the title
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ci-operator/step-registry/telco5g/ptp/tests/telco5g-ptp-tests-commands.sh (2)
14-58: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTighten the release-matching patterns.
The patterns
4.1[2-5]+and4.1[6-7]+apply+to a bracket expression and leave.unescaped.4.1[2-5]+therefore also matches values such as4.152. Real release strings do not hit this today, so this is cosmetic. Anchored patterns make the intent clear.♻️ Optional refactor
-if [[ "$T5CI_VERSION" =~ 4.1[2-5]+ ]]; then +if [[ "$T5CI_VERSION" =~ ^4\.1[2-5]$ ]]; then export EVENT_API_VERSION="1.0" else export EVENT_API_VERSION="2.0" fi -if [[ "$T5CI_VERSION" =~ 4.1[6-7]+ ]]; then +if [[ "$T5CI_VERSION" =~ ^4\.1[6-7]$ ]]; then🤖 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 `@ci-operator/step-registry/telco5g/ptp/tests/telco5g-ptp-tests-commands.sh` around lines 14 - 58, Update the T5CI_VERSION checks controlling EVENT_API_VERSION and ENABLE_V1_REGRESSION to use anchored regular expressions with an escaped dot and exact two-digit minor versions, replacing the current 4.1[2-5]+ and 4.1[6-7]+ patterns. Preserve the existing release ranges and flag assignments.
470-477: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
EVENT_API_VERSIONin the events patch.The configuration block exports
EVENT_API_VERSIONbased on the release, and this block repeats the same release check with a hardcoded"2.0". The two places can drift, and an override ofEVENT_API_VERSIONhas no effect here. Drive the patch from the exported value.♻️ Recommended refactor
-if [[ "$T5CI_VERSION" =~ 4.1[2-5]+ ]]; then +if [[ "${EVENT_API_VERSION}" == "1.0" ]]; then oc patch ptpoperatorconfigs.ptp.openshift.io default -nopenshift-ptp --patch '{"spec":{"ptpEventConfig":{"enableEventPublisher":true, "storageType":"emptyDir"}, "daemonNodeSelector": {"node-role.kubernetes.io/worker":""}}}' --type=merge else - oc patch ptpoperatorconfigs.ptp.openshift.io default -nopenshift-ptp --patch '{"spec":{"ptpEventConfig":{"enableEventPublisher":true, "apiVersion":"2.0"}, "daemonNodeSelector": {"node-role.kubernetes.io/worker":""}}}' --type=merge + oc patch ptpoperatorconfigs.ptp.openshift.io default -nopenshift-ptp --patch "{\"spec\":{\"ptpEventConfig\":{\"enableEventPublisher\":true, \"apiVersion\":\"${EVENT_API_VERSION}\"}, \"daemonNodeSelector\": {\"node-role.kubernetes.io/worker\":\"\"}}}" --type=merge 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 `@ci-operator/step-registry/telco5g/ptp/tests/telco5g-ptp-tests-commands.sh` around lines 470 - 477, Update the PTP events patch in the version-conditional block to use the exported EVENT_API_VERSION value for apiVersion instead of hardcoding "2.0" or repeating the release check. Preserve the existing enableEventPublisher, storageType, and daemonNodeSelector settings.
🤖 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 `@ci-operator/step-registry/telco5g/ptp/tests/telco5g-ptp-tests-commands.sh`:
- Around line 104-119: Restore errexit after the registry-login block by
changing the final shell option command following the login fallback to
re-enable both tracing and errexit, matching the options disabled by the initial
set +xe. Keep the login logic unchanged so later build_images operations such as
git clone, make docker-build, and podman push terminate the script on failure.
- Around line 68-83: Replace the pattern-based removals in the T5CI_VERSION
branching with explicit TEST_MODES array definitions for each affected release
range. Ensure 4.12 excludes dualnicbc, 4.13–4.15 exclude dualnicbcha, and 4.19
excludes tbc, without leaving empty array elements; preserve the existing modes
for all other versions.
---
Nitpick comments:
In `@ci-operator/step-registry/telco5g/ptp/tests/telco5g-ptp-tests-commands.sh`:
- Around line 14-58: Update the T5CI_VERSION checks controlling
EVENT_API_VERSION and ENABLE_V1_REGRESSION to use anchored regular expressions
with an escaped dot and exact two-digit minor versions, replacing the current
4.1[2-5]+ and 4.1[6-7]+ patterns. Preserve the existing release ranges and flag
assignments.
- Around line 470-477: Update the PTP events patch in the version-conditional
block to use the exported EVENT_API_VERSION value for apiVersion instead of
hardcoding "2.0" or repeating the release check. Preserve the existing
enableEventPublisher, storageType, and daemonNodeSelector settings.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b2d9a3bb-040c-4d89-a0ad-15ce5009aa16
📒 Files selected for processing (1)
ci-operator/step-registry/telco5g/ptp/tests/telco5g-ptp-tests-commands.sh
|
@jzding, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
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
`@ci-operator/step-registry/telco5g/ptp-cluster-setup/telco5g-ptp-cluster-setup-commands.sh`:
- Around line 9-10: Update the initialization near SKIP_OCP_INSTALL so it
defaults to true only when the variable is unset, preserving any explicit
caller-provided value such as "false". Keep the existing step contract default
consistent with the intended default behavior.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a01d4a25-66db-45ce-a39b-3327a3ad8387
📒 Files selected for processing (2)
ci-operator/step-registry/telco5g/ptp-cluster-setup/telco5g-ptp-cluster-setup-commands.shci-operator/step-registry/telco5g/ptp/tests/telco5g-ptp-tests-commands.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- ci-operator/step-registry/telco5g/ptp/tests/telco5g-ptp-tests-commands.sh
|
/pj-rehearse periodic-ci-openshift-release-main-nightly-5.0-e2e-telco5g-ptp |
|
@jzding: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-release-main-nightly-5.0-e2e-telco5g-ptp |
|
@jzding: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-ci
Bot
removed
the
approved
jzding
changed the title
|
@jzding, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
5c347c8 to
0dd5802
Compare
|
@jzding, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@jzding, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@jzding, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci-operator/step-registry/telco5g/ptp/tests/Dockerfile.telco-runner (1)
39-46: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd a non-root
USERbeforeWORKDIR.This image has no
USERdirective, so containers run as root by default. Trivy confirms this (Image user should not be 'root').run-in-container.sh(this same PR) launches this image directly with local/remotepodman run, without an OpenShift SCC to reassign the UID, so the root default is a real, live effect here, not just theoretical.The
fix_uid.shscript andchmod -R g+rwxcalls already prepare the image for OpenShift's arbitrary-UID model. Add an explicit non-rootUSERanyway: OpenShift's restricted SCC still overrides the UID (while keeping GID 0), so this does not break that model, and it gives a safe default for directpodman/docker runusage.As per path instructions,
**/{Dockerfile,Containerfile}*requires "USER non-root; never run as root."🔒 Proposed fix
oc version --client +USER 1001 WORKDIR /output🤖 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 `@ci-operator/step-registry/telco5g/ptp/tests/Dockerfile.telco-runner` around lines 39 - 46, Add an explicit non-root USER directive in the Dockerfile before WORKDIR, using the image’s existing arbitrary-UID/OpenShift compatibility setup rather than root. Preserve the current installation and permission behavior while ensuring direct podman or docker execution does not default to root.Sources: Path instructions, Linters/SAST tools
🤖 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 `@ci-operator/step-registry/telco5g/ptp/tests/Dockerfile.telco-runner`:
- Around line 21-43: Update the Go archive installation loop to download the
official Go checksum file and verify each go${version}.linux-amd64.tar.gz with
sha256sum before tar extraction, failing on mismatches. Update the OpenShift
client installation block to download the stable directory’s sha256sum.txt and
validate openshift-client-linux.tar.gz before unpacking; retain cleanup and
existing installation behavior after successful verification.
In `@ci-operator/step-registry/telco5g/ptp/tests/run-in-container.sh`:
- Around line 92-96: Replace the predictable PID-based REMOTE_BUILD_DIR
assignment in the USE_REMOTE branch with a securely created temporary directory
using mktemp -d on the remote host, and capture the resulting path for
subsequent scp and ssh commands. Apply the same change to the corresponding
remote build block also referenced by the comment, preserving cleanup after the
container build.
In `@ci-operator/step-registry/telco5g/ptp/tests/telco5g-ptp-tests-commands.sh`:
- Around line 474-478: Update the event-patch conditional to branch on the
already computed EVENT_API_VERSION instead of re-matching T5CI_VERSION. Preserve
the existing patch payloads: use storageType emptyDir for the 1.0 event API
configuration and apiVersion 2.0 for the alternate configuration, while keeping
the daemonNodeSelector unchanged.
- Around line 122-176: Avoid embedding repository URLs and branch names directly
in the generated script. Pass PTP_REPO, PTP_UNDER_TEST_BRANCH, DAEMON_REPO, and
CEP_REPO through the privileged build pod environment, then update the git clone
commands in the PTP, linuxptp-daemon, and cloud-event-proxy build flow to use
quoted shell variables while preserving the existing branch selection behavior.
---
Outside diff comments:
In `@ci-operator/step-registry/telco5g/ptp/tests/Dockerfile.telco-runner`:
- Around line 39-46: Add an explicit non-root USER directive in the Dockerfile
before WORKDIR, using the image’s existing arbitrary-UID/OpenShift compatibility
setup rather than root. Preserve the current installation and permission
behavior while ensuring direct podman or docker execution does not default to
root.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 655a6879-82f3-4287-8777-62f9a3da49b5
📒 Files selected for processing (4)
ci-operator/step-registry/telco5g/ptp/tests/Dockerfile.telco-runnerci-operator/step-registry/telco5g/ptp/tests/run-in-container.shci-operator/step-registry/telco5g/ptp/tests/telco5g-ptp-tests-commands.shclusters/app.ci/supplemental-ci-images/telco-runner.yaml
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
clusters/app.ci/supplemental-ci-images/telco-runner.yaml (1)
40-40: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider pinning to a patched Go 1.25.x release instead of 1.25.0.
Go 1.25.0 is valid, but several point releases after it (1.25.1 through at least 1.25.12) ship security fixes to
net/http,crypto/tls,crypto/x509, and other packages. Since this image builds and runs test tooling, using an exact1.25.0tag forgoes those fixes.♻️ Proposed change
- RUN for version in 1.19 1.20 1.21.11 1.22.4 1.25.0; do \ + RUN for version in 1.19 1.20 1.21.11 1.22.4 1.25.8; do \🤖 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 `@clusters/app.ci/supplemental-ci-images/telco-runner.yaml` at line 40, Update the Go version list in the RUN loop to replace 1.25.0 with a patched Go 1.25.x point release, while preserving the existing versions and loop structure.
🤖 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 `@ci-operator/step-registry/telco5g/ptp/tests/Dockerfile.telco-runner`:
- Around line 3-9: Add a repository-approved non-root USER to the Dockerfile
after all root-only setup, ensuring the final image does not run as UID 0.
Before switching users, create and grant that user access to /go/.cache and all
required GOPATH paths, including any paths used by /output/fix_uid.sh; retain
root only for setup steps.
- Line 25: Update the chmod command in the Dockerfile’s Go toolchain setup to
remove group write permission, keeping `/usr/local/${version}/go`
read/execute-only. Grant write access only to the required output and cache
directories, preserving the root filesystem as read-only where possible.
In `@ci-operator/step-registry/telco5g/ptp/tests/run-in-container.sh`:
- Around line 244-252: Update both container-run paths in run-in-container.sh,
including the ssh-wrapped podman invocation and the local podman invocation, so
non-zero test exits are captured in status without being terminated by set -e.
Preserve the subsequent artifact copy-back, Results summary, and exit "$status"
flow for both success and failure outcomes.
- Around line 168-196: Replace the runtime sed-based patching in the
entrypoint-generation logic, including the SKIP_BUILD, SKIP_DEPLOY, SKIP_WAIT,
and TEST_MODES override blocks, with a stable interface for controlling the
commands script. Pass skip and test-mode settings through explicit environment
variables or arguments consumed by the target script, or invoke stable
functions/options directly, so behavior does not depend on matching exact
command text such as build_images, sleep 300, deployment anchors, or the
TEST_MODES array.
---
Nitpick comments:
In `@clusters/app.ci/supplemental-ci-images/telco-runner.yaml`:
- Line 40: Update the Go version list in the RUN loop to replace 1.25.0 with a
patched Go 1.25.x point release, while preserving the existing versions and loop
structure.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f8337b4-683e-43a6-89b9-d9d2a6eada0e
📒 Files selected for processing (3)
ci-operator/step-registry/telco5g/ptp/tests/Dockerfile.telco-runnerci-operator/step-registry/telco5g/ptp/tests/run-in-container.shclusters/app.ci/supplemental-ci-images/telco-runner.yaml
57d500d to
db07978
Compare
|
@jzding, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
db07978 to
9dd5708
Compare
|
@jzding, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
9dd5708 to
6c09daa
Compare
|
/pj-rehearse periodic-ci-openshift-release-main-nightly-5.0-e2e-telco5g-ptp-upstream |
|
@jzding: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
5f7512d to
63e1a41
Compare
jzding
changed the title
21df9a1 to
9c4a2e9
Compare
|
/pj-rehearse periodic-ci-openshift-release-main-nightly-5.0-e2e-telco5g-ptp-upstream |
|
@jzding: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-release-main-nightly-4.22-e2e-telco5g-ptp |
|
/pj-rehearse periodic-ci-openshift-release-main-nightly-4.22-e2e-telco5g-ptp |
|
@jzding: your |
|
@jzding: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
9c4a2e9 to
c9fc755
Compare
Refactor telco5g-ptp-tests-commands.sh for developer usability:
- Centralize all overridable env vars in a CONFIGURATION section at the
top (T5CI_VERSION, TEST_REPO, PTP_REPO, TEST_MODES, etc.)
- Extract the 80-line build script from inline pod YAML into a
build_script() function; separate pod YAML into build_pod_definition()
- Replace fragile array removal (${arr[@]/pattern}) with explicit
per-version TEST_MODES arrays and reference tables
- Add info logs marking the start of each major step
- Break builder pod polling loop on Failed status (was wasting 12 min
polling a dead pod, hiding the actual error behind log truncation)
No changes to registry login, podman push, Go version selection, or
test execution logic. Developers can now customize CI runs from a fork
without editing the script by exporting TEST_REPO, PTP_REPO, etc.
Signed-off-by: Jack Ding <jacding@redhat.com>
Signed-off-by: Jack Ding <jackding@gmail.com>
c9fc755 to
c41f55a
Compare
|
/pj-rehearse periodic-ci-openshift-release-main-nightly-4.22-e2e-telco5g-ptp |
|
@jzding: |
|
@jzding: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/pj-rehearse ack |
|
@jzding: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
|
/pj-rehearse periodic-ci-openshift-release-main-nightly-5.0-e2e-telco5g-ptp-upstream |
|
@jzding: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edcdavid, jzding 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 |
|
@jzding: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
9881ff5
into
openshift:main
2 participants
Summary
Refactor
telco5g-ptp-tests-commands.shfor developer usability without changing test behavior.Changes
T5CI_VERSION,TEST_REPO,PTP_REPO,TEST_MODES, etc.) in a single CONFIGURATION section at the top with reference tablesbuild_script()andbuild_pod_definition()functionsTEST_MODESarrays[INFO]markers at each major phase for easier log navigationFailedstatus instead of wasting 12 minutes polling a dead podWhat did NOT change
podman pushcommandsgolang-1.22.4for 4.16+,golang-1.20for 4.12-4.15)Developer usage
Test from a fork without editing the CI script:
Verification
make updatepassesbash -nsyntax check passes