Ptp fix - #83152
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: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe operator configuration removes selected ChangesOperator channel updates
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
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: 3
🤖 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/telcov10n/functional/cnf-ran/ibi-target-hub-config/telcov10n-functional-cnf-ran-ibi-target-hub-config-commands.sh`:
- Around line 2-3: Update the shell options at the start of the step script to
enable nounset alongside errexit and pipefail, using the required default `set
-euo pipefail` configuration.
In
`@ci-operator/step-registry/telcov10n/functional/cnf-ran/ibi-target-mirror-spoke-operators/telcov10n-functional-cnf-ran-ibi-target-mirror-spoke-operators-commands.sh`:
- Around line 2-3: Update the script’s shell options to enable nounset alongside
errexit and pipefail, using the standard commands-script default of set -euo
pipefail. This ensures the CLUSTER_NAME handoff is validated before constructing
the kubeconfig path.
In
`@ci-operator/step-registry/telcov10n/functional/cnf-ran/ibu-target-hub-deploy/telcov10n-functional-cnf-ran-ibu-target-hub-deploy-ref.yaml`:
- Around line 49-67: Remove the kni-qe-128 credential mount entries from the
target hub deploy ref, including the hub, bastion, group, and hypervisor mounts.
Update the documentation block to describe only the IBU target hub and its
supported default, kni-qe-109; leave IBI credentials to the separate IBI ref.
🪄 Autofix
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: adecc515-cc7e-4a02-9bd1-3e064207f517
📒 Files selected for processing (26)
ci-operator/config/openshift-kni/eco-ci-cd/openshift-kni-eco-ci-cd-main__cnf-ran-ibi-4.20.yamlci-operator/config/openshift-kni/eco-ci-cd/openshift-kni-eco-ci-cd-main__cnf-ran-ptp-sno-4.14.yamlci-operator/config/openshift-kni/eco-ci-cd/openshift-kni-eco-ci-cd-main__cnf-ran-ptp-sno-4.16.yamlci-operator/config/openshift-kni/eco-ci-cd/openshift-kni-eco-ci-cd-main__cnf-ran-ptp-sno-4.18.yamlci-operator/config/openshift-kni/eco-ci-cd/openshift-kni-eco-ci-cd-main__cnf-ran-ptp-sno-4.19.yamlci-operator/config/openshift-kni/eco-ci-cd/openshift-kni-eco-ci-cd-main__cnf-ran-ptp-sno-4.20.yamlci-operator/config/openshift-kni/eco-ci-cd/openshift-kni-eco-ci-cd-main__cnf-ran-ptp-sno-4.22.yamlci-operator/config/openshift-kni/eco-ci-cd/openshift-kni-eco-ci-cd-main__cnf-ran-ptp-sno-5.0.yamlci-operator/config/openshift-kni/eco-ci-cd/openshift-kni-eco-ci-cd-main__cnf-ran-ptp-sno-5.1.yamlci-operator/step-registry/telcov10n/functional/cnf-ran/hub-config/telcov10n-functional-cnf-ran-hub-config-commands.shci-operator/step-registry/telcov10n/functional/cnf-ran/hub-config/telcov10n-functional-cnf-ran-hub-config-ref.yamlci-operator/step-registry/telcov10n/functional/cnf-ran/hub-deploy/telcov10n-functional-cnf-ran-hub-deploy-commands.shci-operator/step-registry/telcov10n/functional/cnf-ran/ibi-target-hub-config/OWNERSci-operator/step-registry/telcov10n/functional/cnf-ran/ibi-target-hub-config/telcov10n-functional-cnf-ran-ibi-target-hub-config-commands.shci-operator/step-registry/telcov10n/functional/cnf-ran/ibi-target-hub-config/telcov10n-functional-cnf-ran-ibi-target-hub-config-ref.metadata.jsonci-operator/step-registry/telcov10n/functional/cnf-ran/ibi-target-hub-config/telcov10n-functional-cnf-ran-ibi-target-hub-config-ref.yamlci-operator/step-registry/telcov10n/functional/cnf-ran/ibi-target-hub-deploy/OWNERSci-operator/step-registry/telcov10n/functional/cnf-ran/ibi-target-hub-deploy/telcov10n-functional-cnf-ran-ibi-target-hub-deploy-commands.shci-operator/step-registry/telcov10n/functional/cnf-ran/ibi-target-hub-deploy/telcov10n-functional-cnf-ran-ibi-target-hub-deploy-ref.metadata.jsonci-operator/step-registry/telcov10n/functional/cnf-ran/ibi-target-hub-deploy/telcov10n-functional-cnf-ran-ibi-target-hub-deploy-ref.yamlci-operator/step-registry/telcov10n/functional/cnf-ran/ibi-target-mirror-spoke-operators/OWNERSci-operator/step-registry/telcov10n/functional/cnf-ran/ibi-target-mirror-spoke-operators/telcov10n-functional-cnf-ran-ibi-target-mirror-spoke-operators-commands.shci-operator/step-registry/telcov10n/functional/cnf-ran/ibi-target-mirror-spoke-operators/telcov10n-functional-cnf-ran-ibi-target-mirror-spoke-operators-ref.metadata.jsonci-operator/step-registry/telcov10n/functional/cnf-ran/ibi-target-mirror-spoke-operators/telcov10n-functional-cnf-ran-ibi-target-mirror-spoke-operators-ref.yamlci-operator/step-registry/telcov10n/functional/cnf-ran/ibi/telcov10n-functional-cnf-ran-ibi-workflow.yamlci-operator/step-registry/telcov10n/functional/cnf-ran/ibu-target-hub-deploy/telcov10n-functional-cnf-ran-ibu-target-hub-deploy-ref.yaml
💤 Files with no reviewable changes (7)
- ci-operator/config/openshift-kni/eco-ci-cd/openshift-kni-eco-ci-cd-main__cnf-ran-ptp-sno-4.20.yaml
- ci-operator/config/openshift-kni/eco-ci-cd/openshift-kni-eco-ci-cd-main__cnf-ran-ptp-sno-4.14.yaml
- ci-operator/config/openshift-kni/eco-ci-cd/openshift-kni-eco-ci-cd-main__cnf-ran-ptp-sno-5.1.yaml
- ci-operator/config/openshift-kni/eco-ci-cd/openshift-kni-eco-ci-cd-main__cnf-ran-ptp-sno-4.19.yaml
- ci-operator/config/openshift-kni/eco-ci-cd/openshift-kni-eco-ci-cd-main__cnf-ran-ptp-sno-4.22.yaml
- ci-operator/config/openshift-kni/eco-ci-cd/openshift-kni-eco-ci-cd-main__cnf-ran-ptp-sno-4.16.yaml
- ci-operator/config/openshift-kni/eco-ci-cd/openshift-kni-eco-ci-cd-main__cnf-ran-ptp-sno-5.0.yaml
| set -e | ||
| set -o pipefail |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Enable nounset in the step script.
Line 2 and Line 3 omit nounset. An unset input can expand to an empty value and produce an invalid deployment configuration. Use the required default shell options.
As per coding guidelines, step registry command scripts must default to set -euo pipefail.
Proposed fix
-set -e
-set -o pipefail
+set -euo pipefail📝 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.
| set -e | |
| set -o pipefail | |
| set -euo pipefail |
🤖 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/telcov10n/functional/cnf-ran/ibi-target-hub-config/telcov10n-functional-cnf-ran-ibi-target-hub-config-commands.sh`
around lines 2 - 3, Update the shell options at the start of the step script to
enable nounset alongside errexit and pipefail, using the required default `set
-euo pipefail` configuration.
Source: Coding guidelines
| set -e | ||
| set -o pipefail |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Enable unset-variable checks.
The script can continue with an empty CLUSTER_NAME when ${SHARED_DIR}/cluster_name is absent. It then constructs an invalid kubeconfig path on Line 30. Enable nounset so the step fails at the missing handoff boundary.
Proposed fix
-set -e
-set -o pipefail
+set -euo pipefailAs per coding guidelines, "**/*-commands.sh: ... default to set -euo pipefail without -x."
📝 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.
| set -e | |
| set -o pipefail | |
| set -euo pipefail |
🤖 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/telcov10n/functional/cnf-ran/ibi-target-mirror-spoke-operators/telcov10n-functional-cnf-ran-ibi-target-mirror-spoke-operators-commands.sh`
around lines 2 - 3, Update the script’s shell options to enable nounset
alongside errexit and pipefail, using the standard commands-script default of
set -euo pipefail. This ensures the CLUSTER_NAME handoff is validated before
constructing the kubeconfig path.
Source: Coding guidelines
| # kni-qe-128 (IBI target hub) | ||
| - namespace: test-credentials | ||
| name: telcov10n-ansible-hub-kni-qe-128 | ||
| mount_path: /var/host_variables/kni-qe-128/master0 | ||
| - namespace: test-credentials | ||
| name: telcov10n-ansible-kni-qe-128-bastion | ||
| mount_path: /var/host_variables/kni-qe-128/bastion | ||
| - namespace: test-credentials | ||
| name: telcov10n-ansible-group-hub-kni-qe-128-nodes | ||
| mount_path: /var/group_variables/kni-qe-128/nodes | ||
| - namespace: test-credentials | ||
| name: telcov10n-ansible-group-hub-kni-qe-128-masters | ||
| mount_path: /var/group_variables/kni-qe-128/masters | ||
| - namespace: test-credentials | ||
| name: telcov10n-ansible-hypervisors-helix107 | ||
| mount_path: /var/host_variables/kni-qe-128/hypervisor | ||
| documentation: |- | ||
| Deploy IBU target hub (kni-qe-109). | ||
| The target hub manages the spoke clusters that will be upgraded via IBU. | ||
| Deploy IBU/IBI target hub (kni-qe-109 or kni-qe-128). | ||
| The target hub manages the spoke clusters that will be upgraded/installed via IBU/IBI. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep IBI credentials in the IBI ref.
telcov10n-functional-cnf-ran-ibu-target-hub-deploy remains the IBU ref and defaults to kni-qe-109. The IBI workflow already invokes a separate telcov10n-functional-cnf-ran-ibi-target-hub-deploy ref with its own kni-qe-128 mounts. (raw.githubusercontent.com)
Remove the kni-qe-128 credential mounts from this IBU ref. Restore the documentation to describe only the IBU target hub. Otherwise, IBU jobs receive unrelated IBI credentials.
Proposed fix
- # kni-qe-128 (IBI target hub)
- - namespace: test-credentials
- name: telcov10n-ansible-hub-kni-qe-128
- mount_path: /var/host_variables/kni-qe-128/master0
- - namespace: test-credentials
- name: telcov10n-ansible-kni-qe-128-bastion
- mount_path: /var/host_variables/kni-qe-128/bastion
- - namespace: test-credentials
- name: telcov10n-ansible-group-hub-kni-qe-128-nodes
- mount_path: /var/group_variables/kni-qe-128/nodes
- - namespace: test-credentials
- name: telcov10n-ansible-group-hub-kni-qe-128-masters
- mount_path: /var/group_variables/kni-qe-128/masters
- - namespace: test-credentials
- name: telcov10n-ansible-hypervisors-helix107
- mount_path: /var/host_variables/kni-qe-128/hypervisor🤖 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/telcov10n/functional/cnf-ran/ibu-target-hub-deploy/telcov10n-functional-cnf-ran-ibu-target-hub-deploy-ref.yaml`
around lines 49 - 67, Remove the kni-qe-128 credential mount entries from the
target hub deploy ref, including the hub, bastion, group, and hypervisor mounts.
Update the documentation block to describe only the IBU target hub and its
supported default, kni-qe-109; leave IBI credentials to the separate IBI ref.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TimurMP 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 |
|
/pj-rehearse periodic-ci-openshift-kni-eco-ci-cd-main-cnf-ran-ptp-sno-4.18-cnf-ran-ptp-tests |
|
@TimurMP: 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: |
|
@TimurMP: 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
Summary by CodeRabbit
cnf-ran-ptp-sno-4.18CI configuration for hub and spoke clusters.default_channelfrom Advanced Cluster Management, Multicluster Engine, and Cluster Logging hub operator definitions.stable-6.4tostable-6.1and removes itsdefault_channel.