STOR-2998: use test manifests from top-level directory for GCP PD - #82761
STOR-2998: use test manifests from top-level directory for GCP PD#82761mpatlasov wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@mpatlasov: This pull request references STOR-2998 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. |
WalkthroughThe script detects current, legacy, and standalone repository layouts. It assigns ChangesGCP PD manifest path resolution
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
ci-operator/step-registry/storage/conf/csi-gcp-pd/storage-conf-csi-gcp-pd-commands.sh (1)
20-20: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winQuote all expanded manifest paths.
The new
cpcommands pass unquoted paths. If${SHARED_DIR}or a manifest variable contains whitespace or glob characters,cpcan receive incorrect arguments. Quote both operands. This addresses the reported ShellCheck SC2086 warnings.Proposed fix
- cp test/e2e/${GCPPD}/image-snapshot-manifest.yaml "${SHARED_DIR}/${TEST_CSI_DRIVER_MANIFEST}" + cp "test/e2e/${GCPPD}/image-snapshot-manifest.yaml" "${SHARED_DIR}/${TEST_CSI_DRIVER_MANIFEST}" ... - cp test/e2e/${GCPPD}/hyperdisk-manifest.yaml ${SHARED_DIR}/${TEST_CSI_DRIVER_MANIFEST} + cp "test/e2e/${GCPPD}/hyperdisk-manifest.yaml" "${SHARED_DIR}/${TEST_CSI_DRIVER_MANIFEST}" ... - cp test/e2e/${GCPPD}/manifest.yaml ${SHARED_DIR}/${TEST_CSI_DRIVER_MANIFEST} + cp "test/e2e/${GCPPD}/manifest.yaml" "${SHARED_DIR}/${TEST_CSI_DRIVER_MANIFEST}" ... - cp test/e2e/${GCPPD}/${TEST_VOLUME_ATTRIBUTES_CLASS_MANIFEST} ${SHARED_DIR}/${TEST_VOLUME_ATTRIBUTES_CLASS_MANIFEST} + cp "test/e2e/${GCPPD}/${TEST_VOLUME_ATTRIBUTES_CLASS_MANIFEST}" "${SHARED_DIR}/${TEST_VOLUME_ATTRIBUTES_CLASS_MANIFEST}" ... - cp test/e2e/${GCPPD}/ocp-manifest.yaml ${SHARED_DIR}/${TEST_OCP_CSI_DRIVER_MANIFEST} + cp "test/e2e/${GCPPD}/ocp-manifest.yaml" "${SHARED_DIR}/${TEST_OCP_CSI_DRIVER_MANIFEST}"Also applies to: 24-26, 31-31, 36-36
🤖 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/storage/conf/csi-gcp-pd/storage-conf-csi-gcp-pd-commands.sh` at line 20, Update each new cp command in the storage configuration script to quote both expanded source and destination manifest paths, including the GCPPD, SHARED_DIR, and TEST_CSI_DRIVER_MANIFEST expansions, so whitespace and glob characters are preserved and ShellCheck SC2086 warnings are resolved.Source: 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/storage/conf/csi-gcp-pd/storage-conf-csi-gcp-pd-commands.sh`:
- Around line 6-11: Initialize GCPPD in each fallback layout branch of the
storage configuration selection, setting it to "." before any later ${GCPPD}
expansion; retain GCPPD="gcp-pd" for the current-layout branch.
---
Nitpick comments:
In
`@ci-operator/step-registry/storage/conf/csi-gcp-pd/storage-conf-csi-gcp-pd-commands.sh`:
- Line 20: Update each new cp command in the storage configuration script to
quote both expanded source and destination manifest paths, including the GCPPD,
SHARED_DIR, and TEST_CSI_DRIVER_MANIFEST expansions, so whitespace and glob
characters are preserved and ShellCheck SC2086 warnings are resolved.
🪄 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: eefa83af-e58b-4667-8ed3-caa92923ac9d
📒 Files selected for processing (1)
ci-operator/step-registry/storage/conf/csi-gcp-pd/storage-conf-csi-gcp-pd-commands.sh
e312a7a to
392ceab
Compare
392ceab to
ae47e34
Compare
|
[REHEARSALNOTIFIER]
A total of 328 jobs have been affected by this change. The above listing is non-exhaustive and limited to 25 jobs. A full list of affected jobs can be found here 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/storage/conf/csi-gcp-pd/storage-conf-csi-gcp-pd-commands.sh`:
- Line 26: Quote both the source and destination path expansions in the affected
cp command, and quote every expanded path in the related cat commands, including
the final cat command. Update only the command path arguments while preserving
their existing 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: Enterprise
Run ID: 009570f6-da4b-470e-a2a2-a4f066ff2d79
📒 Files selected for processing (1)
ci-operator/step-registry/storage/conf/csi-gcp-pd/storage-conf-csi-gcp-pd-commands.sh
| elif [ "${COMPUTE_DISK_TYPE}" == "hyperdisk-balanced" ]; then | ||
| # Using hyperdisk-balanced worker | ||
| cp test/e2e/hyperdisk-manifest.yaml ${SHARED_DIR}/${TEST_CSI_DRIVER_MANIFEST} | ||
| cp test/e2e/${GCPPD}/hyperdisk-manifest.yaml ${SHARED_DIR}/${TEST_CSI_DRIVER_MANIFEST} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(git ls-files | rg '(^|/)storage-conf-csi-gcp-pd-commands\.sh$' | head -n 1)
printf '%s\n' "$file"
cat -n "$file"
printf '\nShellCheck availability:\n'
command -v shellcheck || trueRepository: openshift/release
Length of output: 2647
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='ci-operator/step-registry/storage/conf/csi-gcp-pd/storage-conf-csi-gcp-pd-commands.sh'
shellcheck "$file" || true
printf '\nUnquoted path-command expansions:\n'
rg -n '(^|[[:space:]])(cp|cat)[[:space:]].*\$\{' "$file"Repository: openshift/release
Length of output: 4775
Quote all expanded manifest paths.
Quote every source and destination path in the affected cp and cat commands. Also quote the final cat command at line 45.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 26-26: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 26-26: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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/storage/conf/csi-gcp-pd/storage-conf-csi-gcp-pd-commands.sh`
at line 26, Quote both the source and destination path expansions in the
affected cp command, and quote every expanded path in the related cat commands,
including the final cat command. Update only the command path arguments while
preserving their existing behavior.
Source: Linters/SAST tools
|
/lgtm |
|
@dobsonj: 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: dobsonj, mpatlasov 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 |
|
@mpatlasov: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-openshift-gcp-pd-csi-driver-operator-release-4.21-e2e-gcp-csi |
|
@mpatlasov: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@mpatlasov: all tests passed! 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. |
3 participants
https://redhat.atlassian.net/browse/STOR-2998
/legacysubdir is going way, let's use top-leveltest/e2e/gcp-pd.Summary by CodeRabbit
The GCP PD CSI test configuration now selects manifests from the top-level
test/e2e/gcp-pddirectory for the currentcsi-operatorlayout. It preserves legacy and standalone repository support. CSI, volume-attributes, and OpenShift manifest copies now use the selectedtest/e2e/${GCPPD}path.