TEST only - do not merge#79045
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughTwo CI step scripts were changed: the cluster-setup script now sets ChangesPTP Configuration Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: edcdavid 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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-cluster-setup/telco5g-ptp-cluster-setup-commands.sh (1)
301-307:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not hardcode
SKIP_OCP_INSTALLto true in shared step logic.Line 301 changes global default behavior and can cause jobs to run against stale/unprepared clusters (install and post-install config are skipped).
💡 Suggested fix
-SKIP_OCP_INSTALL="true" +: "${SKIP_OCP_INSTALL:=false}"If this PR needs skip behavior for a specific test, set
SKIP_OCP_INSTALL=truein that job’s env/config instead of the script default.🤖 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-cluster-setup/telco5g-ptp-cluster-setup-commands.sh` around lines 301 - 307, The script currently hardcodes SKIP_OCP_INSTALL="true", which overrides job configs; remove that assignment so SKIP_OCP_INSTALL is read from the environment (or set a safe default like "false"), leaving the existing if [[ "$SKIP_OCP_INSTALL" != "true" ]]; then ... fi block and the status handling intact; if a particular job needs skip behavior, set SKIP_OCP_INSTALL=true in that job's env/config rather than in the script (refer to the SKIP_OCP_INSTALL variable and the conditional that runs ANSIBLE_STDOUT_CALLBACK=debug ansible-playbook ... || status=$?).
🤖 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 406-409: The script currently clones a hardcoded fork/branch
("https://github.com/edcdavid/ptp-operator-upstream.git -b fix-wpc1") which
ignores the declared TEST_BRANCH; change the clone to use parameterized
variables (e.g. replace that line with a git clone
"${TEST_CONFORMANCE_REPO:-https://github.com/openshift/ptp-operator-upstream.git}"
-b "${TEST_BRANCH}" ptp-operator-conformance-test) so the repo and branch come
from TEST_CONFORMANCE_REPO and TEST_BRANCH (with sensible defaults) instead of
the hardcoded fork/branch; ensure any references to TEST_BRANCH earlier in the
script match this new clone invocation.
---
Outside diff comments:
In
`@ci-operator/step-registry/telco5g/ptp-cluster-setup/telco5g-ptp-cluster-setup-commands.sh`:
- Around line 301-307: The script currently hardcodes SKIP_OCP_INSTALL="true",
which overrides job configs; remove that assignment so SKIP_OCP_INSTALL is read
from the environment (or set a safe default like "false"), leaving the existing
if [[ "$SKIP_OCP_INSTALL" != "true" ]]; then ... fi block and the status
handling intact; if a particular job needs skip behavior, set
SKIP_OCP_INSTALL=true in that job's env/config rather than in the script (refer
to the SKIP_OCP_INSTALL variable and the conditional that runs
ANSIBLE_STDOUT_CALLBACK=debug ansible-playbook ... || status=$?).
🪄 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: Enterprise
Run ID: 3ad90005-24da-4296-878c-16e8b907137a
📒 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
| echo "running conformance tests from branch ${TEST_BRANCH}" | ||
| # always run test from latest upstream | ||
| git clone https://github.com/k8snetworkplumbingwg/ptp-operator.git -b "${TEST_BRANCH}" ptp-operator-conformance-test | ||
| git clone https://github.com/edcdavid/ptp-operator-upstream.git -b fix-wpc1 ptp-operator-conformance-test | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify branch exists in the configured/default conformance repo.
PTP_CONFORMANCE_REPO="${PTP_CONFORMANCE_REPO:-https://github.com/edcdavid/ptp-operator-upstream.git}"
PTP_CONFORMANCE_BRANCH="${PTP_CONFORMANCE_BRANCH:-fix-wpc1}"
echo "Checking ${PTP_CONFORMANCE_REPO}#${PTP_CONFORMANCE_BRANCH}"
git ls-remote --heads "${PTP_CONFORMANCE_REPO}" "${PTP_CONFORMANCE_BRANCH}"Repository: openshift/release
Length of output: 194
🏁 Script executed:
# First, let's see the context around lines 406-409 and understand TEST_BRANCH
sed -n '390,420p' ci-operator/step-registry/telco5g/ptp/tests/telco5g-ptp-tests-commands.shRepository: openshift/release
Length of output: 991
🏁 Script executed:
grep -n "TEST_BRANCH" ci-operator/step-registry/telco5g/ptp/tests/telco5g-ptp-tests-commands.shRepository: openshift/release
Length of output: 664
Remove hardcoded fork/branch and use parameterized TEST_BRANCH instead.
Line 408 hardcodes fix-wpc1 in a non-canonical fork, but line 406 announces testing against ${TEST_BRANCH} (which resolves to main). This inconsistency causes the script to ignore the declared branch parameter, creating a divergence between intent and execution, and introduces a brittle dependency on a personal fork that may disappear or change.
Parameterize the conformance repo and branch to respect the same branch controls used elsewhere in the script:
Suggested fix
echo "running conformance tests from branch ${TEST_BRANCH}"
# always run test from latest upstream
-git clone https://github.com/edcdavid/ptp-operator-upstream.git -b fix-wpc1 ptp-operator-conformance-test
+: "${PTP_CONFORMANCE_REPO:=https://github.com/k8snetworkplumbingwg/ptp-operator.git}"
+: "${PTP_CONFORMANCE_BRANCH:=${TEST_BRANCH}}"
+git clone --single-branch --branch "${PTP_CONFORMANCE_BRANCH}" "${PTP_CONFORMANCE_REPO}" ptp-operator-conformance-test🤖 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 406 - 409, The script currently clones a hardcoded fork/branch
("https://github.com/edcdavid/ptp-operator-upstream.git -b fix-wpc1") which
ignores the declared TEST_BRANCH; change the clone to use parameterized
variables (e.g. replace that line with a git clone
"${TEST_CONFORMANCE_REPO:-https://github.com/openshift/ptp-operator-upstream.git}"
-b "${TEST_BRANCH}" ptp-operator-conformance-test) so the repo and branch come
from TEST_CONFORMANCE_REPO and TEST_BRANCH (with sensible defaults) instead of
the hardcoded fork/branch; ensure any references to TEST_BRANCH earlier in the
script match this new clone invocation.
|
/pj-rehearse periodic-ci-openshift-release-main-nightly-4.22-e2e-telco5g-ptp-upstream |
|
@edcdavid: 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-upstream |
|
@edcdavid: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse abort |
1 similar comment
|
/pj-rehearse abort |
- Added 'eno12399' to the SKIP_INTERFACES variable in the telco5g PTP test commands script to refine interface exclusions during testing.
|
/pj-rehearse periodic-ci-openshift-release-main-nightly-4.22-e2e-telco5g-ptp-upstream |
|
/pj-rehearse periodic-ci-openshift-release-main-nightly-4.22-e2e-telco5g-ptp-upstream |
|
@edcdavid: 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: |
|
@edcdavid: The following test 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. |
1 participant
Update PTP test commands and cluster setup scripts
Telco5G PTP Testing Configuration Updates (openshift/release)
This PR updates the Telco5G CI/infrastructure used for PTP operator testing so the jobs run against an existing cluster and a specific development branch of the PTP operator.
Practical impact
Changes
Cluster setup script (ci-operator/step-registry/telco5g/ptp-cluster-setup/telco5g-ptp-cluster-setup-commands.sh)
PTP tests script (ci-operator/step-registry/telco5g/ptp/tests/telco5g-ptp-tests-commands.sh)
Metadata / notes