Create post artifact collect#78722
Conversation
Signed-off-by: Eran Ifrach <eifrach@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eifrach 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 |
openshift-ci
Bot
added
the
approved
WalkthroughA new CI operator step reference for artifact collection is introduced into the telcov10n functional test workflow. The step copies artifacts from a bastion host using SSH after preparing an Ansible inventory and extracting SSH credentials. This step is integrated into the workflow's post phase and removes duplicate artifact-collection logic from the config script. ChangesArtifact Collection Step
Sequence DiagramsequenceDiagram
participant Step as Artifact Collector
participant SharedDir as Shared Directory
participant Inventory as Inventory System
participant SSH as SSH/Key Extraction
participant Bastion as Bastion Host
participant ArtifactDir as Artifact Directory
Step->>SharedDir: Check for skip.txt
alt skip.txt exists
Step->>Step: Exit early
else
Step->>SharedDir: Read inventory files
Step->>Inventory: Prepare /eco-ci-cd/inventories/ocp-deployment
Step->>Inventory: Copy group_vars and host_vars
Step->>SharedDir: Read cluster_name
Step->>SSH: Extract ansible_ssh_private_key
Step->>SSH: Extract BASTION_IP and BASTION_USER from YAML
Step->>SSH: Write key to /tmp/temp_ssh_key with restricted perms
Step->>Bastion: SCP /tmp/artifacts/* using key
Bastion->>ArtifactDir: Copy to ${ARTIFACT_DIR}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
[REHEARSALNOTIFIER]
Prior to this PR being merged, you will need to either run and acknowledge or opt to skip these rehearsals. Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ci-operator/step-registry/telcov10n/functional/compute-nto/collect-artifacts/telcov10n-functional-compute-nto-collect-artifacts-commands.sh (2)
20-26: ⚡ Quick winQuote variables and use
cut -f3-to avoid word-splitting and filename truncation.Three related issues flagged by shellcheck (SC2231/SC2086):
${SHARED_DIR}/*in theforloop is unquoted — filenames with spaces or globs inSHARED_DIRwill be split.$file,$DEST_DIR, and$DEST_FILEin lines 22–24 are unquoted — same risk.cut -d'_' -f3extracts only the 3rd underscore-delimited field. A filename likehost_vars_some_namewould yieldDEST_FILE=some, silently dropping_name. Use-f3-to take everything from the 3rd field onwards.🛠 Proposed fix
- for file in ${SHARED_DIR}/*; do + for file in "${SHARED_DIR}"/*; do if [[ "$file" == *"group_vars_"* || "$file" == *"host_vars_"* ]]; then - DEST_DIR=$( basename $file | cut -d'_' -f1,2 ) - DEST_FILE=$( basename $file | cut -d'_' -f3 ) - cp $file ${ECO_CI_CD_INVENTORY_PATH}/$DEST_DIR/$DEST_FILE + DEST_DIR=$( basename "$file" | cut -d'_' -f1,2 ) + DEST_FILE=$( basename "$file" | cut -d'_' -f3- ) + cp "$file" "${ECO_CI_CD_INVENTORY_PATH}/$DEST_DIR/$DEST_FILE" fi done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/telcov10n/functional/compute-nto/collect-artifacts/telcov10n-functional-compute-nto-collect-artifacts-commands.sh` around lines 20 - 26, The loop and variable usages must be quoted and DEST_FILE should preserve trailing underscores/fields: change the for-loop to iterate over "${SHARED_DIR}"/* and wrap all expansions in quotes (e.g., "$file", "$DEST_DIR", "$DEST_FILE") to avoid word-splitting; compute DEST_DIR with basename "$file" | cut -d'_' -f1,2 and compute DEST_FILE with basename "$file" | cut -d'_' -f3- (note the -f3- to keep the rest of the name), and ensure the cp invocation uses quoted arguments like cp "$file" "${ECO_CI_CD_INVENTORY_PATH}/$DEST_DIR/$DEST_FILE".
54-54: ⚡ Quick winSSH key extraction with
grep -A 100is fragile and may silently truncate large keys.The
-A 100flag caps the captured output at 100 lines after the match. A standard RSA 4096-bit key body is ~52 lines, but an RSA 8192-bit key exceeds 100 lines — meaning the-----END ... KEY-----marker would be cut off, producing an invalid key file. Beyond truncation, every subsequent YAML field in the inventory that falls within the 100-line window is appended to the file; while PEM parsers typically stop at the END marker, relying on trailing garbage being silently ignored is brittle.A more reliable alternative is to extract the value using a dedicated YAML-aware tool or
awk:🛠 More robust key extraction (awk)
-grep ansible_ssh_private_key -A 100 "${ECO_CI_CD_INVENTORY_PATH}/group_vars/all" | sed 's/ansible_ssh_private_key: //g' | sed "s/'//g" > "/tmp/temp_ssh_key" +awk '/ansible_ssh_private_key:/{found=1; sub(/ansible_ssh_private_key: /, ""); gsub(/'\''/, "")} found{print; if(/END .* KEY/) exit}' \ + "${ECO_CI_CD_INVENTORY_PATH}/group_vars/all" > "/tmp/temp_ssh_key"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/telcov10n/functional/compute-nto/collect-artifacts/telcov10n-functional-compute-nto-collect-artifacts-commands.sh` at line 54, The current extraction using "grep ansible_ssh_private_key -A 100 ... > /tmp/temp_ssh_key" can truncate or pull unrelated YAML because of the fixed "-A 100" window; replace this with a YAML-aware or line-aware extractor (e.g., use awk to locate the "ansible_ssh_private_key:" key and stream all following lines until the "-----END ... KEY-----" marker, stripping leading "ansible_ssh_private_key: " and surrounding quotes, then write to /tmp/temp_ssh_key) or use yq to read the ansible_ssh_private_key value directly and output it to /tmp/temp_ssh_key; update the snippet that contains the grep command in telcov10n-functional-compute-nto-collect-artifacts-commands.sh to use the chosen robust extractor and ensure the resulting file permissions are set appropriately (chmod 600).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ci-operator/step-registry/telcov10n/functional/compute-nto/collect-artifacts/telcov10n-functional-compute-nto-collect-artifacts-commands.sh`:
- Around line 62-63: The scp command will fail the post step when /tmp/artifacts
on the bastion is missing or empty; change the artifact-copy logic to first SSH
to the bastion and check that /tmp/artifacts exists and is non-empty (e.g.,
using test -d and a non-empty ls check) and only then perform the transfer,
otherwise skip with success; update the block that currently runs scp -r ...
"${BASTION_USER}@${BASTION_IP}":/tmp/artifacts/* "${ARTIFACT_DIR}" to perform
the remote existence/non-empty check using ${BASTION_USER}@${BASTION_IP} and
/tmp/artifacts before invoking scp (or switch to an ssh+tar stream that is only
run when the check passes) so the post step remains best-effort and does not
abort on missing artifacts.
---
Nitpick comments:
In
`@ci-operator/step-registry/telcov10n/functional/compute-nto/collect-artifacts/telcov10n-functional-compute-nto-collect-artifacts-commands.sh`:
- Around line 20-26: The loop and variable usages must be quoted and DEST_FILE
should preserve trailing underscores/fields: change the for-loop to iterate over
"${SHARED_DIR}"/* and wrap all expansions in quotes (e.g., "$file", "$DEST_DIR",
"$DEST_FILE") to avoid word-splitting; compute DEST_DIR with basename "$file" |
cut -d'_' -f1,2 and compute DEST_FILE with basename "$file" | cut -d'_' -f3-
(note the -f3- to keep the rest of the name), and ensure the cp invocation uses
quoted arguments like cp "$file"
"${ECO_CI_CD_INVENTORY_PATH}/$DEST_DIR/$DEST_FILE".
- Line 54: The current extraction using "grep ansible_ssh_private_key -A 100 ...
> /tmp/temp_ssh_key" can truncate or pull unrelated YAML because of the fixed
"-A 100" window; replace this with a YAML-aware or line-aware extractor (e.g.,
use awk to locate the "ansible_ssh_private_key:" key and stream all following
lines until the "-----END ... KEY-----" marker, stripping leading
"ansible_ssh_private_key: " and surrounding quotes, then write to
/tmp/temp_ssh_key) or use yq to read the ansible_ssh_private_key value directly
and output it to /tmp/temp_ssh_key; update the snippet that contains the grep
command in telcov10n-functional-compute-nto-collect-artifacts-commands.sh to use
the chosen robust extractor and ensure the resulting file permissions are set
appropriately (chmod 600).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a119c790-e5f3-4c2f-babe-4296c5d97c41
📒 Files selected for processing (6)
ci-operator/step-registry/telcov10n/functional/compute-nto/collect-artifacts/OWNERSci-operator/step-registry/telcov10n/functional/compute-nto/collect-artifacts/telcov10n-functional-compute-nto-collect-artifacts-commands.shci-operator/step-registry/telcov10n/functional/compute-nto/collect-artifacts/telcov10n-functional-compute-nto-collect-artifacts-ref.metadata.jsonci-operator/step-registry/telcov10n/functional/compute-nto/collect-artifacts/telcov10n-functional-compute-nto-collect-artifacts-ref.yamlci-operator/step-registry/telcov10n/functional/compute-nto/config/telcov10n-functional-compute-nto-config-commands.shci-operator/step-registry/telcov10n/functional/compute-nto/ocp-setup/telcov10n-functional-compute-nto-ocp-setup-workflow.yaml
💤 Files with no reviewable changes (1)
- ci-operator/step-registry/telcov10n/functional/compute-nto/config/telcov10n-functional-compute-nto-config-commands.sh
| scp -r -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i /tmp/temp_ssh_key \ | ||
| "${BASTION_USER}@${BASTION_IP}":/tmp/artifacts/* "${ARTIFACT_DIR}" |
There was a problem hiding this comment.
scp will fail (and abort the post step) if /tmp/artifacts/ is absent or empty on the bastion.
The remote glob /tmp/artifacts/* is expanded by the remote shell. If the directory does not exist or is empty — which is a real scenario when the test step was skipped or failed early — scp returns a non-zero exit code and set -e terminates the entire post step. Since artifact collection is best-effort in a post step, consider a graceful fallback.
🛠 Proposed fix
echo "Copy logs and artifacts to artifacts directory"
-scp -r -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i /tmp/temp_ssh_key \
- "${BASTION_USER}@${BASTION_IP}":/tmp/artifacts/* "${ARTIFACT_DIR}"
+scp -r -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i /tmp/temp_ssh_key \
+ "${BASTION_USER}@${BASTION_IP}":/tmp/artifacts/* "${ARTIFACT_DIR}" || \
+ echo "WARNING: scp returned non-zero (no artifacts or bastion unreachable); continuing."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ci-operator/step-registry/telcov10n/functional/compute-nto/collect-artifacts/telcov10n-functional-compute-nto-collect-artifacts-commands.sh`
around lines 62 - 63, The scp command will fail the post step when
/tmp/artifacts on the bastion is missing or empty; change the artifact-copy
logic to first SSH to the bastion and check that /tmp/artifacts exists and is
non-empty (e.g., using test -d and a non-empty ls check) and only then perform
the transfer, otherwise skip with success; update the block that currently runs
scp -r ... "${BASTION_USER}@${BASTION_IP}":/tmp/artifacts/* "${ARTIFACT_DIR}" to
perform the remote existence/non-empty check using ${BASTION_USER}@${BASTION_IP}
and /tmp/artifacts before invoking scp (or switch to an ssh+tar stream that is
only run when the check passes) so the post step remains best-effort and does
not abort on missing artifacts.
|
/pj-rehearse periodic-ci-openshift-kni-eco-ci-cd-main-compute-nto-e2e-telcov10n-nto-tests-v1-t0-crun-bm-4-18 |
|
@eifrach: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@eifrach: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Summary by CodeRabbit
Release Notes
New Features
Tests