INTEROP-8979: Wire Slack webhook into firewatch step#78332
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@amp-rh: This pull request references INTEROP-8979 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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. |
openshift-ci
Bot
added
the
needs-rebase
WalkthroughThis pull request consolidates and refactors IBM Fusion Access Operator CI step implementations under a unified Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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 |
|
@amp-rh, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, 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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ci-operator/step-registry/ibm-fusion-access/create-local-disks/ibm-fusion-access-create-local-disks-commands.sh (1)
15-16:⚠️ Potential issue | 🟡 MinorConsume
FA__LOCALDISK_NAMEinstead of hardcoding the prefix.The ref.yaml now declares
FA__LOCALDISK_NAME(default"shared-ebs-disk") specifically to parameterize this prefix, but the script still uses the literal. Read the env var here so overrides from job configs take effect, and update the "Inputs:" header on line 5 accordingly.🛠️ Proposed fix
-# Inputs: FA__SCALE__NAMESPACE (step ref env); cluster must have workers with NVMe devices as expected by the step. +# Inputs: FA__SCALE__NAMESPACE, FA__LOCALDISK_NAME (step ref env); cluster must have workers with NVMe devices as expected by the step. @@ for device in "${deviceArr[@]}"; do - typeset localDiskName="shared-ebs-disk-${diskIdx}" + typeset localDiskName="${FA__LOCALDISK_NAME}-${diskIdx}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/ibm-fusion-access/create-local-disks/ibm-fusion-access-create-local-disks-commands.sh` around lines 15 - 16, The script currently hardcodes the local disk prefix when creating localDiskName; change it to read the FA__LOCALDISK_NAME environment variable (falling back to the existing default if empty) and use that prefix when building localDiskName (refer to variables deviceArr, diskIdx and localDiskName). Also update the "Inputs:" header to list FA__LOCALDISK_NAME as an accepted/environment parameter so job overrides are clear. Ensure the behavior is identical when FA__LOCALDISK_NAME is unset by preserving the previous default prefix.ci-operator/step-registry/interop-tests/ibm-fusion-access/verify-shared-storage/interop-tests-ibm-fusion-access-verify-shared-storage-ref.yaml (1)
16-28:⚠️ Potential issue | 🟡 MinorMinor:
FA__CNV__NAMESPACEandFA__NAMESPACEare declared but unused.The ref declares 4 environment variables, but the script only uses
FA__CNV__TEST_NAMESPACEandFA__CNV__SHARED_STORAGE_CLASS. RemoveFA__CNV__NAMESPACEandFA__NAMESPACEfrom the ref unless they are intended for future use.Additionally, the script header comments that
MAP_TESTSis an input, but it is never referenced in the actual script code—either remove it from the header or implement the mapping logic if intended.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/interop-tests/ibm-fusion-access/verify-shared-storage/interop-tests-ibm-fusion-access-verify-shared-storage-ref.yaml` around lines 16 - 28, Remove the unused environment variables FA__CNV__NAMESPACE and FA__NAMESPACE from the YAML ref (they are declared but never used by the script), or if they are intended, wire them into the script where required (e.g., replace hard-coded namespaces with FA__CNV__NAMESPACE/FA__NAMESPACE in functions that reference CNV/IBM Fusion Access namespaces such as the shared-storage setup/cleanup routines and any namespace-scoped kubectl/oc calls); also audit the script header note about MAP_TESTS—either remove MAP_TESTS from the header if not used or implement the mapping logic so MAP_TESTS is consumed by the test runner (update references where tests are selected/mapped) to keep declarations consistent with actual usage.
🧹 Nitpick comments (21)
ci-operator/step-registry/interop-tests/ibm-fusion-access/prepare-lxtrace-files/interop-tests-ibm-fusion-access-prepare-lxtrace-files-commands.sh (2)
13-15: Silentexit 1on zero workers.Emit a one-line message before bailing so on-call doesn't have to piece it together from the
oc get nodesJSON dump.♻️ Optional
if [[ "${workerCount}" -le 0 ]]; then + echo "ERROR: no nodes match node-role.kubernetes.io/worker=" >&2 exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/interop-tests/ibm-fusion-access/prepare-lxtrace-files/interop-tests-ibm-fusion-access-prepare-lxtrace-files-commands.sh` around lines 13 - 15, The script currently exits silently when workerCount is <= 0; update the conditional that checks workerCount (the if [[ "${workerCount}" -le 0 ]] block) to emit a concise one-line error message to stderr (mentioning workerCount is zero/unavailable) immediately before calling exit 1 so on-call can see why the script aborted.
19-40: LGTM — robust inner shell; one observability nit on the sed filter.Inner bash uses
-eux -o pipefailandinherit_errexit nullglob, so the glob is safe when no files match and any failure in the inner block propagates.pipefailalso ensuresoc debugfailures still fail the outer script despite the trailing| sed.Nit: the
sed -e '/Starting pod/d' -e '/Removing debug/d' -e '/To use host/d'strips lines by substring on the merged stdout+stderr stream. If a futureocerror message happens to contain one of those substrings it would be dropped silently. A slightly more targeted filter (anchored tooc debug's known prefixes) would be safer:♻️ Optional
- ' 2>&1 | sed -e '/Starting pod/d' -e '/Removing debug/d' -e '/To use host/d' + ' 2>&1 | sed -E \ + -e '/^Starting pod\/[^ ]+ \.\.\./d' \ + -e '/^Removing debug pod \.\.\./d' \ + -e '/^To use host binaries/d'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/interop-tests/ibm-fusion-access/prepare-lxtrace-files/interop-tests-ibm-fusion-access-prepare-lxtrace-files-commands.sh` around lines 19 - 40, The sed filter currently deletes any line containing the substrings '/Starting pod/', '/Removing debug/', '/To use host/' which can accidentally drop unrelated oc output; update the pipeline after oc debug (the sed invocation that follows oc debug ... 2>&1 | sed -e '/Starting pod/d' -e '/Removing debug/d' -e '/To use host/d') to match those messages more precisely (e.g. anchor the patterns to line starts or the exact oc debug prefix such as using '^Starting pod', '^Removing debug', '^To use host') so only the intended oc debug status lines are removed.ci-operator/step-registry/ibm-fusion-access/prepare-nodes/ibm-fusion-access-prepare-nodes-commands.sh (1)
10-17: Solid refactor; consider an explicit assertion message.The JSON-once +
jq+while readpattern is a genuine robustness win over the previousfor node in $(oc get ... jsonpath=...)loop (no word-splitting hazards, single API round-trip).Nit:
((labeledCount))relies onset -eto abort when the count is zero, which works but is a bit cryptic for someone debugging a "no storage-labeled workers" failure. Optional readability tweak:♻️ Optional: clearer guard
-((labeledCount)) +if (( labeledCount == 0 )); then + echo "ERROR: no workers matched scale.spectrum.ibm.com/role=storage" >&2 + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/ibm-fusion-access/prepare-nodes/ibm-fusion-access-prepare-nodes-commands.sh` around lines 10 - 17, The current check uses ((labeledCount)) which implicitly relies on set -e and is cryptic; replace that with an explicit assertion that fails with a clear message when there are no storage-labeled worker nodes: after computing nodesJson and labeledCount (variables nodesJson and labeledCount in the snippet and the jq expression), add an if test that compares labeledCount to zero (e.g., if [[ "$labeledCount" -eq 0 ]]) and on true print a descriptive error like "No storage-labeled worker nodes found" to stderr and exit 1; otherwise continue normally. Ensure you reference and use the labeledCount value computed by the jq expression and remove the ((labeledCount)) line.ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.sh (2)
49-49: Pre-existing but worth noting while touching this file:echo $report_commandis unquoted.Word-splitting/globbing would apply to any path-looking fragments in
report_command(e.g.--firewatch-config-path=...). It happens to work today because none of the appended fragments contain glob metacharacters, butecho "$report_command"would be safer and future-proof. Not introduced by this PR — entirely optional to fix here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.sh` at line 49, The echo invocation currently uses an unquoted variable (echo $report_command) which allows word-splitting and globbing; update the script to quote the variable (use echo "$report_command") wherever report_command is printed (the echo $report_command line) to ensure paths or arguments with spaces or glob characters are preserved and not expanded.
45-47: Split declare/export to surfacecatfailures (SC2155).
export SLACK_WEBHOOK_URL=$(cat /tmp/secrets/slack/url)always succeeds (export's exit code masks cat's), so a read failure on a mounted-but-unreadable secret would silently produce an emptySLACK_WEBHOOK_URLand the firewatch run would proceed with Slack disabled rather than failing fast. Declaring separately letsset -o errexitcatch the failure.🛠️ Proposed fix
if [ -f /tmp/secrets/slack/url ]; then - export SLACK_WEBHOOK_URL=$(cat /tmp/secrets/slack/url) + SLACK_WEBHOOK_URL=$(cat /tmp/secrets/slack/url) + export SLACK_WEBHOOK_URL fiAdditionally, consider that under
set -o nounset(line 3) this conditional-export is only safe because the ref declaresSLACK_WEBHOOK_URLwithdefault: "". If anyone later removes that env declaration fromfirewatch-report-issues-ref.yaml, a missing secret file would leaveSLACK_WEBHOOK_URLunbound and any downstream read of it (e.g. in the firewatch binary's debug output, or if a future script line references${SLACK_WEBHOOK_URL}) would abort. Leaving the ref default in place is the safer posture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.sh` around lines 45 - 47, Replace the combined export/command with two steps so cat failures propagate: first read the secret into the SLACK_WEBHOOK_URL variable (e.g. SLACK_WEBHOOK_URL="$(cat /tmp/secrets/slack/url)") and then export SLACK_WEBHOOK_URL in a separate statement; ensure this change is used where SLACK_WEBHOOK_URL is referenced and keep the ref default for SLACK_WEBHOOK_URL in the ref (so downstream scripts using nounset remain safe).ci-operator/step-registry/interop-tests/ibm-fusion-access/verify-shared-storage/interop-tests-ibm-fusion-access-verify-shared-storage-commands.sh (1)
106-117: Minor:namespace: default/storageClassName: defaultin the heredoc are misleading placeholders.Both values are overwritten by the jq filter on lines 98-99 (
.metadata.namespace=$ns | .spec.storageClassName=$sc), so behavior is correct, butdefaultis itself a real Kubernetes namespace and a real storage-class name — if someone ever removes the jq step or the heredoc gets accidentally applied without the pipeline, the PVC would land in the realdefaultnamespace withstorageClassName=default. Consider usingplaceholder/placeholder-sclike the VM-migration script does (lines 135-147 there) to make the intent obvious and reduce the blast radius of future edits. Same nit applies at lines 147 and 198 for the two pod manifests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/interop-tests/ibm-fusion-access/verify-shared-storage/interop-tests-ibm-fusion-access-verify-shared-storage-commands.sh` around lines 106 - 117, The heredoc PVC and pod manifests currently use real Kubernetes names ("namespace: default" and "storageClassName: default") which are overwritten by the jq filter ".metadata.namespace=$ns | .spec.storageClassName=$sc"; change those literal values to safe placeholders (e.g. "namespace: placeholder" and "storageClassName: placeholder-sc") in the PVC heredoc and the two pod manifest heredocs (the blocks referenced around the other occurrences) so accidental direct application won't target the real default namespace/storage-class; keep the jq filter unchanged so runtime substitution still sets the real values.ci-operator/step-registry/interop-tests/ibm-fusion-access/configure-cnv-shared-storage/interop-tests-ibm-fusion-access-configure-cnv-shared-storage-commands.sh (1)
210-220: Verify CSI driver pod label selector against actual IBM deployment.The operator label
-l app.kubernetes.io/name=ibm-spectrum-scale-csi-operatorat line 211 is confirmed correct per IBM official installer manifests. However, the driver pod label-l app=ibm-spectrum-scale-csiat line 214 is not explicitly documented in IBM sources, though it follows IBM naming conventions. Sinceoc waitfails with "no matching resources found" if the selector matches no pods, confirm this label matches the actual CSI driver DaemonSet pods in your target Fusion Access release. Theoc get podsoutput at line 219 will help troubleshoot if label mismatch occurs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/interop-tests/ibm-fusion-access/configure-cnv-shared-storage/interop-tests-ibm-fusion-access-configure-cnv-shared-storage-commands.sh` around lines 210 - 220, The oc wait selector for the CSI driver pod (-l app=ibm-spectrum-scale-csi used in the oc wait command that follows the operator wait) may not match the actual driver pod labels in the target Fusion Access release, causing "no matching resources found" failures; update the driver selector to the correct label(s) for the daemonset used in your deployment (or use a more robust selector such as matching the daemonset name or label keys from the actual pods) by changing the second oc wait invocation that references -l app=ibm-spectrum-scale-csi and -n "${FA__SCALE__CSI_NAMESPACE}" to the verified label(s) (or an alternative selection approach) so oc wait finds the CSI driver pods, and keep the existing fallback behavior that sets testMessage="CSI driver not ready within 300s" and dumps oc get pods -n "${FA__SCALE__CSI_NAMESPACE}" for troubleshooting.ci-operator/step-registry/ibm-fusion-access/configure-aws-security-groups/ibm-fusion-access-configure-aws-security-groups-commands.sh (1)
8-9: Consider emitting an error message whenmetadata.jsonis missing.Under
set -eux, a failed[ -f ... ]will exit the step but without a helpful diagnostic — just thextraceline. If this step is run before the installer completes (orSHARED_DIRis empty for any reason), debugging gets harder than it needs to be.Proposed diff
-[ -f "${SHARED_DIR}/metadata.json" ] -infraId="$(jq -r '.infraID' "${SHARED_DIR}/metadata.json")" +if [[ ! -f "${SHARED_DIR}/metadata.json" ]]; then + printf 'metadata.json not found in SHARED_DIR; was the installer step run?\n' >&2 + exit 1 +fi +infraId="$(jq -r '.infraID' "${SHARED_DIR}/metadata.json")"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/ibm-fusion-access/configure-aws-security-groups/ibm-fusion-access-configure-aws-security-groups-commands.sh` around lines 8 - 9, The script currently checks for metadata.json with [ -f "${SHARED_DIR}/metadata.json" ] which under set -eux will abort silently; update the check to explicitly test for the file and emit a clear error before exiting: test for existence of "${SHARED_DIR}/metadata.json" and if missing call echo or printf to stderr with a descriptive message (e.g., "metadata.json not found in SHARED_DIR: $SHARED_DIR") and exit 1, then proceed to populate infraId="$(jq -r '.infraID' "${SHARED_DIR}/metadata.json")"; ensure you reference the existing SHARED_DIR and infraId symbols and keep the check adjacent to the infraId assignment so the failure clearly points to the missing metadata.json.ci-operator/step-registry/interop-tests/ibm-fusion-access/label-nodes/interop-tests-ibm-fusion-access-label-nodes-commands.sh (1)
8-8: Minor:worker=selector is slightly narrower thanworker(exists).
-l node-role.kubernetes.io/worker=selects nodes where the label value equals empty string; the prior-l node-role.kubernetes.io/workerselects nodes that simply have the label set (any value). In standard OCP both match the same set, but if a downstream MachineConfigPool ever populates a non-empty value on the worker role label, this selector would silently match zero nodes and the step would fail. Consider keeping the "exists" form unless the equality was intentional.Proposed diff
-oc label nodes -l node-role.kubernetes.io/worker= scale.spectrum.ibm.com/role=storage --overwrite +oc label nodes -l node-role.kubernetes.io/worker scale.spectrum.ibm.com/role=storage --overwrite🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/interop-tests/ibm-fusion-access/label-nodes/interop-tests-ibm-fusion-access-label-nodes-commands.sh` at line 8, The selector uses an equality check which matches only an empty value; update the oc label command in interop-tests-ibm-fusion-access-label-nodes-commands.sh to use the "exists" form so it selects nodes that have the node-role.kubernetes.io/worker label regardless of value: replace the selector token node-role.kubernetes.io/worker= with node-role.kubernetes.io/worker in the oc label nodes invocation (the rest of the command, including scale.spectrum.ibm.com/role=storage and --overwrite, should remain unchanged).ci-operator/step-registry/ibm-fusion-access/check-crds/ibm-fusion-access-check-crds-commands.sh (1)
83-83: Redundant brace group around the subshell in the EXIT trap.
{( GenerateJunitXml; true )}wraps a subshell in an already-unnecessary brace group; bash's{ ... }grouping normally requires a terminating;/newline before}, so this form is fragile to future edits. The intent (runGenerateJunitXmlon EXIT and never propagate its failure) is clearer written as a single subshell or with explicit|| true.♻️ Proposed simplification
-trap '{( GenerateJunitXml; true )}' EXIT +trap 'GenerateJunitXml || true' EXIT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/ibm-fusion-access/check-crds/ibm-fusion-access-check-crds-commands.sh` at line 83, The EXIT trap currently uses a redundant and fragile brace group around a subshell: "trap '{( GenerateJunitXml; true )}' EXIT"; remove the braces and simplify so failures from GenerateJunitXml are ignored (e.g., use "trap 'GenerateJunitXml || true' EXIT" or a plain subshell "trap '( GenerateJunitXml; true )' EXIT"). Update the trap line referencing GenerateJunitXml accordingly to avoid the unnecessary "{ ... }" grouping.ci-operator/config/RedHatQE/firewatch/RedHatQE-firewatch-main__slack-webhook-test.yaml (1)
1-10: Minor: CLI base image and build_root OCP versions are mismatched (4.21 vs 4.20).
base_images.clipins OCP 4.21 whilebuild_rootusesrhel-9-release-golang-1.23-openshift-4.20. Thenooptest step only runsecho, so there is no functional impact today, but mixing OCP 4.21 CLI with 4.20 build root is inconsistent with other firewatch/interop variants and may surprise anyone copying this config. Recommend pinning both to the same release (e.g., both 4.20 or both 4.21).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/config/RedHatQE/firewatch/RedHatQE-firewatch-main__slack-webhook-test.yaml` around lines 1 - 10, The YAML mixes OCP versions: base_images.cli.name is "4.21" while build_root.image_stream_tag.tag references "openshift-4.20"; update one to match the other so both are pinned to the same release (e.g., set base_images.cli.name to "4.20" or change build_root.image_stream_tag.tag to use "openshift-4.21"), ensuring the fields base_images.cli (name) and build_root.image_stream_tag.tag are consistent across this config.ci-operator/step-registry/ibm-fusion-access/install-fusion-access-operator/ibm-fusion-access-install-fusion-access-operator-commands.sh (1)
100-103: Useoc wait --for=createto match established patterns in the codebase.The jsonpath condition
--for=jsonpath='{.metadata.name}'=fusionaccess-objectis functional but indirect. Thecliimage already supports--for=create(used throughout the step-registry, including line 9 of this same file), which more clearly expresses "wait for resource creation."♻️ Proposed simplification
-if ! oc wait --for=jsonpath='{.metadata.name}'=fusionaccess-object fusionaccess/fusionaccess-object -n "${FA__NAMESPACE}" --timeout=300s; then +if ! oc wait --for=create fusionaccess/fusionaccess-object -n "${FA__NAMESPACE}" --timeout=300s; then oc get fusionaccess fusionaccess-object -n "${FA__NAMESPACE}" -o yaml --ignore-not-found exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/ibm-fusion-access/install-fusion-access-operator/ibm-fusion-access-install-fusion-access-operator-commands.sh` around lines 100 - 103, Replace the indirect jsonpath wait with the clearer create check: change the oc wait invocation that targets fusionaccess/fusionaccess-object (namespace variable FA__NAMESPACE) to use --for=create (keeping the same --timeout=300s) so it matches the rest of the step-registry pattern; retain the existing oc get fusionaccess ... -o yaml --ignore-not-found and exit 1 error handling if the wait fails.ci-operator/step-registry/interop-tests/ibm-fusion-access/test-cnv-shared-storage/interop-tests-ibm-fusion-access-test-cnv-shared-storage-commands.sh (2)
256-264: Nit:if ! cmd; then true; fiis equivalent tocmd || true.Minor readability nit — the pattern shows up twice in this block and reads cleaner as
oc logs ... || true/oc describe pod ... || true. No functional change.🔧 Proposed fix
- if ! oc logs test-shared-storage-pod -n "${FA__CNV__TEST_NAMESPACE}" --tail=10; then - true - fi + oc logs test-shared-storage-pod -n "${FA__CNV__TEST_NAMESPACE}" --tail=10 || true else testMessage="Pod not ready within 2m timeout" oc get pod test-shared-storage-pod -n "${FA__CNV__TEST_NAMESPACE}" -o yaml --ignore-not-found - if ! oc describe pod test-shared-storage-pod -n "${FA__CNV__TEST_NAMESPACE}"; then - true - fi + oc describe pod test-shared-storage-pod -n "${FA__CNV__TEST_NAMESPACE}" || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/interop-tests/ibm-fusion-access/test-cnv-shared-storage/interop-tests-ibm-fusion-access-test-cnv-shared-storage-commands.sh` around lines 256 - 264, Replace the verbose "if ! <cmd>; then true; fi" patterns with the simpler " <cmd> || true" for the two occurrences in this block: the oc logs call and the oc describe pod call (both referencing "test-shared-storage-pod" and namespace variable FA__CNV__TEST_NAMESPACE); this keeps identical behavior but improves readability by using the idiomatic "command || true" form instead of the multi-line if/then/fi construct.
50-68: Inconsistent with sibling scripts:MapTestsForComponentReadiness/InstallYQIfNotExistsare missing here.Every other JUnit-emitting step in this PR (
ibm-fusion-access-check-nodes,ibm-fusion-access-create-cluster,ibm-fusion-access-configure-kmm-registry, andinterop-tests-ibm-fusion-access-verify-cluster) post-processes the JUnit XML withyqto stampREPORTPORTAL_CMPonto the testsuite name whenMAP_TESTS=true.GenerateJunitXmlin this file writes the file and copies it toSHARED_DIRbut skips the remap step, so the CNV shared-storage suite will not be component-mapped in ReportPortal even when the enclosing config setsMAP_TESTS=trueandREPORTPORTAL_CMP=Fusion-access-lp-interop. Either intentionally opt out (and document it in the ref.yaml) or add the same helpers:🔧 Proposed fix
+function InstallYQIfNotExists () { + if ! command -v yq >/dev/null; then + mkdir -p /tmp/bin + export PATH="${PATH}:/tmp/bin" + curl -L "https://github.com/mikefarah/yq/releases/latest/download/yq_linux_$(uname -m | sed 's/aarch64/arm64/;s/x86_64/amd64/')" \ + -o /tmp/bin/yq && chmod +x /tmp/bin/yq + fi + true +} + +function MapTestsForComponentReadiness () { + [[ "${MAP_TESTS:-false}" != "true" ]] && return + typeset resultsFile="${1:-}" + if [[ -n "${resultsFile}" ]] && [[ -f "${resultsFile}" ]]; then + InstallYQIfNotExists + export REPORTPORTAL_CMP="${REPORTPORTAL_CMP:-Fusion-access-lp-interop}" + yq eval -px -ox -iI0 '.testsuites.testsuite.+@name=env(REPORTPORTAL_CMP)' "${resultsFile}" + fi + true +} + function GenerateJunitXml () { typeset -i totalDuration=0 totalDuration=$((SECONDS - testStartTime)) @@ EOF + + MapTestsForComponentReadiness "${junitResultsFile}" if [[ -n "${SHARED_DIR}" ]] && [[ -d "${SHARED_DIR}" ]]; then cp "${junitResultsFile}" "${SHARED_DIR}/junit_cnv_shared_storage_tests.xml" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/interop-tests/ibm-fusion-access/test-cnv-shared-storage/interop-tests-ibm-fusion-access-test-cnv-shared-storage-commands.sh` around lines 50 - 68, GenerateJunitXml currently writes and copies the JUnit file but skips the MAP_TESTS remapping step that other suites perform; add the same pre/post helpers used elsewhere by invoking InstallYQIfNotExists and MapTestsForComponentReadiness (or call the equivalent yq-based remap logic) after writing "${junitResultsFile}" so that when MAP_TESTS=true and REPORTPORTAL_CMP is set the testsuite name is stamped via yq before copying to SHARED_DIR; ensure you reference the existing helper function names InstallYQIfNotExists and MapTestsForComponentReadiness and preserve the current copy-to-SHARED_DIR behavior.ci-operator/step-registry/ibm-fusion-access/check-nodes/ibm-fusion-access-check-nodes-commands.sh (1)
100-102:[[ ... ]]assertion is redundant —GenerateJunitXmlalready exits 1 on failed tests.Because the EXIT trap runs
GenerateJunitXml, which itself doesexit 1whentestsFailed > 0, the bare[[ "${testStatus}" == 'passed' ]]on line 100 has no added effect: on failure, set -e triggers trap→exit 1 via GenerateJunitXml; on success, the assertion is a no-op. It also means the trailingtrueon line 102 is dead code. You can drop both lines, matching the shape ofibm-fusion-access-create-cluster-commands.sh:173-176:🔧 Proposed fix
-[[ "${testStatus}" == 'passed' ]] - -true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/ibm-fusion-access/check-nodes/ibm-fusion-access-check-nodes-commands.sh` around lines 100 - 102, Remove the redundant conditional and dead statement: delete the standalone assertion that checks [[ "${testStatus}" == 'passed' ]] and the following stray true, since GenerateJunitXml already enforces exit on failures (it exits 1 when testsFailed > 0) and this assertion adds no behavior; locate references to the testStatus variable and the EXIT trap that calls GenerateJunitXml to confirm behavior remains unchanged after removing those two lines.ci-operator/step-registry/ibm-fusion-access/configure-kmm-registry/ibm-fusion-access-configure-kmm-registry-commands.sh (2)
105-136:set +xdoes not mask secrets passed via--from-literal; consider--from-filewith a restricted temp file.Hiding xtrace helps with the literal lines themselves, but
FA__KMM__REGISTRY_URLis still present in the process argv (visible viaps, audit logs, and any subprocess inheriting fds) and is baked into the pipeline thatoc applyingests. If the registry URL is truly sensitive, write it to amktempfile with0600perms and use--from-file=kmm_image_registry_url=<file>so it doesn't appear in command-line arguments. If it isn't actually sensitive, you can drop theset +x/set -xdance and simplify the block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/ibm-fusion-access/configure-kmm-registry/ibm-fusion-access-configure-kmm-registry-commands.sh` around lines 105 - 136, The configmap creation uses --from-literal with FA__KMM__REGISTRY_URL which exposes the secret on the command line; instead, create a secure temp file (mktemp) with 0600 perms, write FA__KMM__REGISTRY_URL into it, and change the oc create configmap invocations for kmm-image-config to use --from-file=kmm_image_registry_url=<tempfile> for both FA__NAMESPACE and FA__SCALE__OPERATOR_NAMESPACE blocks (ensure you remove/avoid exposing the value on argv and clean up the temp file), preserving the existing AddTestResult timing/ts/msg logic and keeping set +x/set -x around only the minimal non-sensitive parts.
49-49: Consider using a defensive default for consistency with sibling scripts.The
ibm-fusion-access-configure-kmm-registry-ref.yamldeclaresMAP_TESTSwithdefault: "false", so the env var is guaranteed to be set when the step runs, and[[ "${MAP_TESTS}" != "true" ]]is functionally safe underset -u. However, the sibling scripts (ibm-fusion-access-create-cluster-commands.shandibm-fusion-access-check-nodes-commands.sh) both use the defensive form[[ "${MAP_TESTS:-false}" != "true" ]]. Using the same pattern here would improve consistency and follow defensive programming conventions.🔧 Optional improvement
function MapTestsForComponentReadiness () { - [[ "${MAP_TESTS}" != "true" ]] && return + [[ "${MAP_TESTS:-false}" != "true" ]] && return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/ibm-fusion-access/configure-kmm-registry/ibm-fusion-access-configure-kmm-registry-commands.sh` at line 49, Update the conditional that gates the step to use a defensive default for the MAP_TESTS environment variable: change the test from using "${MAP_TESTS}" to the parameter-expansion form "${MAP_TESTS:-false}" so the guard becomes [[ "${MAP_TESTS:-false}" != "true" ]] (refer to the MAP_TESTS variable in the ibm-fusion-access-configure-kmm-registry step script where the current check is written).ci-operator/step-registry/interop-tests/ibm-fusion-access/verify-cluster/interop-tests-ibm-fusion-access-verify-cluster-commands.sh (2)
15-36: Inconsistent with sibling refactor: no XML escaping forname/classname/message.
ci-operator/step-registry/interop-tests/ibm-fusion-access/test-vm-snapshots/interop-tests-ibm-fusion-access-test-vm-snapshots-commands.shadds anEscapeXmlhelper and escapes every interpolated field before composing the testsuite XML; this verify-cluster version drops that helper even thoughtestMessageis partially built fromoc/jqoutput (e.g.,successStatussourced from.status.conditions[].status). The risk is low today (condition statuses are enum-like), but the inconsistency means a future contributor adding a free-form message could silently produce invalid XML that breaks ReportPortal ingestion. Align with the other scripts.♻️ Proposed change (add helper and use it)
+function EscapeXml () { + typeset text="${1}"; (($#)) && shift + printf '%s' "${text}" | sed 's/&/\&/g; s/</\</g; s/>/\>/g; s/"/\"/g; s/'\''/\'/g' + true +} + function AddTestResult () { typeset testName="${1}"; (($#)) && shift typeset testStatus="${1}"; (($#)) && shift typeset testDuration="${1}"; (($#)) && shift typeset testMessage="${1:-}"; (($#)) && shift typeset testClassName="${1:-VerifyClusterTests}"; (($#)) && shift - + + typeset escapedName escapedClassName escapedMessage + escapedName="$(EscapeXml "${testName}")" + escapedClassName="$(EscapeXml "${testClassName}")" + escapedMessage="$(EscapeXml "${testMessage}")" + ((++testsTotal)) - + if [[ "${testStatus}" == "passed" ]]; then testCases="${testCases} - <testcase name=\"${testName}\" classname=\"${testClassName}\" time=\"${testDuration}\"/>" + <testcase name=\"${escapedName}\" classname=\"${escapedClassName}\" time=\"${testDuration}\"/>" else ((++testsFailed)) testCases="${testCases} - <testcase name=\"${testName}\" classname=\"${testClassName}\" time=\"${testDuration}\"> - <failure message=\"Test failed\">${testMessage}</failure> + <testcase name=\"${escapedName}\" classname=\"${escapedClassName}\" time=\"${testDuration}\"> + <failure message=\"Test failed\">${escapedMessage}</failure> </testcase>" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/interop-tests/ibm-fusion-access/verify-cluster/interop-tests-ibm-fusion-access-verify-cluster-commands.sh` around lines 15 - 36, The AddTestResult function builds JUnit XML without escaping interpolated fields (name, classname, time, and failure message), which can produce invalid XML; add the same EscapeXml helper used in interop-tests-ibm-fusion-access-test-vm-snapshots-commands.sh and call it to sanitize testName, testClassName, testDuration (if needed) and testMessage before composing testCases in AddTestResult so all interpolated values are passed through EscapeXml (use the exact helper name EscapeXml and the function AddTestResult to locate the insertion points).
79-81: GuardSHARED_DIRunderset -u.Script runs with
set -eux, so an unsetSHARED_DIRaborts here before the JUnit result is copied — defeating the point of the EXIT trap for any caller that doesn't populate the variable. Default it.♻️ Proposed change
- if [[ -n "${SHARED_DIR}" ]] && [[ -d "${SHARED_DIR}" ]]; then + if [[ -n "${SHARED_DIR:-}" ]] && [[ -d "${SHARED_DIR}" ]]; then cp "${junitResultsFile}" "${SHARED_DIR}/$(basename "${junitResultsFile}")" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/interop-tests/ibm-fusion-access/verify-cluster/interop-tests-ibm-fusion-access-verify-cluster-commands.sh` around lines 79 - 81, The script fails under set -u if SHARED_DIR is unset; ensure SHARED_DIR is defaulted before use (e.g., set an empty default with parameter expansion) so the conditional copy block referencing SHARED_DIR and junitResultsFile won't abort the script; update the script to initialize SHARED_DIR (referencing SHARED_DIR and the cp block that copies "${junitResultsFile}" to "${SHARED_DIR}/$(basename "${junitResultsFile}")") so the guard [[ -n "${SHARED_DIR}" ]] && [[ -d "${SHARED_DIR}" ]] works safely under set -u.ci-operator/step-registry/interop-tests/ibm-fusion-access/test-vm-snapshots/interop-tests-ibm-fusion-access-test-vm-snapshots-commands.sh (2)
100-116: Pipeline is heavier than needed for a fully static manifest.The VolumeSnapshotClass heredoc has no templated fields, so the
oc create --dry-run=client -o json --save-config | yq -p json -o yaml | oc apply -f -round-trip doesn't add value beyond whatoc apply -f -on the raw heredoc would do. The otherjq-templated blocks (DataVolume/VM/VMSnapshot/VMRestore) legitimately need the pipeline; this one can be simplified to reduce cognitive load.♻️ Proposed change
- if ! ( - { - oc create -f - --dry-run=client -o json --save-config | - yq -p json -o yaml eval . | - oc apply -f - - } 0<<'YAML' -apiVersion: snapshot.storage.k8s.io/v1 -kind: VolumeSnapshotClass -metadata: - name: ibm-spectrum-scale-snapshotclass -driver: spectrumscale.csi.ibm.com -deletionPolicy: Delete -YAML - ); then + if ! oc apply -f - <<'YAML'; then +apiVersion: snapshot.storage.k8s.io/v1 +kind: VolumeSnapshotClass +metadata: + name: ibm-spectrum-scale-snapshotclass +driver: spectrumscale.csi.ibm.com +deletionPolicy: Delete +YAML oc get volumesnapshotclass -o yaml --ignore-not-found true fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/interop-tests/ibm-fusion-access/test-vm-snapshots/interop-tests-ibm-fusion-access-test-vm-snapshots-commands.sh` around lines 100 - 116, The VolumeSnapshotClass creation block is round-tripping through oc create|yq|oc apply despite being a static heredoc; simplify the logic by replacing the pipeline (the oc create -f - --dry-run=client -o json --save-config | yq -p json -o yaml eval . | oc apply -f -) with a direct oc apply -f - against the static VolumeSnapshotClass heredoc (the ibm-spectrum-scale-snapshotclass block) while preserving the surrounding conditional and the fallback oc get volumesnapshotclass --ignore-not-found behavior.
63-65: Defensive expansion forSHARED_DIRunderset -u.With
set -eux, referencing an unsetSHARED_DIRaborts the script before the trap-based JUnit copy can help. OpenShift CI does normally set it, but defaulting keeps the script robust when invoked locally or in a minimal harness.♻️ Proposed change
- if [[ -d "${SHARED_DIR}" ]]; then - cp "${junitResultsFile}" "${SHARED_DIR}/junit_vm_snapshots_tests.xml" + if [[ -n "${SHARED_DIR:-}" && -d "${SHARED_DIR}" ]]; then + cp "${junitResultsFile}" "${SHARED_DIR}/junit_vm_snapshots_tests.xml" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/interop-tests/ibm-fusion-access/test-vm-snapshots/interop-tests-ibm-fusion-access-test-vm-snapshots-commands.sh` around lines 63 - 65, The test script currently references SHARED_DIR under set -u which will abort if SHARED_DIR is unset; update the script to defensively expand or initialize SHARED_DIR before use so the cp in the trap can run safely. Specifically, either initialize SHARED_DIR="${SHARED_DIR:-}" near the top (before the trap and before any -d checks) or use a defensive expansion like "${SHARED_DIR:-}" in the conditional that checks [[ -d "${SHARED_DIR}" ]] and in the cp that copies junitResultsFile so the variable being unset won’t cause the script to exit; ensure references to SHARED_DIR and junitResultsFile in the trap/junit copy logic are updated consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 120ad8dc-0967-45eb-a049-b100d62170d0
📒 Files selected for processing (100)
ci-operator/config/RedHatQE/firewatch/RedHatQE-firewatch-main__slack-webhook-test.yamlci-operator/config/RedHatQE/interop-testing/.config.prowgenci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__ibm-fusion-access-operator-ocp-4.21-lp-interop-cr.yamlci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__ibm-fusion-access-operator-ocp-4.22-lp-interop.yamlci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__ibm-fusion-access-operator-ocp-4.23-lp-interop.yamlci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__ibm-fusion-access-operator-ocp4.20-lp-interop-cr.yamlci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__ibm-fusion-access-operator-ocp4.20-lp-interop.yamlci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__ibm-fusion-access-operator-ocp4.21-lp-interop.yamlci-operator/jobs/RedHatQE/interop-testing/RedHatQE-interop-testing-master-periodics.yamlci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.shci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-ref.yamlci-operator/step-registry/ibm-fusion-access/check-crds/ibm-fusion-access-check-crds-commands.shci-operator/step-registry/ibm-fusion-access/check-crds/ibm-fusion-access-check-crds-ref.yamlci-operator/step-registry/ibm-fusion-access/check-nodes/ibm-fusion-access-check-nodes-commands.shci-operator/step-registry/ibm-fusion-access/check-nodes/ibm-fusion-access-check-nodes-ref.yamlci-operator/step-registry/ibm-fusion-access/configure-aws-security-groups/ibm-fusion-access-configure-aws-security-groups-commands.shci-operator/step-registry/ibm-fusion-access/configure-aws-security-groups/ibm-fusion-access-configure-aws-security-groups-ref.yamlci-operator/step-registry/ibm-fusion-access/configure-kmm-registry/ibm-fusion-access-configure-kmm-registry-commands.shci-operator/step-registry/ibm-fusion-access/configure-kmm-registry/ibm-fusion-access-configure-kmm-registry-ref.yamlci-operator/step-registry/ibm-fusion-access/create-cluster/ibm-fusion-access-create-cluster-commands.shci-operator/step-registry/ibm-fusion-access/create-cluster/ibm-fusion-access-create-cluster-ref.yamlci-operator/step-registry/ibm-fusion-access/create-ebs-filesystem/ibm-fusion-access-create-ebs-filesystem-commands.shci-operator/step-registry/ibm-fusion-access/create-ebs-filesystem/ibm-fusion-access-create-ebs-filesystem-ref.yamlci-operator/step-registry/ibm-fusion-access/create-local-disks/ibm-fusion-access-create-local-disks-commands.shci-operator/step-registry/ibm-fusion-access/create-local-disks/ibm-fusion-access-create-local-disks-ref.yamlci-operator/step-registry/ibm-fusion-access/install-fusion-access-operator/ibm-fusion-access-install-fusion-access-operator-commands.shci-operator/step-registry/ibm-fusion-access/patch-buildgpl/ibm-fusion-access-patch-buildgpl-ref.yamlci-operator/step-registry/ibm-fusion-access/prepare-nodes/ibm-fusion-access-prepare-nodes-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/OWNERSci-operator/step-registry/interop-tests/ibm-fusion-access/check-crds/OWNERSci-operator/step-registry/interop-tests/ibm-fusion-access/check-crds/interop-tests-ibm-fusion-access-check-crds-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/check-crds/interop-tests-ibm-fusion-access-check-crds-ref.metadata.jsonci-operator/step-registry/interop-tests/ibm-fusion-access/check-crds/interop-tests-ibm-fusion-access-check-crds-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/check-nodes/OWNERSci-operator/step-registry/interop-tests/ibm-fusion-access/check-nodes/interop-tests-ibm-fusion-access-check-nodes-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/check-nodes/interop-tests-ibm-fusion-access-check-nodes-ref.metadata.jsonci-operator/step-registry/interop-tests/ibm-fusion-access/check-nodes/interop-tests-ibm-fusion-access-check-nodes-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/configure-aws-security-groups/OWNERSci-operator/step-registry/interop-tests/ibm-fusion-access/configure-aws-security-groups/interop-tests-ibm-fusion-access-configure-aws-security-groups-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/configure-aws-security-groups/interop-tests-ibm-fusion-access-configure-aws-security-groups-ref.metadata.jsonci-operator/step-registry/interop-tests/ibm-fusion-access/configure-aws-security-groups/interop-tests-ibm-fusion-access-configure-aws-security-groups-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/configure-cnv-shared-storage/interop-tests-ibm-fusion-access-configure-cnv-shared-storage-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/configure-cnv-shared-storage/interop-tests-ibm-fusion-access-configure-cnv-shared-storage-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/configure-kmm-registry/OWNERSci-operator/step-registry/interop-tests/ibm-fusion-access/configure-kmm-registry/interop-tests-ibm-fusion-access-configure-kmm-registry-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/configure-kmm-registry/interop-tests-ibm-fusion-access-configure-kmm-registry-ref.metadata.jsonci-operator/step-registry/interop-tests/ibm-fusion-access/configure-kmm-registry/interop-tests-ibm-fusion-access-configure-kmm-registry-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/create-cluster/OWNERSci-operator/step-registry/interop-tests/ibm-fusion-access/create-cluster/interop-tests-ibm-fusion-access-create-cluster-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/create-cluster/interop-tests-ibm-fusion-access-create-cluster-ref.metadata.jsonci-operator/step-registry/interop-tests/ibm-fusion-access/create-cluster/interop-tests-ibm-fusion-access-create-cluster-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/create-ebs-filesystem/OWNERSci-operator/step-registry/interop-tests/ibm-fusion-access/create-ebs-filesystem/interop-tests-ibm-fusion-access-create-ebs-filesystem-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/create-ebs-filesystem/interop-tests-ibm-fusion-access-create-ebs-filesystem-ref.metadata.jsonci-operator/step-registry/interop-tests/ibm-fusion-access/create-ebs-filesystem/interop-tests-ibm-fusion-access-create-ebs-filesystem-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/create-fusionaccess-resource/OWNERSci-operator/step-registry/interop-tests/ibm-fusion-access/create-fusionaccess-resource/interop-tests-ibm-fusion-access-create-fusionaccess-resource-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/create-fusionaccess-resource/interop-tests-ibm-fusion-access-create-fusionaccess-resource-ref.metadata.jsonci-operator/step-registry/interop-tests/ibm-fusion-access/create-fusionaccess-resource/interop-tests-ibm-fusion-access-create-fusionaccess-resource-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/create-local-disks/OWNERSci-operator/step-registry/interop-tests/ibm-fusion-access/create-local-disks/interop-tests-ibm-fusion-access-create-local-disks-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/create-local-disks/interop-tests-ibm-fusion-access-create-local-disks-ref.metadata.jsonci-operator/step-registry/interop-tests/ibm-fusion-access/create-local-disks/interop-tests-ibm-fusion-access-create-local-disks-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/create-namespaces/interop-tests-ibm-fusion-access-create-namespaces-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/create-namespaces/interop-tests-ibm-fusion-access-create-namespaces-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/create-pull-secrets/OWNERSci-operator/step-registry/interop-tests/ibm-fusion-access/create-pull-secrets/interop-tests-ibm-fusion-access-create-pull-secrets-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/create-pull-secrets/interop-tests-ibm-fusion-access-create-pull-secrets-ref.metadata.jsonci-operator/step-registry/interop-tests/ibm-fusion-access/create-pull-secrets/interop-tests-ibm-fusion-access-create-pull-secrets-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/custom-ibm-must-gather/OWNERSci-operator/step-registry/interop-tests/ibm-fusion-access/custom-ibm-must-gather/interop-tests-ibm-fusion-access-custom-ibm-must-gather-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/custom-ibm-must-gather/interop-tests-ibm-fusion-access-custom-ibm-must-gather-ref.metadata.jsonci-operator/step-registry/interop-tests/ibm-fusion-access/custom-ibm-must-gather/interop-tests-ibm-fusion-access-custom-ibm-must-gather-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/ebs-integration-chain/interop-tests-ibm-fusion-access-ebs-integration-chain-chain.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/environment-setup-chain/interop-tests-ibm-fusion-access-environment-setup-chain-chain.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/install-fusion-access-operator/OWNERSci-operator/step-registry/interop-tests/ibm-fusion-access/install-fusion-access-operator/interop-tests-ibm-fusion-access-install-fusion-access-operator-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/install-fusion-access-operator/interop-tests-ibm-fusion-access-install-fusion-access-operator-ref.metadata.jsonci-operator/step-registry/interop-tests/ibm-fusion-access/install-fusion-access-operator/interop-tests-ibm-fusion-access-install-fusion-access-operator-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/label-nodes/interop-tests-ibm-fusion-access-label-nodes-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/patch-buildgpl/OWNERSci-operator/step-registry/interop-tests/ibm-fusion-access/patch-buildgpl/interop-tests-ibm-fusion-access-patch-buildgpl-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/patch-buildgpl/interop-tests-ibm-fusion-access-patch-buildgpl-ref.metadata.jsonci-operator/step-registry/interop-tests/ibm-fusion-access/patch-buildgpl/interop-tests-ibm-fusion-access-patch-buildgpl-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/prepare-lxtrace-files/interop-tests-ibm-fusion-access-prepare-lxtrace-files-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/prepare-lxtrace-files/interop-tests-ibm-fusion-access-prepare-lxtrace-files-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/prepare-worker-nodes/interop-tests-ibm-fusion-access-prepare-worker-nodes-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/prepare-worker-nodes/interop-tests-ibm-fusion-access-prepare-worker-nodes-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/test-cnv-shared-storage/interop-tests-ibm-fusion-access-test-cnv-shared-storage-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/test-cnv-shared-storage/interop-tests-ibm-fusion-access-test-cnv-shared-storage-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/test-vm-lifecycle/interop-tests-ibm-fusion-access-test-vm-lifecycle-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/test-vm-lifecycle/interop-tests-ibm-fusion-access-test-vm-lifecycle-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/test-vm-migration/interop-tests-ibm-fusion-access-test-vm-migration-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/test-vm-migration/interop-tests-ibm-fusion-access-test-vm-migration-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/test-vm-snapshots/interop-tests-ibm-fusion-access-test-vm-snapshots-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/test-vm-snapshots/interop-tests-ibm-fusion-access-test-vm-snapshots-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/verify-cluster/interop-tests-ibm-fusion-access-verify-cluster-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/verify-cluster/interop-tests-ibm-fusion-access-verify-cluster-ref.yamlci-operator/step-registry/interop-tests/ibm-fusion-access/verify-shared-storage/interop-tests-ibm-fusion-access-verify-shared-storage-commands.shci-operator/step-registry/interop-tests/ibm-fusion-access/verify-shared-storage/interop-tests-ibm-fusion-access-verify-shared-storage-ref.yaml
💤 Files with no reviewable changes (48)
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-ebs-filesystem/OWNERS
- ci-operator/step-registry/interop-tests/ibm-fusion-access/check-nodes/OWNERS
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-local-disks/OWNERS
- ci-operator/step-registry/interop-tests/ibm-fusion-access/configure-kmm-registry/OWNERS
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-pull-secrets/OWNERS
- ci-operator/step-registry/interop-tests/ibm-fusion-access/custom-ibm-must-gather/OWNERS
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-fusionaccess-resource/interop-tests-ibm-fusion-access-create-fusionaccess-resource-ref.metadata.json
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-fusionaccess-resource/OWNERS
- ci-operator/step-registry/interop-tests/ibm-fusion-access/patch-buildgpl/interop-tests-ibm-fusion-access-patch-buildgpl-ref.metadata.json
- ci-operator/step-registry/interop-tests/ibm-fusion-access/configure-aws-security-groups/interop-tests-ibm-fusion-access-configure-aws-security-groups-ref.metadata.json
- ci-operator/step-registry/interop-tests/ibm-fusion-access/check-crds/OWNERS
- ci-operator/step-registry/interop-tests/ibm-fusion-access/install-fusion-access-operator/OWNERS
- ci-operator/step-registry/interop-tests/ibm-fusion-access/install-fusion-access-operator/interop-tests-ibm-fusion-access-install-fusion-access-operator-ref.metadata.json
- ci-operator/step-registry/interop-tests/ibm-fusion-access/check-crds/interop-tests-ibm-fusion-access-check-crds-ref.metadata.json
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-cluster/interop-tests-ibm-fusion-access-create-cluster-ref.metadata.json
- ci-operator/step-registry/interop-tests/ibm-fusion-access/configure-aws-security-groups/OWNERS
- ci-operator/step-registry/interop-tests/ibm-fusion-access/check-nodes/interop-tests-ibm-fusion-access-check-nodes-ref.metadata.json
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-pull-secrets/interop-tests-ibm-fusion-access-create-pull-secrets-ref.metadata.json
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-ebs-filesystem/interop-tests-ibm-fusion-access-create-ebs-filesystem-commands.sh
- ci-operator/step-registry/interop-tests/ibm-fusion-access/custom-ibm-must-gather/interop-tests-ibm-fusion-access-custom-ibm-must-gather-ref.metadata.json
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-ebs-filesystem/interop-tests-ibm-fusion-access-create-ebs-filesystem-ref.metadata.json
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-fusionaccess-resource/interop-tests-ibm-fusion-access-create-fusionaccess-resource-ref.yaml
- ci-operator/step-registry/interop-tests/ibm-fusion-access/configure-aws-security-groups/interop-tests-ibm-fusion-access-configure-aws-security-groups-commands.sh
- ci-operator/step-registry/interop-tests/ibm-fusion-access/patch-buildgpl/OWNERS
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-cluster/OWNERS
- ci-operator/step-registry/interop-tests/ibm-fusion-access/install-fusion-access-operator/interop-tests-ibm-fusion-access-install-fusion-access-operator-ref.yaml
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-local-disks/interop-tests-ibm-fusion-access-create-local-disks-ref.yaml
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-cluster/interop-tests-ibm-fusion-access-create-cluster-ref.yaml
- ci-operator/step-registry/interop-tests/ibm-fusion-access/configure-kmm-registry/interop-tests-ibm-fusion-access-configure-kmm-registry-ref.yaml
- ci-operator/step-registry/interop-tests/ibm-fusion-access/install-fusion-access-operator/interop-tests-ibm-fusion-access-install-fusion-access-operator-commands.sh
- ci-operator/step-registry/interop-tests/ibm-fusion-access/check-nodes/interop-tests-ibm-fusion-access-check-nodes-commands.sh
- ci-operator/step-registry/interop-tests/ibm-fusion-access/configure-aws-security-groups/interop-tests-ibm-fusion-access-configure-aws-security-groups-ref.yaml
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-fusionaccess-resource/interop-tests-ibm-fusion-access-create-fusionaccess-resource-commands.sh
- ci-operator/step-registry/interop-tests/ibm-fusion-access/check-crds/interop-tests-ibm-fusion-access-check-crds-commands.sh
- ci-operator/step-registry/interop-tests/ibm-fusion-access/custom-ibm-must-gather/interop-tests-ibm-fusion-access-custom-ibm-must-gather-commands.sh
- ci-operator/step-registry/interop-tests/ibm-fusion-access/check-crds/interop-tests-ibm-fusion-access-check-crds-ref.yaml
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-pull-secrets/interop-tests-ibm-fusion-access-create-pull-secrets-ref.yaml
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-local-disks/interop-tests-ibm-fusion-access-create-local-disks-ref.metadata.json
- ci-operator/step-registry/interop-tests/ibm-fusion-access/custom-ibm-must-gather/interop-tests-ibm-fusion-access-custom-ibm-must-gather-ref.yaml
- ci-operator/step-registry/interop-tests/ibm-fusion-access/patch-buildgpl/interop-tests-ibm-fusion-access-patch-buildgpl-commands.sh
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-ebs-filesystem/interop-tests-ibm-fusion-access-create-ebs-filesystem-ref.yaml
- ci-operator/step-registry/interop-tests/ibm-fusion-access/configure-kmm-registry/interop-tests-ibm-fusion-access-configure-kmm-registry-commands.sh
- ci-operator/step-registry/interop-tests/ibm-fusion-access/configure-kmm-registry/interop-tests-ibm-fusion-access-configure-kmm-registry-ref.metadata.json
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-pull-secrets/interop-tests-ibm-fusion-access-create-pull-secrets-commands.sh
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-cluster/interop-tests-ibm-fusion-access-create-cluster-commands.sh
- ci-operator/step-registry/interop-tests/ibm-fusion-access/check-nodes/interop-tests-ibm-fusion-access-check-nodes-ref.yaml
- ci-operator/step-registry/interop-tests/ibm-fusion-access/create-local-disks/interop-tests-ibm-fusion-access-create-local-disks-commands.sh
- ci-operator/step-registry/interop-tests/ibm-fusion-access/patch-buildgpl/interop-tests-ibm-fusion-access-patch-buildgpl-ref.yaml
| memory: 400Mi | ||
| tests: | ||
| - as: ibm-fusion-access-operator-cr-ocp421 | ||
| - as: cr-fusion-access-aws |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List Fusion Access interop variant filenames and their declared variants to confirm the inconsistency.
fd -t f 'ibm-fusion-access-operator' ci-operator/config/RedHatQE/interop-testing | sort
echo "---"
rg -nP --type=yaml '^\s*variant:\s*ibm-fusion-access-operator' ci-operator/config/RedHatQE/interop-testing
echo "---"
# Also check any references to these variants in periodic/prow jobs
rg -nP 'ibm-fusion-access-operator-ocp-?4\.(20|21)-lp-interop(-cr)?' ci-operator/jobs/RedHatQE/interop-testingRepository: openshift/release
Length of output: 8547
Variant naming is inconsistent within the 4.21 family and across CR variants.
This file declares ibm-fusion-access-operator-ocp-4.21-lp-interop-cr (with dash), but the non-CR 4.21 variant uses ibm-fusion-access-operator-ocp4.21-lp-interop (no dash), and the 4.20 CR variant uses ibm-fusion-access-operator-ocp4.20-lp-interop-cr (no dash). This produces variant-name mismatches in Prow job definitions (e.g., periodic-ci-...-ocp-4.21-lp-interop-cr-... vs. periodic-ci-...-ocp4.21-lp-interop-...), making periodic/cron job references harder to grep and maintain. Adopt a consistent naming convention across all four variants (4.20 CR, 4.21 CR, 4.21 non-CR, 4.20 non-CR) and update their corresponding Prow job definitions in presubmits and periodics accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__ibm-fusion-access-operator-ocp-4.21-lp-interop-cr.yaml`
at line 36, The variant name in this PR is inconsistent: rename the CR variant
"ibm-fusion-access-operator-ocp-4.21-lp-interop-cr" to match the established
convention used by the other variants (choose either the dashed form
"ocp-4.21-..." or the compact form "ocp4.21-...") and update all references
accordingly; specifically update the declaration that currently reads "as:
cr-fusion-access-aws" entry to use the chosen canonical variant name and then
update corresponding Prow job definitions (presubmits and periodics) that
reference "ibm-fusion-access-operator-ocp4.21-lp-interop",
"ibm-fusion-access-operator-ocp4.20-lp-interop-cr" and any occurrences of
"ibm-fusion-access-operator-ocp-4.21-lp-interop-cr" so all four variants (4.20
CR, 4.21 CR, 4.21 non-CR, 4.20 non-CR) follow the same naming convention.
| images: | ||
| - dockerfile_literal: | | ||
| FROM icr.io/cpopen/ibm-spectrum-scale-must-gather:v5.2.3.1 | ||
| FROM icr.io/cpopen/ibm-spectrum-scale-must-gather:v5.2.3.5 | ||
| to: ibm-must-gather |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect step scripts relying on jq and compare cli image assembly across variants.
rg -nP --type=sh '\bjq\b' ci-operator/step-registry/interop-tests/ibm-fusion-access ci-operator/step-registry/ibm-fusion-access | head -n 50
echo "--- 4.20 vs 4.21 CR images blocks ---"
for f in \
ci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__ibm-fusion-access-operator-ocp4.20-lp-interop-cr.yaml \
ci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__ibm-fusion-access-operator-ocp-4.21-lp-interop-cr.yaml \
ci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__ibm-fusion-access-operator-ocp4.20-lp-interop.yaml \
ci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__ibm-fusion-access-operator-ocp4.21-lp-interop.yaml; do
echo "== $f =="
awk '/^base_images:/{p=1} /^images:/{p=1} /^releases:/{p=0} p' "$f"
doneRepository: openshift/release
Length of output: 12603
🏁 Script executed:
# Search for references to ocp/4.20:cli or comments about jq availability in configs
rg -A 5 -B 5 'ocp.*4\.20.*cli|jq.*4\.20|4\.20.*jq' ci-operator/config/RedHatQE/interop-testing/ --type yaml
# Check if there are base ci-operator configurations or step setup that installs jq
rg 'jq' ci-operator/step-registry/ --type sh | grep -E '(install|setup|RUN|apt|dnf|yum)' | head -20
# Look for any infrastructure code that might install jq globally
fd -e yaml -e sh ci-operator/ | xargs grep -l 'install.*jq\|jq.*install' 2>/dev/null | head -20Repository: openshift/release
Length of output: 3689
🏁 Script executed:
# Search for any comments or documentation about ocp base image versions and jq
rg -i 'ocp.*cli.*jq|cli.*jq|base.*jq|4\.20|4\.21' ci-operator/config/RedHatQE/interop-testing/ -A 3 -B 3
# Check if there are any references to base image composition or what's included in ocp/X.XX:cli
rg 'FROM.*ocp|base.*image|cli.*base' ci-operator/config/RedHatQE/interop-testing/ --type yaml
# Look for similar patterns in other interop configs to see if they handle jq differently
find ci-operator/config -name '*interop*.yaml' -exec grep -l 'jq\|cli' {} \; | head -10 | xargs -I {} bash -c 'echo "== {} =="; grep -A 10 "^images:" {}'Repository: openshift/release
Length of output: 59310
Add jq install layer to 4.20 configs for parity with 4.21 variants and to ensure Fusion Access test steps execute successfully.
The 4.21 CR and non-CR configs both explicitly rebuild the cli image to install jq via RUN dnf install -y --setopt=tsflags=nodocs jq && dnf clean all, because the Fusion Access interop test step scripts rely on jq throughout (verify-cluster, test-vm-snapshots, test-vm-migration, etc. all contain 40+ jq invocations). The 4.20 CR and 4.20 non-CR configs skip this rebuild and use the base ocp/4.20:cli image directly. If the base 4.20 image does not include jq, the test steps will fail immediately on the first jq invocation. Apply the same jq install RUN layer to both RedHatQE-interop-testing-master__ibm-fusion-access-operator-ocp4.20-lp-interop-cr.yaml and RedHatQE-interop-testing-master__ibm-fusion-access-operator-ocp4.20-lp-interop.yaml to match the 4.21 pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ci-operator/config/RedHatQE/interop-testing/RedHatQE-interop-testing-master__ibm-fusion-access-operator-ocp4.20-lp-interop-cr.yaml`
around lines 15 - 18, The images block uses a dockerfile_literal for the cli
image (currently FROM icr.io/cpopen/ibm-spectrum-scale-must-gather:v5.2.3.5 /
to: ibm-must-gather) but does not add jq; update the dockerfile_literal for the
ocp/4.20 cli rebuild to include the same RUN layer used in 4.21 (RUN dnf install
-y --setopt=tsflags=nodocs jq && dnf clean all) so the rebuilt cli image
contains jq; apply the identical change to both the CR and non-CR 4.20 configs
to match the 4.21 pattern.
| reporter_config: | ||
| slack: | ||
| channel: '#forum-p2p-cspi' | ||
| job_states_to_report: | ||
| - success | ||
| - failure | ||
| - error | ||
| report_template: '{{if eq .Status.State "success"}} :slack-green: Job *{{.Spec.Job}}* | ||
| ended with *{{.Status.State}}*. <{{.Status.URL}}|View logs> {{else}} :failed: | ||
| Job *{{.Spec.Job}}* ended with *{{.Status.State}}*. <{{.Status.URL}}|View | ||
| logs> {{end}}' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f '.config.prowgen' -p interop-testing --exec cat {}Repository: openshift/release
Length of output: 164
🏁 Script executed:
fd -t f '\.config\.prowgen' ci-operator/jobs/RedHatQE/interop-testing/ --exec cat {}Repository: openshift/release
Length of output: 43
🏁 Script executed:
# Also search for any .config.prowgen files in the repository to understand the structure
fd -t f '\.config\.prowgen' --max-results 10Repository: openshift/release
Length of output: 43
🏁 Script executed:
# Look at the YAML file mentioned in the review to understand context
head -n 2000 ci-operator/jobs/RedHatQE/interop-testing/RedHatQE-interop-testing-master-periodics.yaml | tail -n 100Repository: openshift/release
Length of output: 3223
🏁 Script executed:
# List files in the interop-testing directory
ls -la ci-operator/jobs/RedHatQE/interop-testing/Repository: openshift/release
Length of output: 627
🏁 Script executed:
# Search for prowgen related files
find ci-operator -name '*prowgen*' -o -name '*.config'Repository: openshift/release
Length of output: 12994
🏁 Script executed:
# Search for references to cr-fusion in the config
rg 'cr-fusion' ci-operator/jobs/RedHatQE/interop-testing/ -A 2 -B 2Repository: openshift/release
Length of output: 5468
🏁 Script executed:
# Search for ibm-fusion-access in the directory to understand config
rg 'ibm-fusion' ci-operator/jobs/RedHatQE/interop-testing/ --max-count 20Repository: openshift/release
Length of output: 7177
🏁 Script executed:
cat ci-operator/config/RedHatQE/interop-testing/.config.prowgenRepository: openshift/release
Length of output: 4515
Remove the Slack reporter block from the IPI job or update the prowgen configuration's job_names list.
The .config.prowgen file specifies that the #forum-p2p-cspi channel should only report on jobs matching the patterns lp-interop-aws and cr-fusion-access-aws. The job periodic-ci-RedHatQE-interop-testing-master-ibm-fusion-access-operator-ocp4.20-lp-interop-ibm-fusion-access-operator-ipi-ocp420 (which targets ibm-fusion-access-operator-ipi-ocp420) does not match either pattern and should not have the reporter configured here. Either remove this block or update the prowgen configuration if IPI variants are intended to be included.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ci-operator/jobs/RedHatQE/interop-testing/RedHatQE-interop-testing-master-periodics.yaml`
around lines 1937 - 1947, The Slack reporter block (reporter_config -> slack ->
channel '#forum-p2p-cspi') is applied to an IPI periodic job that doesn't match
the prowgen .config.prowgen job_names patterns; either remove this
reporter_config block from the IPI job definition for
periodic-ci-RedHatQE-interop-testing-master-ibm-fusion-access-operator-ocp4.20-lp-interop-ibm-fusion-access-operator-ipi-ocp420
(targeting ibm-fusion-access-operator-ipi-ocp420) or update the prowgen
configuration's job_names list to include patterns that match IPI variants
(e.g., add a pattern that matches "*-ipi-*" or the specific job name) so the
reporter is only generated for intended jobs.
| # The FusionAccess operator installs the IBM Storage Scale operator which creates these CRDs | ||
| if oc wait --for=condition=Established crd/clusters.scale.spectrum.ibm.com --timeout=600s; then | ||
| exit 0 | ||
| # Purpose: Check that expected IBM Fusion Access and Storage Scale CRDs exist on the cluster and write JUnit results for ReportPortal when MAP_TESTS is enabled. |
There was a problem hiding this comment.
Purpose comment is misleading — only Storage Scale CRD is checked.
The comment claims this step checks "IBM Fusion Access and Storage Scale CRDs", but the only oc wait at Line 89 targets crd/clusters.scale.spectrum.ibm.com (Storage Scale) and the testcase is named test_storage_scale_crds_established. Either drop the "Fusion Access" mention or add the corresponding oc wait for the Fusion Access CRD(s).
📝 Proposed doc fix
-# Purpose: Check that expected IBM Fusion Access and Storage Scale CRDs exist on the cluster and write JUnit results for ReportPortal when MAP_TESTS is enabled.
+# Purpose: Check that the expected IBM Storage Scale CRD is established on the cluster and write JUnit results for ReportPortal when MAP_TESTS is enabled.📝 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.
| # Purpose: Check that expected IBM Fusion Access and Storage Scale CRDs exist on the cluster and write JUnit results for ReportPortal when MAP_TESTS is enabled. | |
| # Purpose: Check that the expected IBM Storage Scale CRD is established on the cluster and write JUnit results for ReportPortal when MAP_TESTS is enabled. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ci-operator/step-registry/ibm-fusion-access/check-crds/ibm-fusion-access-check-crds-commands.sh`
at line 4, The header comment "Purpose: Check that expected IBM Fusion Access
and Storage Scale CRDs..." is inaccurate because the script only waits for
crd/clusters.scale.spectrum.ibm.com (Storage Scale) and the testcase is named
test_storage_scale_crds_established; either remove the "IBM Fusion Access"
mention from the Purpose line (and keep testcase/name as-is) or add the missing
oc wait(s) for the Fusion Access CRD(s) (and update the testcase name to reflect
both checks) — locate the oc wait invocation for
crd/clusters.scale.spectrum.ibm.com and the testcase identifier
test_storage_scale_crds_established to apply the change.
| RecordTest "testsTotal" "testsFailed" "testCases" "${testStart}" "fa_cnv_1011_prerequisite_start_vm" "${testStatus}" "${testMessage}" | ||
|
|
||
| # If VM didn't start, we can't continue with remaining tests | ||
| if [[ "$test_status" != "passed" ]]; then | ||
| echo "" | ||
| echo "❌ VM failed to start - cannot continue with lifecycle tests" | ||
| if [[ "${testStatus}" != "passed" ]]; then | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Tests 2 and 3 are silently dropped from the JUnit report when Test 1 fails.
exit 1 on line 236 short-circuits before Tests 2/3 are recorded, so the generated junit_vm_lifecycle_tests.xml only contains fa_cnv_1011_prerequisite_start_vm. The actual target cases (FA-CNV-1011 stop, FA-CNV-1012 restart) don't appear at all — not even as <skipped/> — which makes ReportPortal / JUnit consumers think those cases were never intended to run for this build, rather than skipped due to a failed prerequisite. Consider recording explicit skipped entries (or a dedicated helper) before exiting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ci-operator/step-registry/interop-tests/ibm-fusion-access/test-vm-lifecycle/interop-tests-ibm-fusion-access-test-vm-lifecycle-commands.sh`
around lines 232 - 237, When the prerequisite test
fa_cnv_1011_prerequisite_start_vm fails, we currently exit before recording the
remaining cases; update the failure branch (where RecordTest is called and
testStatus is checked) to call RecordTest for the remaining test cases (e.g.,
"FA-CNV-1011 stop", "FA-CNV-1012 restart") with a status of "skipped" and a
message like "skipped due to failed prerequisite: ${testMessage}" (use the same
RecordTest signature and variables testStatus/testMessage) so
junit_vm_lifecycle_tests.xml contains explicit skipped entries, then exit 1.
| - name: FA__CNV__NAMESPACE | ||
| documentation: The namespace for CNV | ||
| default: "openshift-cnv" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm FA__CNV__NAMESPACE usage across the ibm-fusion-access step scripts.
rg -nP '\bFA__CNV__NAMESPACE\b' ci-operator/step-registry/interop-tests/ibm-fusion-accessRepository: openshift/release
Length of output: 4450
🏁 Script executed:
#!/bin/bash
# Check the companion commands script for test-vm-lifecycle step
cat -n ci-operator/step-registry/interop-tests/ibm-fusion-access/test-vm-lifecycle/interop-tests-ibm-fusion-access-test-vm-lifecycle-commands.sh | head -50Repository: openshift/release
Length of output: 2257
🏁 Script executed:
#!/bin/bash
# Search for FA__CNV__NAMESPACE usage in test-vm-lifecycle-commands.sh
rg -n '\bFA__CNV__NAMESPACE\b' ci-operator/step-registry/interop-tests/ibm-fusion-access/test-vm-lifecycle/interop-tests-ibm-fusion-access-test-vm-lifecycle-commands.shRepository: openshift/release
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Read the test-vm-lifecycle-ref.yaml file
cat ci-operator/step-registry/interop-tests/ibm-fusion-access/test-vm-lifecycle/interop-tests-ibm-fusion-access-test-vm-lifecycle-ref.yamlRepository: openshift/release
Length of output: 1647
FA__CNV__NAMESPACE is unused by the test-vm-lifecycle companion script.
The test-vm-lifecycle-commands.sh header (line 5) documents inputs as FA__CNV__TEST_NAMESPACE, FA__CNV__SHARED_STORAGE_CLASS, FA__CNV__VM_NAME, FA__CNV__VM_MEMORY_REQUEST, FA__CNV__VM_CPU_REQUEST — FA__CNV__NAMESPACE is not referenced in the script. Either remove this variable from the ref or wire it into the script if the CNV operator namespace needs to be configurable for this test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ci-operator/step-registry/interop-tests/ibm-fusion-access/test-vm-lifecycle/interop-tests-ibm-fusion-access-test-vm-lifecycle-ref.yaml`
around lines 27 - 29, The CI ref defines FA__CNV__NAMESPACE but the test script
test-vm-lifecycle-commands.sh uses FA__CNV__TEST_NAMESPACE (and other variables)
and never references FA__CNV__NAMESPACE; either remove FA__CNV__NAMESPACE from
the ref or update the script to consume it—specifically, in the ref remove the
FA__CNV__NAMESPACE entry if the operator namespace is not needed, or modify
test-vm-lifecycle-commands.sh to accept FA__CNV__NAMESPACE (replace/alias
FA__CNV__TEST_NAMESPACE where appropriate) and use that variable in functions
that target the CNV operator namespace (e.g., any kubectl/oc commands or
namespace variables) so the config becomes effective.
| typeset -i snapshotClasses=0 | ||
| snapshotClasses=$( | ||
| oc get volumesnapshotclass \ | ||
| -o jsonpath-as-json='{.items[*].metadata.name}' | | ||
| jq 'length' | ||
| ) | ||
|
|
||
| if [[ "${snapshotClasses}" -eq 0 ]]; then | ||
| if ! ( | ||
| { | ||
| oc create -f - --dry-run=client -o json --save-config | | ||
| yq -p json -o yaml eval . | | ||
| oc apply -f - | ||
| } 0<<'YAML' | ||
| apiVersion: snapshot.storage.k8s.io/v1 | ||
| kind: VolumeSnapshotClass | ||
| metadata: | ||
| name: ibm-spectrum-scale-snapshotclass | ||
| driver: spectrumscale.csi.ibm.com | ||
| deletionPolicy: Delete | ||
| EOF | ||
| then | ||
| echo " ✅ VolumeSnapshotClass created" | ||
| else | ||
| echo " ⚠️ Failed to create VolumeSnapshotClass" | ||
| echo " Snapshot tests may fail without VolumeSnapshotClass" | ||
| YAML | ||
| ); then | ||
| oc get volumesnapshotclass -o yaml --ignore-not-found | ||
| true | ||
| fi | ||
| else | ||
| echo " ✅ VolumeSnapshotClass available" | ||
| echo " 📋 Available VolumeSnapshotClasses:" | ||
| oc get volumesnapshotclass -o custom-columns="NAME:.metadata.name,DRIVER:.driver,DELETIONPOLICY:.deletionPolicy" | ||
| fi |
There was a problem hiding this comment.
Existence check should filter by driver, not any VolumeSnapshotClass.
Counting every VolumeSnapshotClass causes the spectrumscale.csi.ibm.com class to be skipped whenever any snapshot class from another CSI driver (e.g., EBS, OCS) already exists on the cluster. Downstream VirtualMachineSnapshot creation at line 224 then has no class matching the Spectrum Scale PVC's provisioner, and the snapshot request will fail with an opaque error. Scope the check to the driver you intend to provision.
🛠️ Proposed fix
typeset -i snapshotClasses=0
snapshotClasses=$(
- oc get volumesnapshotclass \
- -o jsonpath-as-json='{.items[*].metadata.name}' |
- jq 'length'
+ oc get volumesnapshotclass -o json |
+ jq '[.items[]? | select(.driver=="spectrumscale.csi.ibm.com")] | length'
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ci-operator/step-registry/interop-tests/ibm-fusion-access/test-vm-snapshots/interop-tests-ibm-fusion-access-test-vm-snapshots-commands.sh`
around lines 92 - 119, The existence check currently counts all
VolumeSnapshotClass objects via snapshotClasses and may skip creating
ibm-spectrum-scale-snapshotclass if any unrelated class exists; change the check
to only count classes whose driver equals "spectrumscale.csi.ibm.com" (i.e.,
adjust the oc get volumesnapshotclass jsonpath/jq pipeline used to populate
snapshotClasses to filter by .driver == "spectrumscale.csi.ibm.com") so the
script will create the ibm-spectrum-scale-snapshotclass when no Spectrum Scale
driver class exists; update references around the snapshotClasses variable and
the oc get volumesnapshotclass command used to list existing classes to reflect
this driver-scoped filter.
| function InstallYQIfNotExists () { | ||
| if ! command -v yq >/dev/null; then | ||
| typeset arch='' | ||
| arch="$(uname -m | sed 's/aarch64/arm64/;s/x86_64/amd64/')" | ||
| mkdir -p /tmp/bin | ||
| export PATH="${PATH}:/tmp/bin/" | ||
| curl -L "https://github.com/mikefarah/yq/releases/latest/download/yq_linux_${arch}" \ | ||
| -o /tmp/bin/yq && chmod +x /tmp/bin/yq | ||
| fi | ||
|
|
||
| true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest stable release of mikefarah/yq and does it support the .+@nameXML attribute-set expression used inyq eval -px -ox?
💡 Result:
The latest stable release of mikefarah/yq is v4.53.2, released on 2026-04-17. Yes, it supports the .+@name XML attribute-set expression. The default --xml-attribute-prefix is now +@, so XML attributes are parsed into YAML fields like .+@name. This works with yq eval -px -ox (XML input and XML output), allowing access and manipulation of attributes using paths like .element.+@name.
Citations:
- 1: https://github.com/mikefarah/yq/releases
- 2: https://github.com/mikefarah/yq
- 3: https://github.com/mikefarah/yq/
- 4: https://pkg.go.dev/github.com/mikefarah/yq/v4
- 5: https://mikefarah.gitbook.io/yq/usage/xml
- 6: https://github.com/mikefarah/yq/blob/master/README.md
- 7: https://github.com/mikefarah/yq/releases/tag/v4.53.2
Pin yq version rather than /releases/latest.
Downloading releases/latest of mikefarah/yq means the component-readiness mapping step can silently change behavior (or break on XML-syntax changes) whenever upstream publishes a new release — an especially bad property for CI steps gated on a MAP_TESTS toggle that only runs on scheduled jobs. Pin a known-good version and bump deliberately.
♻️ Proposed change (pin and cache-friendly)
function InstallYQIfNotExists () {
if ! command -v yq >/dev/null; then
typeset arch=''
arch="$(uname -m | sed 's/aarch64/arm64/;s/x86_64/amd64/')"
+ typeset yqVersion="${YQ_VERSION:-v4.53.2}"
mkdir -p /tmp/bin
export PATH="${PATH}:/tmp/bin/"
- curl -L "https://github.com/mikefarah/yq/releases/latest/download/yq_linux_${arch}" \
+ curl -fsSL "https://github.com/mikefarah/yq/releases/download/${yqVersion}/yq_linux_${arch}" \
-o /tmp/bin/yq && chmod +x /tmp/bin/yq
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ci-operator/step-registry/interop-tests/ibm-fusion-access/verify-cluster/interop-tests-ibm-fusion-access-verify-cluster-commands.sh`
around lines 38 - 49, The InstallYQIfNotExists function currently downloads yq
from releases/latest which is unstable; change it to pin a specific yq release
by introducing a YQ_VERSION variable (e.g., default to a known-good tag) and use
that in the download URL instead of "latest"; also download to a versioned cache
path (e.g., /tmp/bin/yq-${YQ_VERSION}) and symlink or add that path into PATH so
repeated runs reuse the cached binary; update references in InstallYQIfNotExists
(arch, curl, chmod, PATH export) to use the versioned filename and ensure the
function remains no-op if the versioned binary already exists.
| set -eux -o pipefail; shopt -s inherit_errexit | ||
|
|
||
| echo "🔍 Verifying Shared Storage Between CNV and IBM Fusion Access" | ||
| echo "=======================================================" | ||
|
|
||
| # Set default values | ||
| CNV_NAMESPACE="${CNV_NAMESPACE:-openshift-cnv}" | ||
| FUSION_ACCESS_NAMESPACE="${FUSION_ACCESS_NAMESPACE:-ibm-fusion-access}" | ||
| SHARED_STORAGE_CLASS="${SHARED_STORAGE_CLASS:-ibm-spectrum-scale-cnv}" | ||
| TEST_NAMESPACE="${TEST_NAMESPACE:-shared-storage-test}" | ||
|
|
||
| # JUnit XML test results | ||
| JUNIT_RESULTS_FILE="${ARTIFACT_DIR}/junit_verify_shared_storage_tests.xml" | ||
| TEST_START_TIME=$SECONDS | ||
| TESTS_TOTAL=0 | ||
| TESTS_FAILED=0 | ||
| TESTS_PASSED=0 | ||
| TEST_CASES="" | ||
|
|
||
| # Function to escape XML special characters | ||
| escape_xml() { | ||
| local text="$1" | ||
| # Escape XML special characters: & must be first to avoid double-escaping | ||
| echo "$text" | sed 's/&/\&/g; s/</\</g; s/>/\>/g; s/"/\"/g; s/'\''/\'/g' | ||
| # Purpose: Verify PVC binding and pod mount of shared Storage Scale storage in the CNV test namespace; emit JUnit. | ||
| # Inputs: FA__CNV__TEST_NAMESPACE, FA__CNV__SHARED_STORAGE_CLASS, ARTIFACT_DIR, MAP_TESTS. | ||
| # Non-obvious: Creates a test namespace and workload to confirm shared storage is usable. | ||
|
|
||
| typeset junitResultsFile="${ARTIFACT_DIR}/junit_verify_shared_storage_tests.xml" | ||
| typeset -i testStartTime="${SECONDS}" | ||
| typeset -i testsTotal=0 | ||
| typeset -i testsFailed=0 | ||
| typeset testCases testStart testStatus testMessage |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Reproduces nounset failure against the current line-12 declaration style.
bash -c '
set -eu
typeset testCases
testCases="${testCases}appended"
echo OK: "$testCases"
' ; echo "exit=$?"
# Expect: "testCases: unbound variable" and a non-zero exit, confirming the bug.Repository: openshift/release
Length of output: 128
🏁 Script executed:
# Check the actual file content
cd ci-operator/step-registry/interop-tests/ibm-fusion-access/verify-shared-storage/
head -20 interop-tests-ibm-fusion-access-verify-shared-storage-commands.shRepository: openshift/release
Length of output: 816
🏁 Script executed:
# Check line 132 for the RecordTest call
cd ci-operator/step-registry/interop-tests/ibm-fusion-access/verify-shared-storage/
sed -n '130,135p' interop-tests-ibm-fusion-access-verify-shared-storage-commands.shRepository: openshift/release
Length of output: 195
🏁 Script executed:
# Check sibling scripts for initialization pattern
find ci-operator/step-registry/interop-tests/ibm-fusion-access -name "*commands.sh" -type f | head -10Repository: openshift/release
Length of output: 1491
🏁 Script executed:
# Examine sibling script initialization (e.g., configure-cnv-shared-storage-commands.sh)
find ci-operator/step-registry/interop-tests/ibm-fusion-access -name "configure-cnv-shared-storage-commands.sh" -o -name "prepare-worker-nodes-commands.sh" -o -name "test-vm-migration-commands.sh" | head -3 | xargs -I {} sh -c 'echo "=== {} ===" && sed -n "10,15p" "{}"'Repository: openshift/release
Length of output: 43
🏁 Script executed:
# Check the sibling scripts more directly
sed -n '10,15p' ci-operator/step-registry/interop-tests/ibm-fusion-access/configure-cnv-shared-storage/interop-tests-ibm-fusion-access-configure-cnv-shared-storage-commands.shRepository: openshift/release
Length of output: 200
🏁 Script executed:
# Check prepare-worker-nodes script
sed -n '10,15p' ci-operator/step-registry/interop-tests/ibm-fusion-access/prepare-worker-nodes/interop-tests-ibm-fusion-access-prepare-worker-nodes-commands.shRepository: openshift/release
Length of output: 180
🏁 Script executed:
# Check test-vm-migration script
sed -n '10,15p' ci-operator/step-registry/interop-tests/ibm-fusion-access/test-vm-migration/interop-tests-ibm-fusion-access-test-vm-migration-commands.shRepository: openshift/release
Length of output: 242
🏁 Script executed:
# Verify the AddTestResult function exists and line 38 reads testCases
cd ci-operator/step-registry/interop-tests/ibm-fusion-access/verify-shared-storage/
sed -n '36,42p' interop-tests-ibm-fusion-access-verify-shared-storage-commands.shRepository: openshift/release
Length of output: 292
🏁 Script executed:
# Double-check: Verify the target file's line 12 shows uninitialized typeset
cd ci-operator/step-registry/interop-tests/ibm-fusion-access/verify-shared-storage/
sed -n '12p' interop-tests-ibm-fusion-access-verify-shared-storage-commands.shRepository: openshift/release
Length of output: 112
Critical: uninitialized typeset variables under set -u will abort the script on first AddTestResult call.
Line 2 enables nounset, but line 12 declares typeset testCases testStart testStatus testMessage without initialization — leaving these variables unbound. When the first RecordTest at line 132 calls AddTestResult, line 38 reads ${testCases} before assignment, triggering testCases: unbound variable error and aborting the script before any test results are recorded.
Every sibling script in this directory (configure-cnv-shared-storage-commands.sh, prepare-worker-nodes-commands.sh, test-vm-migration-commands.sh) correctly initializes testCases=''; only this file is missing it.
Fix
typeset -i testsTotal=0
typeset -i testsFailed=0
-typeset testCases testStart testStatus testMessage
+typeset testCases=''
+typeset testStart=0
+typeset testStatus=''
+typeset testMessage=''📝 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 -eux -o pipefail; shopt -s inherit_errexit | |
| echo "🔍 Verifying Shared Storage Between CNV and IBM Fusion Access" | |
| echo "=======================================================" | |
| # Set default values | |
| CNV_NAMESPACE="${CNV_NAMESPACE:-openshift-cnv}" | |
| FUSION_ACCESS_NAMESPACE="${FUSION_ACCESS_NAMESPACE:-ibm-fusion-access}" | |
| SHARED_STORAGE_CLASS="${SHARED_STORAGE_CLASS:-ibm-spectrum-scale-cnv}" | |
| TEST_NAMESPACE="${TEST_NAMESPACE:-shared-storage-test}" | |
| # JUnit XML test results | |
| JUNIT_RESULTS_FILE="${ARTIFACT_DIR}/junit_verify_shared_storage_tests.xml" | |
| TEST_START_TIME=$SECONDS | |
| TESTS_TOTAL=0 | |
| TESTS_FAILED=0 | |
| TESTS_PASSED=0 | |
| TEST_CASES="" | |
| # Function to escape XML special characters | |
| escape_xml() { | |
| local text="$1" | |
| # Escape XML special characters: & must be first to avoid double-escaping | |
| echo "$text" | sed 's/&/\&/g; s/</\</g; s/>/\>/g; s/"/\"/g; s/'\''/\'/g' | |
| # Purpose: Verify PVC binding and pod mount of shared Storage Scale storage in the CNV test namespace; emit JUnit. | |
| # Inputs: FA__CNV__TEST_NAMESPACE, FA__CNV__SHARED_STORAGE_CLASS, ARTIFACT_DIR, MAP_TESTS. | |
| # Non-obvious: Creates a test namespace and workload to confirm shared storage is usable. | |
| typeset junitResultsFile="${ARTIFACT_DIR}/junit_verify_shared_storage_tests.xml" | |
| typeset -i testStartTime="${SECONDS}" | |
| typeset -i testsTotal=0 | |
| typeset -i testsFailed=0 | |
| typeset testCases testStart testStatus testMessage | |
| set -eux -o pipefail; shopt -s inherit_errexit | |
| # Purpose: Verify PVC binding and pod mount of shared Storage Scale storage in the CNV test namespace; emit JUnit. | |
| # Inputs: FA__CNV__TEST_NAMESPACE, FA__CNV__SHARED_STORAGE_CLASS, ARTIFACT_DIR, MAP_TESTS. | |
| # Non-obvious: Creates a test namespace and workload to confirm shared storage is usable. | |
| typeset junitResultsFile="${ARTIFACT_DIR}/junit_verify_shared_storage_tests.xml" | |
| typeset -i testStartTime="${SECONDS}" | |
| typeset -i testsTotal=0 | |
| typeset -i testsFailed=0 | |
| typeset testCases='' | |
| typeset testStart=0 | |
| typeset testStatus='' | |
| typeset testMessage='' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ci-operator/step-registry/interop-tests/ibm-fusion-access/verify-shared-storage/interop-tests-ibm-fusion-access-verify-shared-storage-commands.sh`
around lines 2 - 12, The script is aborting under set -u because variables
declared with "typeset testCases testStart testStatus testMessage" are
uninitialized; initialize these variables (e.g., set testCases='', testStart='',
testStatus='', testMessage='') where the typeset block is defined so
AddTestResult / RecordTest can read ${testCases} safely; keep numeric types
(testsTotal/testsFailed/testStartTime) unchanged and only initialize the string
test* variables near the existing typeset declarations.
amp-rh
force-pushed
the
interop-8979/firewatch-slack-webhook-test
branch
from
b6d52d5 to
11861e6
Compare
|
@amp-rh, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@amp-rh, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.sh (1)
45-47: Separate declaration and assignment to avoid maskingcatfailure (SC2155).With
set -o errexit,export VAR=$(cat ...)only surfacesexport's exit status, so acatfailure (e.g., unreadable file due to permissions) would be silently swallowed andSLACK_WEBHOOK_URLset to an empty value — the subsequentfirewatch reportwould then run as if no webhook were configured, with no indication why.🛠️ Proposed fix
if [ -f /tmp/secrets/slack/url ]; then - export SLACK_WEBHOOK_URL=$(cat /tmp/secrets/slack/url) + SLACK_WEBHOOK_URL=$(cat /tmp/secrets/slack/url) + export SLACK_WEBHOOK_URL fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.sh` around lines 45 - 47, The current export SLACK_WEBHOOK_URL=$(cat ...) masks cat failures; change this to first read the file content into a temporary variable (e.g., slack_url_content) using a standalone cat or read command, verify the read succeeded and the content is non-empty, then export SLACK_WEBHOOK_URL="$slack_url_content"; if the read fails, surface an error and exit or skip with a clear log. Update the existing if block that checks the file and the use of SLACK_WEBHOOK_URL to use this two-step read-and-export flow so cat failures are not silently swallowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.sh`:
- Around line 45-47: The current export SLACK_WEBHOOK_URL=$(cat ...) masks cat
failures; change this to first read the file content into a temporary variable
(e.g., slack_url_content) using a standalone cat or read command, verify the
read succeeded and the content is non-empty, then export
SLACK_WEBHOOK_URL="$slack_url_content"; if the read fails, surface an error and
exit or skip with a clear log. Update the existing if block that checks the file
and the use of SLACK_WEBHOOK_URL to use this two-step read-and-export flow so
cat failures are not silently swallowed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7b9b4d00-1646-4b29-b847-d10f3a393d4a
📒 Files selected for processing (3)
ci-operator/config/RedHatQE/firewatch/RedHatQE-firewatch-main__slack-webhook-test.yamlci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.shci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-ref.yaml
✅ Files skipped from review due to trivial changes (1)
- ci-operator/config/RedHatQE/firewatch/RedHatQE-firewatch-main__slack-webhook-test.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-ref.yaml
- Mount firewatch-slack-webhook secret at /tmp/secrets/slack - Read /tmp/secrets/slack/url into SLACK_WEBHOOK_URL env var - Add minimal test config to verify end-to-end webhook delivery Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wire up per-rule Slack routing in the test config so pj-rehearse validates that the webhook payload includes the slack_user field. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
amp-rh
force-pushed
the
interop-8979/firewatch-slack-webhook-test
branch
from
5cd672e to
5b4c0c0
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/pj-rehearse periodic-ci-RedHatQE-firewatch-main-slack-webhook-test-slack-webhook-test |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@amp-rh, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amp-rh 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 |
|
@amp-rh, If the problem persists, please contact Test Platform. |
|
Closing in favour of a clean PR — the previous branch accidentally included unrelated IBM Fusion Access changes that cluttered the review thread. |
|
@amp-rh: 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. |
2 participants
Summary
firewatch-slack-notify-webhook-urlsecret at/tmp/secrets/slackin thefirewatch-report-issuesstep/tmp/secrets/slack/urland export as$SLACK_WEBHOOK_URLbefore running firewatch reportSLACK_WEBHOOK_URLenv var declaration to the step refRedHatQE-firewatch-main__slack-webhook-test.yaml) withslack_userrouting to verify end-to-end webhook deliveryDependencies
Requires firewatch PR: RedHatQE/firewatch#274
slack_userandslack_channelper-rule fields to the webhook payloadpost_webhook()static method toSlackClient$SLACK_WEBHOOK_URLon issue creation, duplicate detection, and successPrerequisites
firewatch-slack-notify-webhook-urlsecret intest-credentialsnamespace with keyurlTest plan
slack-webhook-testjob from this PRslack_userfieldRelates: https://issues.redhat.com/browse/INTEROP-8979