Run Warp tests nightly on IBM Cloud #9196
Conversation
WalkthroughAdds IBM Cloud nightly provisioning/cleanup workflows, a cloud-init VM config, and helper scripts to run containerized Warp tests, upload logs to IBM COS, and notify Slack. Enhances Warp test wrappers and Makefile to forward CLI args and support a new --obj-randsize flag. Adds accompanying documentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Scheduler as GitHub Scheduler
participant ProvWF as Provision Workflow
participant IBM as IBM Cloud
participant VM as VM (cloud-init)
participant Repo as Git Repo
participant COS as IBM COS (S3 API)
participant Slack as Slack
Scheduler->>ProvWF: Trigger nightly provision
ProvWF->>IBM: Create VM + attach Floating IP (tags)
ProvWF-->>Scheduler: Provision complete
Note over VM: cloud-init boot
VM->>Repo: git clone noobaa-core
VM->>VM: install deps, enable Docker
VM->>VM: run containerized warp runner
VM->>COS: Upload warp logs (success or failure logs)
VM->>Slack: Post status + log location
VM-->>IBM: Shutdown VM
sequenceDiagram
autonumber
participant Runner as run_containerized_warp_on_cloud_runner.sh
participant Make as make tester / test-warp
participant Docker as Tester Container
participant COS as IBM COS (S3 API)
participant Slack as Slack
Runner->>Runner: source /etc/warp.env, set traps
Runner->>Make: build tester image
Runner->>Make: test-warp WARP_ARGS=...
Make->>Docker: run warp test
alt Success
Runner->>COS: Upload logs (warp-test-logs/)
Runner->>Slack: Notify success
else Failure
Runner->>COS: Upload cloud-init logs
Runner->>Slack: Notify failure
end
Runner->>Runner: shutdown on EXIT
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
Neon-White
force-pushed
the
nightly-warp
branch
from
3cfc9ef to
5668356
Compare
Neon-White
force-pushed
the
nightly-warp
branch
from
7b02317 to
5bdb339
Compare
Neon-White
force-pushed
the
nightly-warp
branch
from
143c369 to
9b3dcd0
Compare
Neon-White
force-pushed
the
nightly-warp
branch
from
29c47ee to
ffb8b50
Compare
Neon-White
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/external_tests/warp/run_warp.js (1)
81-88: Credential guard allows partial keys.If only one of access_key/secret_key is provided, the script proceeds with an undefined credential. Require either account_name OR both keys.
- if (!account_name && !access_key && !secret_key) { - console.error('Please provide account_name, access_key and secret_key'); - throw new Error('Please provide account_name or access_key and secret_key'); - } + if (!(account_name || (access_key && secret_key))) { + throw new Error('Provide account_name or both access_key and secret_key'); + } + if (!account_name && (!!access_key ^ !!secret_key)) { + throw new Error('access_key and secret_key must be provided together'); + }
🧹 Nitpick comments (21)
src/test/external_tests/warp/run_nc_warp_on_test_container.sh (1)
73-73: Pass-through of CLI args is correct; prefer exec to preserve exit code and PID.Replace to avoid an extra shell process and ensure Node’s exit code is surfaced directly.
-node ./src/test/external_tests/warp/run_warp.js "$@" +exec node ./src/test/external_tests/warp/run_warp.js "$@"tools/ibm_runner_helpers/slack_notifier.js (3)
9-11: Exit early on missing webhook is fine; add minimal usage guard for status/message.Avoid sending “undefined” text if message is omitted; fall back to a short default.
- if (!webhookUrl) { + if (!webhookUrl) { process.exit(0); } + const safeStatus = status || 'info'; + const safeMessage = message || '(no message)';
23-27: Do not swallow diagnostic detail; log stack and keep exit code policy.Retain exit 0 but include stack for postmortems.
- } catch (error) { - console.error('Notification failed:', error.message); + } catch (error) { + console.error('Notification failed:', error && (error.stack || error.message || error)); }
4-4: Global dependency assumption on @slack/webhook.This relies on NODE_PATH/global install. Consider bundling this as a project dep or falling back to a simple HTTPS POST to reduce coupling to global tooling on the VM.
src/test/external_tests/warp/run_warp_on_test_container.sh (1)
73-73: Pass-through of CLI args is correct; prefer exec for cleaner exit semantics.-node ./src/test/external_tests/warp/run_warp.js "$@" +exec node ./src/test/external_tests/warp/run_warp.js "$@"Makefile (1)
368-368: Same quoting concern for NC target. Apply the same change here.- $(CONTAINER_ENGINE) run ... $(TESTER_TAG) bash -c "./src/test/external_tests/warp/run_nc_warp_on_test_container.sh $(WARP_ARGS)" + $(CONTAINER_ENGINE) run ... $(TESTER_TAG) ./src/test/external_tests/warp/run_nc_warp_on_test_container.sh $(WARP_ARGS).github/ibm-warp-runner-config.yaml (1)
32-36: Repo ref is hardcoded to default branch.Consider accepting BRANCH/REF via secret/env to allow testing PR SHAs reproducibly on the VM.
- - [ git, clone, https://github.com/noobaa/noobaa-core.git, /home/ubuntu/tests/noobaa-core ] + - [ bash, -lc, 'git clone --depth 1 --branch "${NOOBAA_REF:-master}" https://github.com/noobaa/noobaa-core.git /home/ubuntu/tests/noobaa-core' ]docs/CI & Tests/warp-on-ibm.md (4)
80-83: Workflow path mismatch.Doc links to ibm-vm-cleanup.yaml but repo uses ibm-nightly-warp-vm-cleanup.yaml.
-**File**: [`.github/workflows/ibm-vm-cleanup.yaml`](../../.github/workflows/ibm-vm-cleanup.yaml) +**File**: [`.github/workflows/ibm-nightly-warp-vm-cleanup.yaml`](../../.github/workflows/ibm-nightly-warp-vm-cleanup.yaml)
58-68: Markdown lint: add language to fenced block.Specify a language for the ASCII diagram fence to satisfy MD040.
-``` +```text ┌─────────────────┐ ┌─────────────────┐ ┌─────────────────┐ │ Provision │ │ Test & Log │ │ Cleanup │ │ (Midnight) │───▶│ (3+ hours) │───▶│ (4 AM) │ └─────────────────┘ └─────────────────┘ └─────────────────┘
90-93: List indentation (MD007).Fix indentation to satisfy markdownlint.
- - The required packages that'll be installed on the VM - - Additional setup commands - - An environment file (`warp.env`) that is used to pass env vars from GitHub to the runner (via `envsubst` variable templating) - - The 'entrypoint' (that is the last command that is run by cloud-init; in our case, the Warp scripts) +- The required packages that'll be installed on the VM +- Additional setup commands +- An environment file (`warp.env`) that is used to pass env vars from GitHub to the runner (via `envsubst` variable templating) +- The 'entrypoint' (that is the last command that is run by cloud-init; in our case, the Warp scripts)
114-114: Remove trailing punctuation in heading (MD026).Drop the colon.
-## Log locations: +## Log locations.github/workflows/ibm-nightly-warp-vm-provision.yaml (2)
70-87: Don’t suppress provision command output.
>/dev/null 2>&1obscures diagnostics; keep output for incident triage.- if ! ibmcloud is instance-create \ + if ! ibmcloud is instance-create \ "${INSTANCE_NAME}" \ "${VPC_NAME}" \ "${ZONE}" \ "${INSTANCE_PROFILE}" \ "${SUBNET_ID}" \ --image "${IMAGE_ID}" \ --sgs "${SECURITY_GROUP_ID}" \ - --user-data @/tmp/ibm-warp-runner-config-with-secrets.yaml \ - > /dev/null 2>&1 + --user-data @/tmp/ibm-warp-runner-config-with-secrets.yaml then echo "❌ Failed to provision IBM VM" exit 1 fi
88-104: Bind floating IP may race before NIC is ready; add a short retry.Reduce flakiness by polling NIC for a few seconds before reserve/bind.
- - name: Reserve and bind a floating IP + - name: Reserve and bind a floating IP run: | - - VM_JSON=$(ibmcloud is instance "${INSTANCE_NAME}" --output json) - NIC_ID=$(echo "${VM_JSON}" | jq -r '.primary_network_attachment.virtual_network_interface.id') + for i in {1..10}; do + VM_JSON=$(ibmcloud is instance "${INSTANCE_NAME}" --output json) || true + NIC_ID=$(echo "${VM_JSON}" | jq -r '.primary_network_attachment.virtual_network_interface.id // empty') + [ -n "${NIC_ID}" ] && break + sleep 3 + done + if [ -z "${NIC_ID}" ]; then + echo "❌ NIC not ready"; exit 1 + fi.github/workflows/ibm-nightly-warp-vm-cleanup.yaml (3)
16-16: Increase cleanup timeout to accommodate slow deletions.IBM VPC instance deletion can exceed 10 minutes. Suggest 20–30 minutes to avoid premature job termination.
- timeout-minutes: 10 + timeout-minutes: 30
22-27: Pin or verify CLI/plugin versions for supply‑chain stability.Curl‑pipe‑sh installs latest CLI and plugin at run time; a breaking release could break the workflow. Pin versions or at least print them and fail if below a minimum.
- curl -fsSL https://clis.cloud.ibm.com/install/linux | sh >/dev/null - ibmcloud plugin install vpc-infrastructure -f >/dev/null - ibmcloud --version + curl -fsSL https://clis.cloud.ibm.com/install/linux | sh >/dev/null + ibmcloud --version + # Pin or assert plugin version (example: >= 2.22.0) + ibmcloud plugin install vpc-infrastructure -f >/dev/null + ibmcloud plugin list | grep -E '^vpc-infrastructure' || { echo "missing vpc plugin"; exit 1; }
39-45: Target the resource group as well as the region (if provided).To avoid cross‑group lookups and accidental deletions, set the resource group from config when available.
ibmcloud login --apikey "${IBM_CLOUD_API_KEY}" -r "${REGION}" >/dev/null - ibmcloud target -r "${REGION}" >/dev/null + ibmcloud target -r "${REGION}" ${RESOURCE_GROUP:+-g "${RESOURCE_GROUP}"} >/dev/nulltools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh (5)
7-10: Upload logs on failure too; factor out upload to a reusable function.Currently failures shut down without uploading logs. Extract upload logic and call it from both paths.
-handle_failure() { -local msg="$1" -send_failure_notification "$msg" -} +upload_logs() { + # Upload logs to S3 bucket with timestamp-based directory + local timestamp + timestamp=$(date -u +"%Y-%m-%dT%H-%M-%SZ") + local log_dir="warp-test-logs-${timestamp}" + local logs_path="/home/ubuntu/tests/noobaa-core/logs/warp-test-logs" + echo "Uploading logs to IBM COS s3://${WARP_LOGS_BUCKET}/${log_dir}/" + aws s3 cp "$logs_path" "s3://${WARP_LOGS_BUCKET}/${log_dir}/" --recursive --no-progress --endpoint-url "${IBM_COS_ENDPOINT}" + echo "Successfully uploaded logs to IBM COS s3://${WARP_LOGS_BUCKET}/${log_dir}/" + echo "$log_dir" +} + +handle_failure() { + local msg="$1" + local dir="" + { dir="$(upload_logs)"; } || echo "Log upload on failure skipped (upload error)" + send_failure_notification "${msg}${dir:+ (logs: s3://${WARP_LOGS_BUCKET}/${dir}/)}" +} @@ -handle_success() { -# Upload logs to S3 bucket with timestamp-based directory -local timestamp=$(date +"%Y-%m-%d_%H-%M-%S") -local log_dir="warp-test-logs-${timestamp}" -local logs_path="/home/ubuntu/tests/noobaa-core/logs/warp-test-logs" - -echo "Uploading logs to IBM COS s3://${WARP_LOGS_BUCKET}/${log_dir}/" -aws s3 cp "$logs_path" "s3://${WARP_LOGS_BUCKET}/${log_dir}/" --recursive --no-progress --endpoint-url "${IBM_COS_ENDPOINT}" -echo "Successfully uploaded logs to IBM COS s3://${WARP_LOGS_BUCKET}/${log_dir}/" -send_success_notification "s3://${WARP_LOGS_BUCKET}/${log_dir}/" -} +handle_success() { + local dir + dir="$(upload_logs)" + send_success_notification "s3://${WARP_LOGS_BUCKET}/${dir}/" +}Also applies to: 12-22
14-16: Shellcheck SC2155 and use UTC timestamps.Avoid assignment in declaration and prefer UTC/Z‑suffixed format for cross‑region consistency.
-local timestamp=$(date +"%Y-%m-%d_%H-%M-%S") +local timestamp +timestamp=$(date -u +"%Y-%m-%dT%H-%M-%SZ")
51-53: Allow overriding WARP_ARGS via env; keep sensible defaults.Hardcoding prevents parameterization from cloud-init or Makefile wrappers.
-echo "Running Warp tests..." -make test-warp -o tester WARP_ARGS="--duration 3h --obj-size 10m --obj-randsize" +echo "Running Warp tests..." +DEFAULT_WARP_ARGS="--duration 3h --obj-size 10m --obj-randsize" +make test-warp -o tester WARP_ARGS="${WARP_ARGS:-$DEFAULT_WARP_ARGS}"
39-41: Enhance ERR trap detail with failing command.Include
$BASH_COMMANDfor quicker triage.-trap 'handle_failure "line $LINENO exit $?"' ERR +trap 'handle_failure "cmd: $BASH_COMMAND | line: $LINENO | status: $?"' ERR
35-37: Graceful shutdown: give notifications/flush a moment.Add a small delay before poweroff to ensure last logs/stdout flush.
echo "Shutting down VM..." sudo shutdown -P now +# give kernel a moment to flush (best-effort) +sleep 1 || true
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/ibm-warp-runner-config.yaml(1 hunks).github/workflows/ibm-nightly-warp-vm-cleanup.yaml(1 hunks).github/workflows/ibm-nightly-warp-vm-provision.yaml(1 hunks)Makefile(1 hunks)docs/CI & Tests/warp-on-ibm.md(1 hunks)src/test/external_tests/warp/run_nc_warp_on_test_container.sh(1 hunks)src/test/external_tests/warp/run_warp.js(3 hunks)src/test/external_tests/warp/run_warp_on_test_container.sh(1 hunks)tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh(1 hunks)tools/ibm_runner_helpers/slack_notifier.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/external_tests/warp/run_warp_on_test_container.shsrc/test/external_tests/warp/run_warp.jssrc/test/external_tests/warp/run_nc_warp_on_test_container.sh
🧠 Learnings (1)
📚 Learning: 2025-06-19T11:03:49.934Z
Learnt from: nadavMiz
PR: noobaa/noobaa-core#9099
File: .github/workflows/run-pr-tests.yaml:135-136
Timestamp: 2025-06-19T11:03:49.934Z
Learning: In the noobaa-core project, the `make tester` command also calls `make noobaa` due to makefile target dependencies, so running `make tester` will build both the noobaa and tester images.
Applied to files:
Makefile
🧬 Code graph analysis (1)
tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh (1)
src/manage_nsfs/manage_nsfs_logging.js (1)
success(19-19)
🪛 markdownlint-cli2 (0.17.2)
docs/CI & Tests/warp-on-ibm.md
58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
90-90: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
91-91: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
92-92: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
93-93: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
114-114: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🪛 Shellcheck (0.10.0)
tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh
[warning] 14-14: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (2)
.github/workflows/ibm-nightly-warp-vm-provision.yaml (1)
13-18: Timeout may be tight.10 minutes can be marginal for provisioning + IP binding. Consider 15–20 minutes to avoid false failures.
Makefile (1)
360-360: ```shell
#!/bin/bash
set -euo pipefailecho "1) Searching repository for WARP_ARGS and script invocation..."
rg -n --hidden -S '\bWARP_ARGS\b' -C3 || true
rg -n --hidden -S 'run_warp_on_test_container.sh' -C3 || true
rg -n --hidden -S 'noobaa_.*run_warp_on_test_container.sh' -C3 || trueecho
echo "2) Show Makefile snippet around line ~360 (if present)..."
if [ -f Makefile ]; then
sed -n '320,400p' Makefile || true
else
echo "Makefile not found at repo root"
fiecho
echo "3) Try to locate the warp script file and show its header (first 200 lines)..."
script_path=$(rg -n --hidden -S 'run_warp_on_test_container.sh' --no-line-number | head -n1 | cut -d: -f1 || true)
if [ -n "$script_path" ] && [ -f "$script_path" ]; then
echo "Found script: $script_path"
sed -n '1,200p' "$script_path" || true
else
echo "run_warp_on_test_container.sh not found via ripgrep"
fiecho
echo "4) Show where CONTAINER_ENGINE / TESTER_TAG / WARP_ARGS are defined (if anywhere)"
rg -n --hidden -S '\bCONTAINER_ENGINE\b' -C2 || true
rg -n --hidden -S '\bTESTER_TAG\b' -C2 || true
rg -n --hidden -S '^\sWARP_ARGS\s[:?+]?=' -C2 || trueecho
echo "5) Print examples of Makefile targets invoking docker/run for context (search common patterns)"
rg -n --hidden -S 'docker run' -C2 || true
rg -n --hidden -S '\b$(CONTAINER_ENGINE)\s+run\b' -C2 || true</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| # Enable and start Docker daemon | ||
| - [ systemctl, enable, --now, docker ] | ||
| # Install Slack webhook package globally for the notifier script | ||
| - [ npm, -g, install, '@slack/webhook' ] |
There was a problem hiding this comment.
Pin and verify Slack package install.
Global, unpinned npm install is non-reproducible and may break silently. Pin a version and disable audits for speed.
- - [ npm, -g, install, '@slack/webhook' ]
+ - [ npm, -g, install, '@slack/webhook@^7', '--no-audit', '--no-fund' ]📝 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.
| - [ npm, -g, install, '@slack/webhook' ] | |
| - [ npm, -g, install, '@slack/webhook@^7', '--no-audit', '--no-fund' ] |
🤖 Prompt for AI Agents
.github/ibm-warp-runner-config.yaml around line 24: the config uses a global,
unpinned npm install of '@slack/webhook' which is non-reproducible; update the
command to pin a specific semantic version (e.g.
'@slack/webhook@<major>.<minor>.<patch>') and add the npm flag --no-audit to
speed runs, so replace the unpinned entry with a pinned-package install and
include --no-audit (and keep -g) to ensure reproducible, faster installs.
| - [ curl, -sS, https://awscli.amazonaws.com/awscli-exe-linux-x86_64.zip, -o, awscliv2.zip ] | ||
| - [ unzip, awscliv2.zip ] | ||
| - [ ./aws/install ] |
There was a problem hiding this comment.
Verify AWS CLI download integrity.
Add checksum/signature verification to mitigate MITM/corruption.
- [ curl, -sS, https://awscli.amazonaws.com/awscli-exe-linux-x86_64.zip, -o, awscliv2.zip ]
+ - [ curl, -sS, https://awscli.amazonaws.com/awscli-exe-linux-x86_64.zip.sig, -o, awscliv2.zip.sig ]
+ # (Option A) Sig verification if GPG key is provisioned
+ # - [ gpg, --verify, awscliv2.zip.sig, awscliv2.zip ]
+ # (Option B) Checksum verification (replace <SHA256> with the published value)
+ # - [ sha256sum, -c, <<<, "<SHA256> awscliv2.zip" ]
- [ unzip, awscliv2.zip ]Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .github/ibm-warp-runner-config.yaml around lines 26 to 28, the workflow
downloads and unzips the AWS CLI without validating the artifact; add integrity
verification by fetching the official SHA256 checksum (or signature) from AWS
over HTTPS, compute the downloaded file's checksum and compare it (or verify the
signature with the vendor's GPG key), and abort the job on mismatch; only
proceed to unzip and install if verification succeeds and clean up the archive
afterward.
| - name: Mask and export VM config from JSON secret | ||
| env: | ||
| IBM_WARP_VM_CONFIG: ${{ secrets.IBM_WARP_VM_CONFIG }} | ||
| run: | | ||
| # Parse JSON -> KEY=VALUE lines, mask each VALUE before adding to env | ||
| jq -r 'to_entries[] | "\(.key)=\(.value)"' <<<"${IBM_WARP_VM_CONFIG}" | | ||
| while IFS='=' read -r k v; do | ||
| [ -n "$v" ] && echo "::add-mask::$v" | ||
| printf '%s=%s\n' "$k" "$v" >> "${GITHUB_ENV}" | ||
| done |
There was a problem hiding this comment.
Robust JSON→env export: handle newlines/equals and invalid keys.
The current loop breaks on values with newlines or '=' and may emit invalid env var names. Also, masking before export is good, but still logs raw keys.
- jq -r 'to_entries[] | "\(.key)=\(.value)"' <<<"${IBM_WARP_VM_CONFIG}" |
- while IFS='=' read -r k v; do
- [ -n "$v" ] && echo "::add-mask::$v"
- printf '%s=%s\n' "$k" "$v" >> "${GITHUB_ENV}"
- done
+ # Only accept shell-safe keys; base64-encode values to preserve content, then decode on export
+ jq -r 'to_entries[] | select(.key|test("^[A-Z_][A-Z0-9_]*$")) | "\(.key)=\(.value|tostring|@base64)"' <<<"${IBM_WARP_VM_CONFIG}" |
+ while IFS='=' read -r k v_b64; do
+ v="$(printf '%s' "$v_b64" | base64 -d)"
+ [ -n "$v" ] && echo "::add-mask::$v"
+ printf '%s<<__EOV__\n%s\n__EOV__\n' "$k" "$v" >> "${GITHUB_ENV}"
+ done📝 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.
| - name: Mask and export VM config from JSON secret | |
| env: | |
| IBM_WARP_VM_CONFIG: ${{ secrets.IBM_WARP_VM_CONFIG }} | |
| run: | | |
| # Parse JSON -> KEY=VALUE lines, mask each VALUE before adding to env | |
| jq -r 'to_entries[] | "\(.key)=\(.value)"' <<<"${IBM_WARP_VM_CONFIG}" | | |
| while IFS='=' read -r k v; do | |
| [ -n "$v" ] && echo "::add-mask::$v" | |
| printf '%s=%s\n' "$k" "$v" >> "${GITHUB_ENV}" | |
| done | |
| - name: Mask and export VM config from JSON secret | |
| env: | |
| IBM_WARP_VM_CONFIG: ${{ secrets.IBM_WARP_VM_CONFIG }} | |
| run: | | |
| # Only accept shell-safe keys; base64-encode values to preserve content, then decode on export | |
| jq -r 'to_entries[] | select(.key|test("^[A-Z_][A-Z0-9_]*$")) | "\(.key)=\(.value|tostring|@base64)"' <<<"${IBM_WARP_VM_CONFIG}" | | |
| while IFS='=' read -r k v_b64; do | |
| v="$(printf '%s' "$v_b64" | base64 -d)" | |
| [ -n "$v" ] && echo "::add-mask::$v" | |
| printf '%s<<__EOV__\n%s\n__EOV__\n' "$k" "$v" >> "${GITHUB_ENV}" | |
| done |
🤖 Prompt for AI Agents
.github/workflows/ibm-nightly-warp-vm-cleanup.yaml lines 28-37: the current
JSON->env loop breaks on values containing newlines or '=' and can produce
invalid env var names and leak raw keys; fix by iterating keys with jq -r
'keys[]', for each key fetch the raw value with jq -r --arg k "$key" '.[$k]' to
preserve newlines, sanitize the key into a valid env var name (replace non
[A-Za-z0-9_] with '_' and if it starts with a digit prefix with '_'), call
::add-mask:: on the full value but do not print raw keys, and write the var to
GITHUB_ENV using the multiline heredoc format (e.g. echo "SANITIZED_KEY<<EOF" >>
"$GITHUB_ENV"; echo "$VALUE" >> "$GITHUB_ENV"; echo "EOF" >> "$GITHUB_ENV") so
equals and newlines are preserved.
| - name: Clean up floating IPs | ||
| run: | | ||
| echo "🔍 Looking for floating IP" | ||
| FLOATING_IP_ID=$(ibmcloud is floating-ips --output json | jq -r --arg name "${FLOATING_IP_NAME}" '.[] | select(.name == $name) | .id' | head -n1) | ||
| if [ -n "${FLOATING_IP_ID}" ] && [ "${FLOATING_IP_ID}" != "null" ]; then | ||
| echo "🗑️ Releasing floating IP" | ||
| ibmcloud is floating-ip-release "${FLOATING_IP_ID}" --force >/dev/null | ||
| echo "✅ Floating IP released successfully" | ||
| else | ||
| echo "ℹ️ No floating IP found" | ||
| fi | ||
There was a problem hiding this comment.
Prefer identifiers from provisioning, fall back to name.
Safer to release by ID emitted during provisioning; name matching risks wrong resource if duplicates exist.
- FLOATING_IP_ID=$(ibmcloud is floating-ips --output json | jq -r --arg name "${FLOATING_IP_NAME}" '.[] | select(.name == $name) | .id' | head -n1)
+ if [ -n "${FLOATING_IP_ID:-}" ] && [ "${FLOATING_IP_ID}" != "null" ]; then
+ echo "Using provided FLOATING_IP_ID"
+ else
+ FLOATING_IP_ID=$(ibmcloud is floating-ips --output json | jq -r --arg name "${FLOATING_IP_NAME}" '.[] | select(.name == $name) | .id' | head -n1)
+ fi📝 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.
| - name: Clean up floating IPs | |
| run: | | |
| echo "🔍 Looking for floating IP" | |
| FLOATING_IP_ID=$(ibmcloud is floating-ips --output json | jq -r --arg name "${FLOATING_IP_NAME}" '.[] | select(.name == $name) | .id' | head -n1) | |
| if [ -n "${FLOATING_IP_ID}" ] && [ "${FLOATING_IP_ID}" != "null" ]; then | |
| echo "🗑️ Releasing floating IP" | |
| ibmcloud is floating-ip-release "${FLOATING_IP_ID}" --force >/dev/null | |
| echo "✅ Floating IP released successfully" | |
| else | |
| echo "ℹ️ No floating IP found" | |
| fi | |
| - name: Clean up floating IPs | |
| run: | | |
| echo "🔍 Looking for floating IP" | |
| if [ -n "${FLOATING_IP_ID:-}" ] && [ "${FLOATING_IP_ID}" != "null" ]; then | |
| echo "Using provided FLOATING_IP_ID" | |
| else | |
| FLOATING_IP_ID=$(ibmcloud is floating-ips --output json | jq -r --arg name "${FLOATING_IP_NAME}" '.[] | select(.name == $name) | .id' | head -n1) | |
| fi | |
| if [ -n "${FLOATING_IP_ID}" ] && [ "${FLOATING_IP_ID}" != "null" ]; then | |
| echo "🗑️ Releasing floating IP" | |
| ibmcloud is floating-ip-release "${FLOATING_IP_ID}" --force >/dev/null | |
| echo "✅ Floating IP released successfully" | |
| else | |
| echo "ℹ️ No floating IP found" | |
| fi |
🤖 Prompt for AI Agents
.github/workflows/ibm-nightly-warp-vm-cleanup.yaml around lines 46 to 58: the
workflow currently looks up and releases a floating IP by name which can
accidentally target the wrong resource when duplicates exist; update the script
to prefer an ID emitted during provisioning (e.g., FLOATING_IP_ID or a
prov-specific env var) and only fall back to searching by name if that ID is
empty, using the chosen ID in the release call; add robust checks for
empty/"null" values and keep the same success/failure echoes.
| - name: Clean up IBM VM | ||
| run: | | ||
| echo "🔍 Looking for IBM VM to delete..." | ||
| INSTANCE_ID=$(ibmcloud is instances --output json | jq -r --arg name "${INSTANCE_NAME}" '.[] | select(.name == $name) | .id') | ||
| if [ -z "${INSTANCE_ID}" ] || [ "${INSTANCE_ID}" = "null" ]; then | ||
| echo "⚠️ No VM found with name" | ||
| echo "This might be expected if the VM was already deleted or never created." | ||
| exit 0 | ||
| fi | ||
| echo "🔎 Found VM" | ||
| echo "🗑️ Deleting IBM VM..." | ||
| ibmcloud is instance-delete "${INSTANCE_ID}" --force >/dev/null | ||
| echo "✅ IBM VM deleted successfully" | ||
| # Wait and verify deletion | ||
| sleep 10 | ||
| VERIFY_ID=$(ibmcloud is instances --output json | jq -r --arg name "${INSTANCE_NAME}" '.[] | select(.name == $name) | .id' 2>/dev/null || echo "") | ||
| if [ -z "${VERIFY_ID}" ] || [ "${VERIFY_ID}" = "null" ]; then | ||
| echo "✅ Verified: VM has been completely removed" | ||
| else | ||
| echo "⚠️ VM deletion initiated but may still be in progress" | ||
| fi | ||
There was a problem hiding this comment.
Avoid deleting the wrong VM; add ID preference and deletion polling.
Deletion by name is risky; also, a fixed 10s wait is brittle. Prefer known INSTANCE_ID and poll until gone (with a cap).
- INSTANCE_ID=$(ibmcloud is instances --output json | jq -r --arg name "${INSTANCE_NAME}" '.[] | select(.name == $name) | .id')
+ if [ -n "${INSTANCE_ID:-}" ] && [ "${INSTANCE_ID}" != "null" ]; then
+ echo "Using provided INSTANCE_ID"
+ else
+ INSTANCE_ID=$(ibmcloud is instances --output json | jq -r --arg name "${INSTANCE_NAME}" '.[] | select(.name == $name) | .id' | head -n1)
+ fi
@@
- # Wait and verify deletion
- sleep 10
- VERIFY_ID=$(ibmcloud is instances --output json | jq -r --arg name "${INSTANCE_NAME}" '.[] | select(.name == $name) | .id' 2>/dev/null || echo "")
-
- if [ -z "${VERIFY_ID}" ] || [ "${VERIFY_ID}" = "null" ]; then
- echo "✅ Verified: VM has been completely removed"
- else
- echo "⚠️ VM deletion initiated but may still be in progress"
- fi
+ # Poll until deletion completes (max ~5 minutes)
+ for i in {1..30}; do
+ sleep 10
+ VERIFY_ID=$(ibmcloud is instances --output json | jq -r --arg id "${INSTANCE_ID}" '.[] | select(.id == $id) | .id' 2>/dev/null || echo "")
+ if [ -z "${VERIFY_ID}" ] || [ "${VERIFY_ID}" = "null" ]; then
+ echo "✅ Verified: VM has been completely removed"
+ break
+ fi
+ echo "⏳ Deletion in progress... (${i}/30)"
+ done📝 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.
| - name: Clean up IBM VM | |
| run: | | |
| echo "🔍 Looking for IBM VM to delete..." | |
| INSTANCE_ID=$(ibmcloud is instances --output json | jq -r --arg name "${INSTANCE_NAME}" '.[] | select(.name == $name) | .id') | |
| if [ -z "${INSTANCE_ID}" ] || [ "${INSTANCE_ID}" = "null" ]; then | |
| echo "⚠️ No VM found with name" | |
| echo "This might be expected if the VM was already deleted or never created." | |
| exit 0 | |
| fi | |
| echo "🔎 Found VM" | |
| echo "🗑️ Deleting IBM VM..." | |
| ibmcloud is instance-delete "${INSTANCE_ID}" --force >/dev/null | |
| echo "✅ IBM VM deleted successfully" | |
| # Wait and verify deletion | |
| sleep 10 | |
| VERIFY_ID=$(ibmcloud is instances --output json | jq -r --arg name "${INSTANCE_NAME}" '.[] | select(.name == $name) | .id' 2>/dev/null || echo "") | |
| if [ -z "${VERIFY_ID}" ] || [ "${VERIFY_ID}" = "null" ]; then | |
| echo "✅ Verified: VM has been completely removed" | |
| else | |
| echo "⚠️ VM deletion initiated but may still be in progress" | |
| fi | |
| - name: Clean up IBM VM | |
| run: | | |
| echo "🔍 Looking for IBM VM to delete..." | |
| if [ -n "${INSTANCE_ID:-}" ] && [ "${INSTANCE_ID}" != "null" ]; then | |
| echo "Using provided INSTANCE_ID" | |
| else | |
| INSTANCE_ID=$(ibmcloud is instances --output json | jq -r --arg name "${INSTANCE_NAME}" '.[] | select(.name == $name) | .id' | head -n1) | |
| fi | |
| if [ -z "${INSTANCE_ID}" ] || [ "${INSTANCE_ID}" = "null" ]; then | |
| echo "⚠️ No VM found with name" | |
| echo "This might be expected if the VM was already deleted or never created." | |
| exit 0 | |
| fi | |
| echo "🔎 Found VM" | |
| echo "🗑️ Deleting IBM VM..." | |
| ibmcloud is instance-delete "${INSTANCE_ID}" --force >/dev/null | |
| echo "✅ IBM VM deleted successfully" | |
| # Poll until deletion completes (max ~5 minutes) | |
| for i in {1..30}; do | |
| sleep 10 | |
| VERIFY_ID=$(ibmcloud is instances --output json | jq -r --arg id "${INSTANCE_ID}" '.[] | select(.id == $id) | .id' 2>/dev/null || echo "") | |
| if [ -z "${VERIFY_ID}" ] || [ "${VERIFY_ID}" = "null" ]; then | |
| echo "✅ Verified: VM has been completely removed" | |
| break | |
| fi | |
| echo "⏳ Deletion in progress... (${i}/30)" | |
| done |
| --obj-size <string> | ||
| set the object size to use for the tests. by default it is set to 1k. | ||
| example: 1k, 1m, 1g | ||
| --obj-randsize <boolean> | ||
| set to true to enable random object sizes. by default it is set to false. |
There was a problem hiding this comment.
🧩 Analysis chain
Usage text: good fix on --obj-size; ensure op name matches actual default.
Usage says “mix” (Line 17) but code defaults to “mixed” (Line 69). Align both to the valid Warp op.
🌐 Web query:
MinIO warp valid operations: is the mixed operation spelled "mix" or "mixed"?
💡 Result:
The operation is spelled "mixed" (e.g., warp mixed ...). [1][2]
Sources:
[1] GitHub - minio/warp README.
[2] pkg.go.dev description for github.com/minio/warp.
Use Warp op 'mixed' (not 'mix')
src/test/external_tests/warp/run_warp.js — change the usage text at line 17 from "mix" to "mixed" to match the code default at line 69 and the Warp CLI.
🤖 Prompt for AI Agents
In src/test/external_tests/warp/run_warp.js around lines 31 to 35 (and
specifically update the usage text at line 17), the usage string incorrectly
shows the Warp op as "mix" while the code default and Warp CLI expect "mixed";
change the usage/help text to use "mixed" instead of "mix" so it matches the
default at line 69 and the external CLI, and verify no other occurrences of
"mix" remain in help messages.
| const duration = argv.duration || '10m'; | ||
| const number_of_workers = argv.concurrency || DEFAULT_NUMBER_OF_WORKERS; | ||
| const disable_multipart = argv.disable_multipart || true; | ||
| const obj_randsize = argv.obj_randsize || false; |
There was a problem hiding this comment.
Boolean parsing for --obj-randsize is unsafe.
argv.obj_randsize || false treats the string "false" as truthy. Parse explicitly.
- const obj_randsize = argv.obj_randsize || false;
+ const parseBool = v => (v === true || v === 'true' || v === 1 || v === '1');
+ const obj_randsize = argv.obj_randsize !== undefined ? parseBool(argv.obj_randsize) : false;Also apply the same to disable_multipart (Line 77) so users can pass --disable-multipart false.
- const disable_multipart = argv.disable_multipart || true;
+ const disable_multipart = argv.disable_multipart !== undefined ? parseBool(argv.disable_multipart) : true;📝 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.
| const obj_randsize = argv.obj_randsize || false; | |
| const parseBool = v => (v === true || v === 'true' || v === 1 || v === '1'); | |
| const disable_multipart = argv.disable_multipart !== undefined ? parseBool(argv.disable_multipart) : true; | |
| const obj_randsize = argv.obj_randsize !== undefined ? parseBool(argv.obj_randsize) : false; |
🤖 Prompt for AI Agents
In src/test/external_tests/warp/run_warp.js around line 78 (and also line 77),
the flags are being set with loose truthiness (argv.obj_randsize || false and
argv.disable_multipart || false) which treats the string "false" as truthy;
change both to explicit boolean parsing: detect if argv.* is the string "true"
or "false" and convert to the corresponding boolean, otherwise preserve numeric
or other values as appropriate (e.g., if value is undefined fall back to false).
Ensure both obj_randsize and disable_multipart are assigned via this explicit
conversion so passing --obj-randsize false or --disable-multipart false behaves
correctly.
| // TODO - add --benchdata so that the result csv will be saved in logs | ||
| // const warp_logs_dir = WARP_TEST.warp_logs_dir_path; | ||
| const warp_command = `warp ${op} --host=localhost:${endpoint} --access-key=${access_key} --secret-key=${secret_key} --bucket=${bucket} --obj.size=${obj_size} --duration=${duration} --disable-multipart=${disable_multipart} --tls --insecure --concurrent ${number_of_workers}`; | ||
| const warp_command = `warp ${op} --host=localhost:${endpoint} --access-key=${access_key} --secret-key=${secret_key} --bucket=${bucket} --obj.size=${obj_size} --duration=${duration} --disable-multipart=${disable_multipart} --tls --insecure --concurrent ${number_of_workers} ${obj_randsize ? ' --obj.randsize' : ''}`; |
There was a problem hiding this comment.
Secrets leak: access_key/secret_key printed in logs.
console.info prints full command including credentials. Mask secrets in logs.
- const warp_command = `warp ${op} --host=localhost:${endpoint} --access-key=${access_key} --secret-key=${secret_key} --bucket=${bucket} --obj.size=${obj_size} --duration=${duration} --disable-multipart=${disable_multipart} --tls --insecure --concurrent ${number_of_workers} ${obj_randsize ? ' --obj.randsize' : ''}`;
+ const warp_command = `warp ${op} --host=localhost:${endpoint} --access-key=${access_key} --secret-key=${secret_key} --bucket=${bucket} --obj.size=${obj_size} --duration=${duration} --disable-multipart=${disable_multipart} --tls --insecure --concurrent ${number_of_workers}${obj_randsize ? ' --obj.randsize' : ''}`;
+ const masked_warp_command = `warp ${op} --host=localhost:${endpoint} --access-key=*** --secret-key=*** --bucket=${bucket} --obj.size=${obj_size} --duration=${duration} --disable-multipart=${disable_multipart} --tls --insecure --concurrent ${number_of_workers}${obj_randsize ? ' --obj.randsize' : ''}`;
@@
- console.info(`Running warp ${warp_command}`);
+ console.info(`Running warp ${masked_warp_command}`);📝 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.
| const warp_command = `warp ${op} --host=localhost:${endpoint} --access-key=${access_key} --secret-key=${secret_key} --bucket=${bucket} --obj.size=${obj_size} --duration=${duration} --disable-multipart=${disable_multipart} --tls --insecure --concurrent ${number_of_workers} ${obj_randsize ? ' --obj.randsize' : ''}`; | |
| const warp_command = `warp ${op} --host=localhost:${endpoint} --access-key=${access_key} --secret-key=${secret_key} --bucket=${bucket} --obj.size=${obj_size} --duration=${duration} --disable-multipart=${disable_multipart} --tls --insecure --concurrent ${number_of_workers}${obj_randsize ? ' --obj.randsize' : ''}`; | |
| const masked_warp_command = `warp ${op} --host=localhost:${endpoint} --access-key=*** --secret-key=*** --bucket=${bucket} --obj.size=${obj_size} --duration=${duration} --disable-multipart=${disable_multipart} --tls --insecure --concurrent ${number_of_workers}${obj_randsize ? ' --obj.randsize' : ''}`; | |
| console.info(`Running warp ${masked_warp_command}`); |
🤖 Prompt for AI Agents
In src/test/external_tests/warp/run_warp.js around line 92, the constructed warp
command string includes access_key and secret_key which are then printed,
leaking secrets; modify logging so secrets are not logged by creating a separate
log-safe command (e.g., replace access_key and secret_key values with masked
placeholders like **** or remove those flags) and use that for console.info,
while keeping the original command (or set credentials via env vars) for
execution; ensure any further logging uses the masked version and do not write
the real keys to stdout/stderr.
tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh
Outdated
Show resolved
Hide resolved
Neon-White
force-pushed
the
nightly-warp
branch
from
ffb8b50 to
792b094
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh (2)
3-3: Source of /etc/warp.env without ownership/perm checks → RCE risk.-set -a; . /etc/warp.env; set +a +if [ -f /etc/warp.env ]; then + owngrp=$(stat -c '%U:%G' /etc/warp.env) || { echo "stat failed"; exit 1; } + mode=$(stat -c '%a' /etc/warp.env) + case "$owngrp $mode" in + "root:root 600"|"root:root 640") ;; + *) echo "/etc/warp.env has insecure ownership/permissions ($owngrp $mode)"; exit 1;; + esac + set -a; . /etc/warp.env; set +a +else + echo "/etc/warp.env not found"; exit 1 +fi
24-32: set -x leaks Slack webhook; guard and disable xtrace around notifier.send_failure_notification() { local msg="$1" - node /usr/local/bin/slack_notifier.js "${SLACK_NIGHTLY_RESULTS_URL}" failure "Warp failed: $msg" || true + if [ -n "${SLACK_NIGHTLY_RESULTS_URL:-}" ]; then + set +x + node /usr/local/bin/slack_notifier.js "${SLACK_NIGHTLY_RESULTS_URL}" failure "Warp failed: $msg" || true + set -x + else + echo "SLACK_NIGHTLY_RESULTS_URL not set; skipping failure notification" + fi } send_success_notification() { local log_location="$1" - node /usr/local/bin/slack_notifier.js "${SLACK_NIGHTLY_RESULTS_URL}" success "Warp success - logs available at: $log_location" || true + if [ -n "${SLACK_NIGHTLY_RESULTS_URL:-}" ]; then + set +x + node /usr/local/bin/slack_notifier.js "${SLACK_NIGHTLY_RESULTS_URL}" success "Warp success - logs available at: $log_location" || true + set -x + else + echo "SLACK_NIGHTLY_RESULTS_URL not set; skipping success notification" + fi }
🧹 Nitpick comments (9)
docs/CI & Tests/warp-on-ibm.md (4)
58-68: Add a language to the fenced diagram block (MD040).-``` +```text ┌─────────────────┐ ┌─────────────────┐ ┌─────────────────┐ │ Provision │ │ Test & Log │ │ Cleanup │ │ (Midnight) │───▶│ (3+ hours) │───▶│ (4 AM) │ └─────────────────┘ └─────────────────┘ └─────────────────┘ │ │ │ ▼ ▼ ▼ IBM Cloud VM Warp tests Resource cleanup Floating IP Log upload VM termination Slack notification--- `90-93`: **Fix unordered list indentation (MD007).** ```diff - - The required packages that'll be installed on the VM - - Additional setup commands - - An environment file (`warp.env`) that is used to pass env vars from GitHub to the runner (via `envsubst` variable templating) - - The 'entrypoint' (that is the last command that is run by cloud-init; in our case, the Warp scripts) +- The required packages that'll be installed on the VM +- Additional setup commands +- An environment file (`warp.env`) that is used to pass env vars from GitHub to the runner (via `envsubst` variable templating) +- The 'entrypoint' (that is the last command that is run by cloud-init; in our case, the Warp scripts)
114-114: Remove trailing colon in heading (MD026).-## Log locations: +## Log locations
138-144: Avoid leaking secrets in shell history during local debugging.Add a note to use a subshell with history disabled or read -s for secrets.
1. **Export required environment variables** locally from the "Required Secrets" section: ```bash - export IBM_CLOUD_API_KEY="your-api-key" - export IBM_WARP_VM_CONFIG='{"INSTANCE_NAME": "example-name", ...}' + # Tip: prevent storing secrets in history + ( set +o history + export IBM_CLOUD_API_KEY="your-api-key" + export IBM_WARP_VM_CONFIG='{"INSTANCE_NAME": "example-name", ...}' + # ... other required variables + ) # ... other required variables ```tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh (5)
2-2: Make tracing opt-in; default off in CI to avoid secret spills.-set -euxo pipefail +set -euo pipefail +# Enable xtrace only if TRACE=1 +[ "${TRACE:-0}" = "1" ] && set -x
14-17: SC2155: assign separately; also quote and use printf for timestamp.-local timestamp=$(date +"%Y-%m-%d_%H-%M-%S") -local log_dir="warp-test-logs-${timestamp}" +local timestamp +timestamp="$(date '+%Y-%m-%d_%H-%M-%S')" +local log_dir +log_dir="warp-test-logs-${timestamp}"
39-41: Improve ERR trap context (exit code and failing command).-trap 'handle_failure "line $LINENO exit $?"' ERR +trap 'rc=$?; line=$LINENO; cmd=$BASH_COMMAND; handle_failure "cmd: \"$cmd\" at line $line exit $rc"' ERR
51-53: Honor WARP_ARGS override; keep sensible defaults.-echo "Running Warp tests..." -make test-warp -o tester WARP_ARGS="--duration 3h --obj-size 10m --obj-randsize" +echo "Running Warp tests..." +DEFAULT_WARP_ARGS="--duration 3h --obj-size 10m --obj-randsize" +make test-warp -o tester WARP_ARGS="${WARP_ARGS:-$DEFAULT_WARP_ARGS}"
18-21: Quiet AWS CLI logs; fail fast on upload errors.-aws s3 cp "$logs_path" "s3://${WARP_LOGS_BUCKET}/${log_dir}/" --recursive --no-progress --endpoint-url "${IBM_COS_ENDPOINT}" +aws s3 cp "$logs_path" "s3://${WARP_LOGS_BUCKET}/${log_dir}/" \ + --recursive --no-progress --only-show-errors \ + --endpoint-url "${IBM_COS_ENDPOINT}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/ibm-warp-runner-config.yaml(1 hunks).github/workflows/ibm-nightly-warp-vm-cleanup.yaml(1 hunks).github/workflows/ibm-nightly-warp-vm-provision.yaml(1 hunks)Makefile(1 hunks)docs/CI & Tests/warp-on-ibm.md(1 hunks)src/test/external_tests/warp/run_nc_warp_on_test_container.sh(1 hunks)src/test/external_tests/warp/run_warp.js(3 hunks)src/test/external_tests/warp/run_warp_on_test_container.sh(1 hunks)tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh(1 hunks)tools/ibm_runner_helpers/slack_notifier.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/test/external_tests/warp/run_nc_warp_on_test_container.sh
- .github/workflows/ibm-nightly-warp-vm-provision.yaml
- src/test/external_tests/warp/run_warp_on_test_container.sh
- .github/workflows/ibm-nightly-warp-vm-cleanup.yaml
- tools/ibm_runner_helpers/slack_notifier.js
- .github/ibm-warp-runner-config.yaml
- src/test/external_tests/warp/run_warp.js
- Makefile
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/CI & Tests/warp-on-ibm.md
58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
90-90: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
91-91: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
92-92: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
93-93: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
114-114: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🪛 Shellcheck (0.10.0)
tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh
[warning] 14-14: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-package-lock-validation
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
Neon-White
force-pushed
the
nightly-warp
branch
from
792b094 to
1ae6211
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh (1)
12-20: Guard Slack notifier when webhook is unset (avoid noisy failures).Skip notifier calls if
SLACK_NIGHTLY_RESULTS_URLis empty.send_failure_notification() { local msg="$1" - node /usr/local/bin/slack_notifier.js "${SLACK_NIGHTLY_RESULTS_URL}" failure "Warp failed: ${msg}" || true + if [ -n "${SLACK_NIGHTLY_RESULTS_URL:-}" ]; then + node /usr/local/bin/slack_notifier.js "${SLACK_NIGHTLY_RESULTS_URL}" failure "Warp failed: ${msg}" || true + else + echo "SLACK_NIGHTLY_RESULTS_URL not set; skipping failure notification" >&2 + fi } @@ send_success_notification() { local log_location="$1" - node /usr/local/bin/slack_notifier.js "${SLACK_NIGHTLY_RESULTS_URL}" success "Warp success - logs available at: ${log_location}" || true + if [ -n "${SLACK_NIGHTLY_RESULTS_URL:-}" ]; then + node /usr/local/bin/slack_notifier.js "${SLACK_NIGHTLY_RESULTS_URL}" success "Warp success - logs available at: ${log_location}" || true + else + echo "SLACK_NIGHTLY_RESULTS_URL not set; skipping success notification" >&2 + fi }
🧹 Nitpick comments (11)
docs/CI & Tests/warp-on-ibm.md (6)
226-227: Fix visible filename to include .yaml for consistency.Link target already ends with .yaml; update the visible text to match.
-**File**: [`.github/workflows/ibm-nightly-warp-vm-cleanup`](../../.github/workflows/ibm-nightly-warp-vm-cleanup.yaml) +**File**: [`.github/workflows/ibm-nightly-warp-vm-cleanup.yaml`](../../.github/workflows/ibm-nightly-warp-vm-cleanup.yaml)
69-80: Add a language to the fenced ASCII diagram.Satisfies MD040; use a neutral lexer.
-``` +```text ┌─────────────────┐ ┌─────────────────┐ ┌─────────────────┐ │ Provision │ │ Test & Log │ │ Cleanup │ │ (Midnight) │───▶│ (3+ hours) │───▶│ (4 AM) │ └─────────────────┘ └─────────────────┘ └─────────────────┘ │ │ │ ▼ ▼ ▼ IBM Cloud VM Warp tests Resource cleanup Floating IP Log upload VM termination Slack notification--- `23-29`: **Normalize table pipe style (leading_and_trailing).** Addresses MD055; ensure each row ends with a trailing pipe. ```diff -| Secret Name | Description | -|-------------|-------------| -| `IBM_CLOUD_API_KEY` | IBM Cloud API key with VPC infrastructure permissions | -| `SLACK_NIGHTLY_RESULTS_URL` | Slack webhook URL for nightly test result notifications | -| `IBM_COS_WRITER_CREDENTIALS` | JSON configuration for IBM Cloud Object Storage access (see below) -| `IBM_WARP_VM_CONFIG` | JSON configuration for VM provisioning (see below) | +| Secret Name | Description | +|-------------|-------------| +| `IBM_CLOUD_API_KEY` | IBM Cloud API key with VPC infrastructure permissions | +| `SLACK_NIGHTLY_RESULTS_URL` | Slack webhook URL for nightly test result notifications | +| `IBM_COS_WRITER_CREDENTIALS` | JSON configuration for IBM Cloud Object Storage access (see below) | +| `IBM_WARP_VM_CONFIG` | JSON configuration for VM provisioning (see below) |
260-260: Remove trailing colon from heading.Fixes MD026.
-## Log locations: +## Log locations
236-239: Fix unordered list indentation to satisfy MD007.Outdent list items under “The config includes”.
- - The required packages that'll be installed on the VM - - Additional setup commands - - An environment file (`warp.env`) that is used to pass env vars from GitHub to the runner (via `envsubst` variable templating) - - The 'entrypoint' (that is the last command that is run by cloud-init; in our case, the Warp scripts) +- The required packages that'll be installed on the VM +- Additional setup commands +- An environment file (`warp.env`) that is used to pass env vars from GitHub to the runner (via `envsubst` variable templating) +- The 'entrypoint' (that is the last command that is run by cloud-init; in our case, the Warp scripts)
135-138: Document restrictive permissions of /etc/warp.env for clarity.Small note improves reader confidence about secret handling.
- - Creates `/etc/warp.env` with environment variables from GitHub secrets + - Creates `/etc/warp.env` (mode `0600`) with environment variables from GitHub secretstools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh (5)
22-27: Ensure aws cp uploads directory contents (not the parent dir).Add a trailing slash to the source path for clarity/consistency.
-aws s3 cp "${test_logs_path}" "s3://${WARP_LOGS_BUCKET}/${log_dir}/" --recursive --no-progress --endpoint-url "${IBM_COS_ENDPOINT}" +aws s3 cp "${test_logs_path}/" "s3://${WARP_LOGS_BUCKET}/${log_dir}/" --recursive --no-progress --endpoint-url "${IBM_COS_ENDPOINT}"
29-34: Upload more diagnostics on failure and make uploads best-effort.Include cloud-init.log and any Warp logs if present; don’t let upload errors mask the original failure.
handle_failure() { echo "Uploading cloud-init logs to IBM COS s3://${WARP_LOGS_BUCKET}/${log_dir}/" - aws s3 cp "/var/log/cloud-init-output.log" "s3://${WARP_LOGS_BUCKET}/${log_dir}/" --no-progress --endpoint-url "${IBM_COS_ENDPOINT}" + aws s3 cp "/var/log/cloud-init-output.log" "s3://${WARP_LOGS_BUCKET}/${log_dir}/" --no-progress --endpoint-url "${IBM_COS_ENDPOINT}" || true + [ -f "/var/log/cloud-init.log" ] && aws s3 cp "/var/log/cloud-init.log" "s3://${WARP_LOGS_BUCKET}/${log_dir}/" --no-progress --endpoint-url "${IBM_COS_ENDPOINT}" || true + [ -d "${test_logs_path}" ] && aws s3 cp "${test_logs_path}/" "s3://${WARP_LOGS_BUCKET}/${log_dir}/warp-logs/" --recursive --no-progress --endpoint-url "${IBM_COS_ENDPOINT}" || true echo "Successfully uploaded cloud-init logs to IBM COS s3://${WARP_LOGS_BUCKET}/${log_dir}/" send_failure_notification "Run failed, logs available at s3://${WARP_LOGS_BUCKET}/${log_dir}/" }
41-42: Improve ERR trap context (command, line, exit code).Current message may not show the failing command or accurate line.
-trap 'handle_failure "line $LINENO exit $?"' ERR +trap 'rc=$?; line=$LINENO; cmd=$BASH_COMMAND; handle_failure "cmd=\"$cmd\" at line $line (exit $rc)"' ERR trap 'shutdown_vm' EXIT
50-51: Tighten permissions on logs directory.Use group-readable, not world-readable.
-chmod 755 logs/warp-test-logs +chmod 750 logs/warp-test-logs
53-55: Optional: allow overriding WARP_ARGS via env with a sensible default.Keeps current defaults but enables Makefile/secret-driven overrides.
-echo "Running Warp tests..." -make test-warp -o tester WARP_ARGS="--duration 3h --obj-size 10m --obj-randsize" +echo "Running Warp tests..." +: "${WARP_ARGS:=--duration 3h --obj-size 10m --obj-randsize}" +make test-warp -o tester WARP_ARGS="${WARP_ARGS}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/ibm-warp-runner-config.yaml(1 hunks).github/workflows/ibm-nightly-warp-vm-cleanup.yaml(1 hunks).github/workflows/ibm-nightly-warp-vm-provision.yaml(1 hunks)Makefile(1 hunks)docs/CI & Tests/warp-on-ibm.md(1 hunks)src/test/external_tests/warp/run_nc_warp_on_test_container.sh(1 hunks)src/test/external_tests/warp/run_warp.js(3 hunks)src/test/external_tests/warp/run_warp_on_test_container.sh(1 hunks)tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh(1 hunks)tools/ibm_runner_helpers/slack_notifier.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/test/external_tests/warp/run_warp_on_test_container.sh
- src/test/external_tests/warp/run_nc_warp_on_test_container.sh
- .github/workflows/ibm-nightly-warp-vm-provision.yaml
- tools/ibm_runner_helpers/slack_notifier.js
- .github/ibm-warp-runner-config.yaml
- src/test/external_tests/warp/run_warp.js
- Makefile
- .github/workflows/ibm-nightly-warp-vm-cleanup.yaml
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.032Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with mode 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically, so additional permission checks before sourcing are not necessary in this context.
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.033Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with permissions 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically by cloud-init, so additional permission checks before sourcing are not necessary in this context.
📚 Learning: 2025-09-18T07:12:14.280Z
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:2-5
Timestamp: 2025-09-18T07:12:14.280Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, debug logs with `set -x` are kept unmasked even when containing webhook URLs or other secrets because the logs are only accessible within the provisioned VM by permitted team members, and the debugging value outweighs secret exposure concerns in this controlled internal environment.
Applied to files:
tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.shdocs/CI & Tests/warp-on-ibm.md
📚 Learning: 2025-09-18T07:11:55.032Z
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.032Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with mode 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically, so additional permission checks before sourcing are not necessary in this context.
Applied to files:
tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.shdocs/CI & Tests/warp-on-ibm.md
📚 Learning: 2025-09-18T07:11:55.033Z
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.033Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with permissions 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically by cloud-init, so additional permission checks before sourcing are not necessary in this context.
Applied to files:
tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.shdocs/CI & Tests/warp-on-ibm.md
🪛 markdownlint-cli2 (0.17.2)
docs/CI & Tests/warp-on-ibm.md
27-27: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
69-69: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
236-236: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
237-237: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
238-238: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
239-239: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
260-260: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
- GitHub Check: Build Noobaa Image
🔇 Additional comments (1)
tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh (1)
3-3: Acknowledged: sourcing warp.env early is correct in this controlled flow.Given cloud-init creates
/etc/warp.envwith mode 0600, sourcing it directly is acceptable here.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/ibm-nightly-warp-vm-cleanup.yaml (2)
28-36: Harden JSON→env export (handles equals/newlines; valid keys).Prevents malformed env and preserves values. This was raised before; applying here too.
- # Mask non-empty values - jq -r 'to_entries[] | select(.value != "") | "::add-mask::\(.value)"' <<<"${IBM_WARP_VM_CONFIG}" - # Append KEY=VALUE lines to GITHUB_ENV - jq -r 'to_entries[] | "\(.key)=\(.value)"' <<<"${IBM_WARP_VM_CONFIG}" >>"$GITHUB_ENV" + # Only accept shell-safe keys; write using multiline heredoc to preserve content + while IFS= read -r key; do + val="$(jq -r --arg k "$key" '.[$k]' <<<"${IBM_WARP_VM_CONFIG}")" + [ -n "${val}" ] && echo "::add-mask::${val}" + # Sanity check key + sane_key="$(printf '%s' "$key" | sed -E 's/[^A-Za-z0-9_]/_/g; s/^([0-9])/_\1/')" + { + printf '%s<<__EOV__\n' "$sane_key" + printf '%s\n' "$val" + printf '__EOV__\n' + } >> "$GITHUB_ENV" + done < <(jq -r 'keys[]' <<<"${IBM_WARP_VM_CONFIG}")
45-61: Prefer known IDs when available; fall back to tag search.Safer cleanup when duplicates exist. This was suggested earlier as well.
- FLOATING_IP_IDS=$(ibmcloud resource search "tags:\"${RESOURCE_TAG}\" AND type:floating-ip" --output JSON | jq -r '.items[].resource_id' 2>/dev/null || echo "") + if [ -n "${FLOATING_IP_IDS:-}" ]; then + echo "Using provided FLOATING_IP_IDS" + else + FLOATING_IP_IDS=$(ibmcloud resource search "tags:\"${RESOURCE_TAG}\" AND type:floating-ip" --output JSON | jq -r '.items[].resource_id' 2>/dev/null || echo "") + fi @@ - for IP_ID in ${FLOATING_IP_IDS} - do - ibmcloud is floating-ip-release "${IP_ID}" --force >/dev/null - done + readarray -t FIP_ARR <<<"${FLOATING_IP_IDS}" + for IP_ID in "${FIP_ARR[@]}"; do + [ -n "${IP_ID}" ] || continue + ibmcloud is floating-ip-release "${IP_ID}" --force >/dev/null + doneApply similar “prefer IDs” pattern for INSTANCE_IDS if you capture them during provisioning.
🧹 Nitpick comments (9)
docs/CI & Tests/warp-on-ibm.md (8)
23-29: Fix markdown table pipe style.Add trailing pipes to satisfy MD055 and render consistently.
-| Secret Name | Description | -|-------------|-------------| -| `IBM_CLOUD_API_KEY` | IBM Cloud API key with VPC infrastructure permissions | -| `SLACK_NIGHTLY_RESULTS_URL` | Slack webhook URL for nightly test result notifications | -| `IBM_COS_WRITER_CREDENTIALS` | JSON configuration for IBM Cloud Object Storage access (see below) -| `IBM_WARP_VM_CONFIG` | JSON configuration for VM provisioning (see below) | +| Secret Name | Description | +|-------------|-------------| +| `IBM_CLOUD_API_KEY` | IBM Cloud API key with VPC infrastructure permissions | +| `SLACK_NIGHTLY_RESULTS_URL` | Slack webhook URL for nightly test result notifications | +| `IBM_COS_WRITER_CREDENTIALS` | JSON configuration for IBM Cloud Object Storage access (see below) | +| `IBM_WARP_VM_CONFIG` | JSON configuration for VM provisioning (see below) |
70-80: Add language to fenced ASCII diagram.Set a language to satisfy MD040.
-``` +```text ...--- `237-241`: **Normalize bullet indentation.** Fix MD007 by removing the leading space. ```diff - - The required packages that'll be installed on the VM - - Additional setup commands - - An environment file (`warp.env`) that is used to pass env vars from GitHub to the runner (via `envsubst` variable templating) - - The 'entrypoint' (that is the last command that is run by cloud-init; in our case, the Warp scripts) +- The required packages that'll be installed on the VM +- Additional setup commands +- An environment file (`warp.env`) that is used to pass env vars from GitHub to the runner (via `envsubst` variable templating) +- The 'entrypoint' (that is the last command that is run by cloud-init; in our case, the Warp scripts)
261-261: Remove trailing colon from heading.Satisfy MD026.
-## Log locations: +## Log locations
221-229: Align link label with filename.Minor nit: include “.yaml” in the link text for consistency.
-**File**: [`.github/workflows/ibm-nightly-warp-vm-cleanup`](../../.github/workflows/ibm-nightly-warp-vm-cleanup.yaml) +**File**: [`.github/workflows/ibm-nightly-warp-vm-cleanup.yaml`](../../.github/workflows/ibm-nightly-warp-vm-cleanup.yaml)
136-139: Document warp.env permissions and rationale.Clarify that cloud-init writes /etc/warp.env with mode 0600; aligns with our internal flow. Based on retrieved learnings.
-2. **Environment Configuration** - - Creates `/etc/warp.env` with environment variables from GitHub secrets - - Sets up Node.js path for Slack webhook functionality +2. **Environment Configuration** + - Creates `/etc/warp.env` (permissions 0600) with environment variables from GitHub secrets + - Sets up Node.js path for Slack webhook functionality
167-173: Mention WARP_ARGS override and Makefile wrapper.Add a note showing how to customize params locally and in CI.
- Captures all test output and metrics + +Note: You can override defaults via the Makefile wrapper that preserves argument expansion: + +```bash +make test-warp WARP_ARGS="--duration 1m --obj-size 1KB" +```
292-298: Correct cloud-init SSH example structure.ssh_authorized_keys typically lives under users. Suggest a safer snippet.
-ssh_authorized_keys: - - ssh-rsa AAAA... your-public-key +users: + - name: ubuntu + ssh_authorized_keys: + - ssh-rsa AAAA... your-public-key.github/workflows/ibm-nightly-warp-vm-cleanup.yaml (1)
63-63: Quote expansions to satisfy SC2086.Double-quote variable expansions and iterate over arrays to avoid word-splitting/globbing.
No additional diff beyond the array-based loops proposed in prior comments is needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/ibm-nightly-warp-vm-cleanup.yaml(1 hunks).github/workflows/ibm-nightly-warp-vm-provision.yaml(1 hunks)docs/CI & Tests/warp-on-ibm.md(1 hunks)tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ibm-nightly-warp-vm-provision.yaml
- tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.032Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with mode 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically, so additional permission checks before sourcing are not necessary in this context.
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.033Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with permissions 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically by cloud-init, so additional permission checks before sourcing are not necessary in this context.
📚 Learning: 2025-09-18T07:11:55.032Z
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.032Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with mode 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically, so additional permission checks before sourcing are not necessary in this context.
Applied to files:
docs/CI & Tests/warp-on-ibm.md
📚 Learning: 2025-09-18T07:11:55.033Z
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.033Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with permissions 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically by cloud-init, so additional permission checks before sourcing are not necessary in this context.
Applied to files:
docs/CI & Tests/warp-on-ibm.md
📚 Learning: 2025-09-18T07:12:14.280Z
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:2-5
Timestamp: 2025-09-18T07:12:14.280Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, debug logs with `set -x` are kept unmasked even when containing webhook URLs or other secrets because the logs are only accessible within the provisioned VM by permitted team members, and the debugging value outweighs secret exposure concerns in this controlled internal environment.
Applied to files:
docs/CI & Tests/warp-on-ibm.md
🪛 actionlint (1.7.7)
.github/workflows/ibm-nightly-warp-vm-cleanup.yaml
63-63: shellcheck reported issue in this script: SC2086:info:25:5: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 markdownlint-cli2 (0.17.2)
docs/CI & Tests/warp-on-ibm.md
27-27: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
70-70: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
237-237: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
238-238: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
239-239: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
240-240: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
261-261: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
| - name: Clean up IBM VMs | ||
| run: | | ||
| echo "🔍 Looking for IBM VMs with the resource tag" | ||
| INSTANCE_IDS=$(ibmcloud resource search "tags:\"${RESOURCES_TAG}\" AND type:instance" --output JSON | jq -r '.items[].resource_id' 2>/dev/null || echo "") | ||
|
|
||
| if [ -z "${INSTANCE_IDS}" ] | ||
| then | ||
| echo "ℹ️ No VMs found with the resource tag" | ||
| echo "This might be expected if the VMs were already deleted or never created." | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "🔎 Found VM(s) with the resource tag" | ||
| echo "🗑️ Deleting IBM VM(s)..." | ||
| for INSTANCE_ID in ${INSTANCE_IDS} | ||
| do | ||
| ibmcloud is instance-delete "${INSTANCE_ID}" --force >/dev/null | ||
| done | ||
| echo "✅ All tagged IBM VMs deleted successfully" | ||
|
|
||
| # Wait up to 1 minute for deletion to complete | ||
| for i in {1..12} | ||
| do | ||
| sleep 5 | ||
| REMAINING=$(ibmcloud resource search "tags:\"${RESOURCES_TAG}\" AND type:instance" --output JSON | jq -r '.items[].resource_id' 2>/dev/null || echo "") | ||
| [ -z "${REMAINING}" ] && echo "✅ Deletion verified" && break | ||
| [ $i -eq 12 ] && echo "⚠️ Some VMs may still be deleting" | ||
| done | ||
|
|
There was a problem hiding this comment.
Use RESOURCE_TAG consistently and improve deletion verification.
Align variable name and verify deletion against specific IDs.
- echo "🔍 Looking for IBM VMs with the resource tag"
- INSTANCE_IDS=$(ibmcloud resource search "tags:\"${RESOURCES_TAG}\" AND type:instance" --output JSON | jq -r '.items[].resource_id' 2>/dev/null || echo "")
+ echo "🔍 Looking for IBM VMs with the resource tag: ${RESOURCE_TAG}"
+ INSTANCE_IDS=$(ibmcloud resource search "tags:\"${RESOURCE_TAG}\" AND type:instance" --output JSON | jq -r '.items[].resource_id' 2>/dev/null || echo "")
@@
- for INSTANCE_ID in ${INSTANCE_IDS}
- do
- ibmcloud is instance-delete "${INSTANCE_ID}" --force >/dev/null
- done
+ readarray -t INSTANCE_ARR <<<"${INSTANCE_IDS}"
+ for INSTANCE_ID in "${INSTANCE_ARR[@]}"; do
+ [ -n "${INSTANCE_ID}" ] || continue
+ ibmcloud is instance-delete "${INSTANCE_ID}" --force >/dev/null
+ done
@@
- for i in {1..12}
- do
- sleep 5
- REMAINING=$(ibmcloud resource search "tags:\"${RESOURCES_TAG}\" AND type:instance" --output JSON | jq -r '.items[].resource_id' 2>/dev/null || echo "")
- [ -z "${REMAINING}" ] && echo "✅ Deletion verified" && break
- [ $i -eq 12 ] && echo "⚠️ Some VMs may still be deleting"
- done
+ # Poll up to 3 minutes for each instance ID to disappear
+ for INSTANCE_ID in "${INSTANCE_ARR[@]}"; do
+ [ -n "${INSTANCE_ID}" ] || continue
+ for i in {1..36}; do
+ sleep 5
+ # Consider it gone when lookup by ID returns empty
+ STILL_THERE=$(ibmcloud is instance "${INSTANCE_ID}" --output json 2>/dev/null | jq -r '.id // empty')
+ if [ -z "${STILL_THERE}" ]; then
+ echo "✅ Deletion verified for ${INSTANCE_ID}"
+ break
+ fi
+ [ $i -eq 36 ] && echo "⚠️ ${INSTANCE_ID} may still be deleting"
+ done
+ doneAlso addresses actionlint SC2086 by quoting expansions and using arrays.
📝 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.
| - name: Clean up IBM VMs | |
| run: | | |
| echo "🔍 Looking for IBM VMs with the resource tag" | |
| INSTANCE_IDS=$(ibmcloud resource search "tags:\"${RESOURCES_TAG}\" AND type:instance" --output JSON | jq -r '.items[].resource_id' 2>/dev/null || echo "") | |
| if [ -z "${INSTANCE_IDS}" ] | |
| then | |
| echo "ℹ️ No VMs found with the resource tag" | |
| echo "This might be expected if the VMs were already deleted or never created." | |
| exit 0 | |
| fi | |
| echo "🔎 Found VM(s) with the resource tag" | |
| echo "🗑️ Deleting IBM VM(s)..." | |
| for INSTANCE_ID in ${INSTANCE_IDS} | |
| do | |
| ibmcloud is instance-delete "${INSTANCE_ID}" --force >/dev/null | |
| done | |
| echo "✅ All tagged IBM VMs deleted successfully" | |
| # Wait up to 1 minute for deletion to complete | |
| for i in {1..12} | |
| do | |
| sleep 5 | |
| REMAINING=$(ibmcloud resource search "tags:\"${RESOURCES_TAG}\" AND type:instance" --output JSON | jq -r '.items[].resource_id' 2>/dev/null || echo "") | |
| [ -z "${REMAINING}" ] && echo "✅ Deletion verified" && break | |
| [ $i -eq 12 ] && echo "⚠️ Some VMs may still be deleting" | |
| done | |
| - name: Clean up IBM VMs | |
| run: | | |
| echo "🔍 Looking for IBM VMs with the resource tag: ${RESOURCE_TAG}" | |
| INSTANCE_IDS=$(ibmcloud resource search "tags:\"${RESOURCE_TAG}\" AND type:instance" --output JSON | jq -r '.items[].resource_id' 2>/dev/null || echo "") | |
| if [ -z "${INSTANCE_IDS}" ] | |
| then | |
| echo "ℹ️ No VMs found with the resource tag" | |
| echo "This might be expected if the VMs were already deleted or never created." | |
| exit 0 | |
| fi | |
| echo "🔎 Found VM(s) with the resource tag" | |
| echo "🗑️ Deleting IBM VM(s)..." | |
| readarray -t INSTANCE_ARR <<<"${INSTANCE_IDS}" | |
| for INSTANCE_ID in "${INSTANCE_ARR[@]}"; do | |
| [ -n "${INSTANCE_ID}" ] || continue | |
| ibmcloud is instance-delete "${INSTANCE_ID}" --force >/dev/null | |
| done | |
| echo "✅ All tagged IBM VMs deleted successfully" | |
| # Poll up to 3 minutes for each instance ID to disappear | |
| for INSTANCE_ID in "${INSTANCE_ARR[@]}"; do | |
| [ -n "${INSTANCE_ID}" ] || continue | |
| for i in {1..36}; do | |
| sleep 5 | |
| # Consider it gone when lookup by ID returns empty | |
| STILL_THERE=$(ibmcloud is instance "${INSTANCE_ID}" --output json 2>/dev/null | jq -r '.id // empty') | |
| if [ -z "${STILL_THERE}" ]; then | |
| echo "✅ Deletion verified for ${INSTANCE_ID}" | |
| break | |
| fi | |
| [ $i -eq 36 ] && echo "⚠️ ${INSTANCE_ID} may still be deleting" | |
| done | |
| done |
🧰 Tools
🪛 actionlint (1.7.7)
63-63: shellcheck reported issue in this script: SC2086:info:25:5: Double quote to prevent globbing and word splitting
(shellcheck)
🤖 Prompt for AI Agents
.github/workflows/ibm-nightly-warp-vm-cleanup.yaml lines 62-90: rename the
environment variable and local var from RESOURCES_TAG to RESOURCE_TAG for
consistency, capture INSTANCE_IDS into a bash array (e.g. readarray -t
INSTANCE_IDS <<< "$(ibmcloud resource search "tags:\"${RESOURCE_TAG}\" AND
type:instance" --output JSON | jq -r '.items[].resource_id' 2>/dev/null)") to
avoid word-splitting, quote expansions everywhere (e.g. "${INSTANCE_IDS[@]}" and
"${INSTANCE_ID}") to satisfy actionlint SC2086, delete each instance with a
quoted id and --force, then verify deletion by checking each specific
INSTANCE_ID no longer appears (re-run the search filtering for that resource_id
or check ibmcloud is instance-get "${INSTANCE_ID}" and treat non-zero/404 as
deleted) rather than relying on the tag-only global search; also handle empty
array case and exit 0 when no IDs found.
Signed-off-by: Ben <[email protected]>
Add tags to VSIs and FIPs Modify cleanup flow to rely on tags instead of names Format shell script Signed-off-by: Ben <[email protected]>
Signed-off-by: Ben <[email protected]>
Signed-off-by: Ben <[email protected]>
Neon-White
force-pushed
the
nightly-warp
branch
from
782e9e9 to
f56f4aa
Compare
Signed-off-by: Ben <[email protected]>
Signed-off-by: Ben <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh (2)
31-36: Consider uploading partial test logs on failure for better debugging.Currently,
handle_failureonly uploads/var/log/cloud-init-output.log. If the Warp tests fail partway through, partial logs in${test_logs_path}would aid debugging but are not uploaded.Consider uploading both cloud-init logs and any available test logs:
handle_failure() { echo "Uploading cloud-init logs to IBM COS s3://${WARP_LOGS_BUCKET}/${log_dir}/" aws s3 cp "/var/log/cloud-init-output.log" "s3://${WARP_LOGS_BUCKET}/${log_dir}/" --no-progress --endpoint-url "${IBM_COS_ENDPOINT}" + if [ -d "${test_logs_path}" ] && [ -n "$(ls -A "${test_logs_path}")" ]; then + echo "Uploading partial test logs to IBM COS s3://${WARP_LOGS_BUCKET}/${log_dir}/" + aws s3 cp "${test_logs_path}" "s3://${WARP_LOGS_BUCKET}/${log_dir}/" --recursive --no-progress --endpoint-url "${IBM_COS_ENDPOINT}" || true + fi echo "Successfully uploaded cloud-init logs to IBM COS s3://${WARP_LOGS_BUCKET}/${log_dir}/" send_failure_notification "Run failed, logs available at s3://${WARP_LOGS_BUCKET}/${log_dir}/" }
55-56: Consider making Warp test parameters configurable.The test parameters are currently hardcoded. While this aligns with the documented gap in the PR objectives, making these configurable via environment variables would improve flexibility for different test scenarios.
Consider sourcing parameters from environment variables with defaults:
echo "Running Warp tests..." -make test-warp -o tester WARP_ARGS="--duration 3h --obj-size 10m --obj-randsize" +make test-warp -o tester WARP_ARGS="${WARP_TEST_PARAMS:---duration 3h --obj-size 10m --obj-randsize}"Then define
WARP_TEST_PARAMSin/etc/warp.envor the cloud-init configuration when different test parameters are needed..github/workflows/ibm-nightly-vm-cleanup.yaml (2)
48-63: Consider using arrays to avoid word-splitting issues.While the current unquoted expansion in the
forloop works for typical resource IDs, using a proper array would be more robust and would satisfy shellcheck's SC2086 warning.echo "🔍 Looking for floating IPs with the resource tag" - FLOATING_IP_IDS=$(ibmcloud resource search "tags:\"${RESOURCE_TAG}\" AND type:floating-ip" --output JSON | jq -r '.items[].resource_id' 2>/dev/null || echo "") + readarray -t FLOATING_IP_IDS < <(ibmcloud resource search "tags:\"${RESOURCE_TAG}\" AND type:floating-ip" --output JSON | jq -r '.items[].resource_id' 2>/dev/null || echo "") - if [ -n "${FLOATING_IP_IDS}" ] + if [ ${#FLOATING_IP_IDS[@]} -gt 0 ] && [ -n "${FLOATING_IP_IDS[0]}" ] then echo "🗑️ Releasing floating IP(s)" - for IP_ID in ${FLOATING_IP_IDS} + for IP_ID in "${FLOATING_IP_IDS[@]}" do ibmcloud is floating-ip-release "${IP_ID}" --force >/dev/null done
65-92: Apply the same array-based approach for VM cleanup.The VM cleanup section has the same word-splitting concerns as the floating IP cleanup. Additionally, consider verifying deletion of specific instance IDs rather than re-querying by tag.
echo "🔍 Looking for IBM VMs with the resource tag" - INSTANCE_IDS=$(ibmcloud resource search "tags:\"${RESOURCE_TAG}\" AND type:instance" --output JSON | jq -r '.items[].resource_id' 2>/dev/null || echo "") + readarray -t INSTANCE_IDS < <(ibmcloud resource search "tags:\"${RESOURCE_TAG}\" AND type:instance" --output JSON | jq -r '.items[].resource_id' 2>/dev/null || echo "") - if [ -z "${INSTANCE_IDS}" ] + if [ ${#INSTANCE_IDS[@]} -eq 0 ] || [ -z "${INSTANCE_IDS[0]}" ] then echo "ℹ️ No VMs found with the resource tag" echo "This might be expected if the VMs were already deleted or never created." exit 0 fi echo "🔎 Found VM(s) with the resource tag" echo "🗑️ Deleting IBM VM(s)..." - for INSTANCE_ID in ${INSTANCE_IDS} + for INSTANCE_ID in "${INSTANCE_IDS[@]}" do ibmcloud is instance-delete "${INSTANCE_ID}" --force >/dev/null done echo "✅ All tagged IBM VMs deleted successfully"docs/CI & Tests/warp-on-ibm.md (2)
70-80: Specify language for fenced code blockThe ASCII diagram should specify a language identifier for proper rendering.
-``` +```text ┌─────────────────┐ ┌─────────────────┐ ┌─────────────────┐
237-240: Fix list indentation for consistencyThe bullet list at lines 237-240 should start at column 0 for markdown consistency.
The config includes: - - The required packages that'll be installed on the VM - - Additional setup commands - - An environment file (`warp.env`) that is used to pass env vars from GitHub to the runner (via `envsubst` variable templating) - - The 'entrypoint' (that is the last command that is run by cloud-init; in our case, the Warp scripts) +- The required packages that'll be installed on the VM +- Additional setup commands +- An environment file (`warp.env`) that is used to pass env vars from GitHub to the runner (via `envsubst` variable templating) +- The 'entrypoint' (that is the last command that is run by cloud-init; in our case, the Warp scripts)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/ibm-warp-runner-config.yaml(1 hunks).github/workflows/ibm-nightly-cleanup-dispatcher.yaml(1 hunks).github/workflows/ibm-nightly-provision-dispatcher.yaml(1 hunks).github/workflows/ibm-nightly-vm-cleanup.yaml(1 hunks).github/workflows/ibm-nightly-vm-provision.yaml(1 hunks)Makefile(1 hunks)docs/CI & Tests/warp-on-ibm.md(1 hunks)src/test/external_tests/warp/run_nc_warp_on_test_container.sh(1 hunks)src/test/external_tests/warp/run_warp.js(3 hunks)src/test/external_tests/warp/run_warp_on_test_container.sh(1 hunks)tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh(1 hunks)tools/ibm_runner_helpers/slack_notifier.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- tools/ibm_runner_helpers/slack_notifier.js
- src/test/external_tests/warp/run_nc_warp_on_test_container.sh
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/external_tests/warp/run_warp_on_test_container.shsrc/test/external_tests/warp/run_warp.js
🧠 Learnings (4)
📓 Common learnings
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.053Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with mode 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically, so additional permission checks before sourcing are not necessary in this context.
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.053Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with permissions 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically by cloud-init, so additional permission checks before sourcing are not necessary in this context.
📚 Learning: 2025-09-18T07:12:14.304Z
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:2-5
Timestamp: 2025-09-18T07:12:14.304Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, debug logs with `set -x` are kept unmasked even when containing webhook URLs or other secrets because the logs are only accessible within the provisioned VM by permitted team members, and the debugging value outweighs secret exposure concerns in this controlled internal environment.
Applied to files:
docs/CI & Tests/warp-on-ibm.mdsrc/test/external_tests/warp/run_warp.jstools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh
📚 Learning: 2025-09-18T07:11:55.053Z
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.053Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with mode 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically, so additional permission checks before sourcing are not necessary in this context.
Applied to files:
docs/CI & Tests/warp-on-ibm.mdtools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh.github/ibm-warp-runner-config.yaml
📚 Learning: 2025-09-18T07:11:55.053Z
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.053Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with permissions 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically by cloud-init, so additional permission checks before sourcing are not necessary in this context.
Applied to files:
docs/CI & Tests/warp-on-ibm.mdtools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh.github/ibm-warp-runner-config.yaml
🧬 Code graph analysis (1)
tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh (2)
src/util/notifications_util.js (1)
failure(385-385)src/manage_nsfs/manage_nsfs_logging.js (1)
success(19-19)
🪛 actionlint (1.7.7)
.github/workflows/ibm-nightly-vm-cleanup.yaml
66-66: shellcheck reported issue in this script: SC2086:info:25:5: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/ibm-nightly-vm-provision.yaml
73-73: shellcheck reported issue in this script: SC2086:info:7:12: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 markdownlint-cli2 (0.18.1)
docs/CI & Tests/warp-on-ibm.md
27-27: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
70-70: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
237-237: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
238-238: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
239-239: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
240-240: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (17)
tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh (3)
1-8: LGTM! Shell configuration is robust.The script properly configures error handling (
-e), pipefail behavior, and debug tracing. Environment sourcing from/etc/warp.envis secure per the cloud-init provisioning model.
9-22: LGTM! Notification functions are well-structured.The notification functions correctly invoke the Slack notifier with appropriate error suppression (
|| true) to prevent notification failures from terminating the script.
43-44: LGTM! Trap configuration ensures proper cleanup.The trap setup correctly handles errors and guarantees VM shutdown on script exit.
.github/workflows/ibm-nightly-cleanup-dispatcher.yaml (1)
1-14: LGTM! Cleanup dispatcher is well-configured.The workflow correctly schedules nightly cleanup 4 hours after provisioning and properly passes required secrets to the reusable cleanup workflow. The manual trigger option facilitates testing.
src/test/external_tests/warp/run_warp_on_test_container.sh (1)
73-73: LGTM! Argument forwarding enables dynamic test parameters.The change correctly forwards all CLI arguments to the Warp runner script, enabling the flexible parameter passing needed for the nightly test infrastructure.
.github/workflows/ibm-nightly-provision-dispatcher.yaml (1)
1-18: LGTM! Provisioning dispatcher is properly configured.The workflow correctly schedules nightly VM provisioning, passes the cloud-init configuration location, and provides all required secrets to the reusable workflow. The manual trigger option is valuable for testing.
.github/ibm-warp-runner-config.yaml (4)
18-18: LGTM: Safety shutdown is a good failsafeThe 4-hour shutdown timer ensures the VM terminates even if test scripts fail, preventing runaway costs.
24-24: Pin npm package version for reproducibilityUnpinned
@slack/webhookinstall is non-reproducible and may break silently.- - [ npm, -g, install, '@slack/webhook' ] + - [ npm, -g, install, '@slack/webhook@^7', '--no-audit', '--no-fund' ]
26-28: Verify AWS CLI download integrityDownloading without checksum/signature verification is vulnerable to MITM or corruption.
- [ curl, -sS, https://awscli.amazonaws.com/awscli-exe-linux-x86_64.zip, -o, awscliv2.zip ] + - [ curl, -sS, https://awscli.amazonaws.com/awscli-exe-linux-x86_64.zip.sig, -o, awscliv2.zip.sig ] + # Verify signature (requires AWS public key provisioned) + # - [ gpg, --verify, awscliv2.zip.sig, awscliv2.zip ] - [ unzip, awscliv2.zip ]
32-32: Git clone approach is appropriate for ephemeral CI/CD infrastructureThis is a cloud-init configuration for single-use test runners (note the 4-hour shutdown timer on line 17). The repository is cloned to execute test scripts in an ephemeral environment, not for production deployment.
HTTPS cloning from github.com provides TLS certificate verification by default, which prevents MITM attacks. Commit pinning would be counterproductive here since the purpose is to run tests against the latest code. Other workflows in this repository properly use
actions/checkoutfor GitHub Actions contexts, which is the appropriate pattern.The suggested mitigations (commit pinning, SSH with known hosts, GPG verification) are security practices for persistent infrastructure or production deployments, not ephemeral test runners.
Likely an incorrect or invalid review comment.
.github/workflows/ibm-nightly-vm-provision.yaml (3)
42-67: LGTM: Secret masking and export pattern is well-implementedThe use of
jqto mask sensitive values before exporting toGITHUB_ENVfollows security best practices for GitHub Actions.
82-103: LGTM: VM provisioning with proper error handlingThe provisioning step includes timestamped naming, secret masking, and explicit error handling with clear failure messages.
110-131: LGTM: Floating IP reservation logic is correctRetrieving the NIC ID via
jqand binding the floating IP in a single command is the correct approach for IBM Cloud VPC.docs/CI & Tests/warp-on-ibm.md (1)
266-313: LGTM: Comprehensive debugging guidanceThe debugging section provides clear instructions for both serial console access (no authentication required) and SSH-based debugging when needed. The security trade-offs are well-documented.
src/test/external_tests/warp/run_warp.js (3)
31-35: Update usage text: "mix" → "mixed"The usage text at line 17 says "mix" but the code default at line 69 and the Warp CLI expect "mixed". Align the usage text to match.
- warp operation to run, by default mix op is used + warp operation to run, by default mixed op is used
78-78: Boolean parsing for--obj-randsizeis unsafe
argv.obj_randsize || falsetreats the string"false"as truthy. Parse explicitly so users can pass--obj-randsize false.- const disable_multipart = argv.disable_multipart || true; - const obj_randsize = argv.obj_randsize || false; + const parseBool = v => (v === true || v === 'true' || v === 1 || v === '1'); + const disable_multipart = argv.disable_multipart !== undefined ? parseBool(argv.disable_multipart) : true; + const obj_randsize = argv.obj_randsize !== undefined ? parseBool(argv.obj_randsize) : false;
92-92: Secrets leak: mask access_key/secret_key in logs
console.infoat line 94 prints the full command including credentials. Mask secrets before logging.const warp_command = `warp ${op} --host=localhost:${endpoint} --access-key=${access_key} --secret-key=${secret_key} --bucket=${bucket} --obj.size=${obj_size} --duration=${duration} --disable-multipart=${disable_multipart} --tls --insecure --concurrent ${number_of_workers}${obj_randsize ? ' --obj.randsize' : ''}`; + const masked_warp_command = `warp ${op} --host=localhost:${endpoint} --access-key=*** --secret-key=*** --bucket=${bucket} --obj.size=${obj_size} --duration=${duration} --disable-multipart=${disable_multipart} --tls --insecure --concurrent ${number_of_workers}${obj_randsize ? ' --obj.randsize' : ''}`; - console.info(`Running warp ${warp_command}`); + console.info(`Running warp ${masked_warp_command}`);
| run: | | ||
| # Mask sensitive values | ||
| [ -n "${SLACK_NIGHTLY_RESULTS_URL:-}" ] && echo "::add-mask::$SLACK_NIGHTLY_RESULTS_URL" | ||
| export SLACK_NIGHTLY_RESULTS_URL | ||
| export AWS_ACCESS_KEY_ID AWS_SECRET_ACCESS_KEY WARP_LOGS_BUCKET IBM_COS_ENDPOINT | ||
| # Use envsubst to replace variables in the config | ||
| envsubst < ${VM_CLOUD_INIT_CONFIG_LOCATION} > /tmp/ibm-vm-runner-config-with-secrets.yaml |
There was a problem hiding this comment.
Quote variable to prevent word splitting
Line 80: ${VM_CLOUD_INIT_CONFIG_LOCATION} should be quoted to prevent globbing and word splitting.
- envsubst < ${VM_CLOUD_INIT_CONFIG_LOCATION} > /tmp/ibm-vm-runner-config-with-secrets.yaml
+ envsubst < "${VM_CLOUD_INIT_CONFIG_LOCATION}" > /tmp/ibm-vm-runner-config-with-secrets.yaml📝 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.
| run: | | |
| # Mask sensitive values | |
| [ -n "${SLACK_NIGHTLY_RESULTS_URL:-}" ] && echo "::add-mask::$SLACK_NIGHTLY_RESULTS_URL" | |
| export SLACK_NIGHTLY_RESULTS_URL | |
| export AWS_ACCESS_KEY_ID AWS_SECRET_ACCESS_KEY WARP_LOGS_BUCKET IBM_COS_ENDPOINT | |
| # Use envsubst to replace variables in the config | |
| envsubst < ${VM_CLOUD_INIT_CONFIG_LOCATION} > /tmp/ibm-vm-runner-config-with-secrets.yaml | |
| run: | | |
| # Mask sensitive values | |
| [ -n "${SLACK_NIGHTLY_RESULTS_URL:-}" ] && echo "::add-mask::$SLACK_NIGHTLY_RESULTS_URL" | |
| export SLACK_NIGHTLY_RESULTS_URL | |
| export AWS_ACCESS_KEY_ID AWS_SECRET_ACCESS_KEY WARP_LOGS_BUCKET IBM_COS_ENDPOINT | |
| # Use envsubst to replace variables in the config | |
| envsubst < "${VM_CLOUD_INIT_CONFIG_LOCATION}" > /tmp/ibm-vm-runner-config-with-secrets.yaml |
🧰 Tools
🪛 actionlint (1.7.7)
73-73: shellcheck reported issue in this script: SC2086:info:7:12: Double quote to prevent globbing and word splitting
(shellcheck)
🤖 Prompt for AI Agents
.github/workflows/ibm-nightly-vm-provision.yaml lines 73-80: the use of
${VM_CLOUD_INIT_CONFIG_LOCATION} is unquoted which can cause word splitting or
globbing; update the envsubst invocation to read from a quoted variable (e.g.
envsubst < "${VM_CLOUD_INIT_CONFIG_LOCATION}" >
/tmp/ibm-vm-runner-config-with-secrets.yaml) so the filename is treated as a
single argument and avoid shell word-splitting/globbing issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/CI & Tests/warp-on-ibm.md (2)
27-27: Minor formatting: Add trailing pipe to table rowThe table is missing a trailing pipe character on line 27.
Apply this diff:
-| `IBM_COS_WRITER_CREDENTIALS` | JSON configuration for IBM Cloud Object Storage access (see below) +| `IBM_COS_WRITER_CREDENTIALS` | JSON configuration for IBM Cloud Object Storage access (see below) |
257-260: Optional: Adjust list indentation for linter complianceThe linter expects no indentation for these list items. The current formatting is readable, but you may adjust it for linter compliance.
Apply this diff:
The config includes: - - The required packages that'll be installed on the VM - - Additional setup commands - - An environment file (`warp.env`) that is used to pass env vars from GitHub to the runner (via `envsubst` variable templating) - - The 'entrypoint' (that is the last command that is run by cloud-init; in our case, the Warp scripts) +- The required packages that'll be installed on the VM +- Additional setup commands +- An environment file (`warp.env`) that is used to pass env vars from GitHub to the runner (via `envsubst` variable templating) +- The 'entrypoint' (that is the last command that is run by cloud-init; in our case, the Warp scripts)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/CI & Tests/warp-on-ibm.md(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.053Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with mode 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically, so additional permission checks before sourcing are not necessary in this context.
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.053Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with permissions 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically by cloud-init, so additional permission checks before sourcing are not necessary in this context.
📚 Learning: 2025-09-18T07:12:14.304Z
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:2-5
Timestamp: 2025-09-18T07:12:14.304Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, debug logs with `set -x` are kept unmasked even when containing webhook URLs or other secrets because the logs are only accessible within the provisioned VM by permitted team members, and the debugging value outweighs secret exposure concerns in this controlled internal environment.
Applied to files:
docs/CI & Tests/warp-on-ibm.md
📚 Learning: 2025-09-18T07:11:55.053Z
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.053Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with mode 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically, so additional permission checks before sourcing are not necessary in this context.
Applied to files:
docs/CI & Tests/warp-on-ibm.md
📚 Learning: 2025-09-18T07:11:55.053Z
Learnt from: Neon-White
PR: noobaa/noobaa-core#9196
File: tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh:3-3
Timestamp: 2025-09-18T07:11:55.053Z
Learning: In the NooBaa project's IBM Cloud Warp testing infrastructure, `/etc/warp.env` is created by cloud-init during VM provisioning with permissions 0600 (owner read/write only). This file contains environment variables and secrets passed from GitHub Actions. The file creation is part of a controlled internal flow with proper permissions set automatically by cloud-init, so additional permission checks before sourcing are not necessary in this context.
Applied to files:
docs/CI & Tests/warp-on-ibm.md
🪛 markdownlint-cli2 (0.18.1)
docs/CI & Tests/warp-on-ibm.md
27-27: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
257-257: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
258-258: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
259-259: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
260-260: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (8)
docs/CI & Tests/warp-on-ibm.md (8)
17-60: Well-documented prerequisites sectionThe prerequisites are clearly structured with detailed descriptions and JSON format examples for complex secrets. This will help operators configure the infrastructure correctly.
62-82: Clear architecture overview with visual flowThe component breakdown and ASCII flow diagram effectively communicate the automation's structure and timing.
84-217: Comprehensive execution flow documentationThe detailed walkthrough effectively maps the complete automation lifecycle from midnight provisioning through 4 AM cleanup. The timeline table, file references, and step-by-step descriptions provide excellent operational clarity.
219-249: Well-structured workflow documentationThe dispatcher pattern is clearly explained with accurate file references and purpose descriptions for each workflow component.
262-279: Clear helper script documentationBoth scripts are well-described with their key responsibilities and parameters clearly outlined.
281-284: Comprehensive log location referenceAll relevant log locations are documented for effective troubleshooting.
286-332: Excellent debugging guidanceThe serial console instructions provide a secure, no-authentication debugging method, while the SSH debug section offers a comprehensive fallback for deeper investigation. The security considerations and cleanup reminder are appropriate.
1-333: Excellent comprehensive documentationThis documentation provides thorough coverage of the IBM Cloud Warp automation infrastructure. The structure flows logically from prerequisites through execution details to debugging, making it valuable for both initial setup and ongoing operations. The timeline tables, ASCII diagrams, and file references significantly enhance clarity.
1 participant
Design doc
Explain the Changes
bash -cwrapper to properly handleWARP_ARGSexpansion, allowing to pass dynamic test parameters torun_warp.jsthrough the Makefile targetsIssues:
Testing Instructions:
Test VM provisioning workflow: Trigger the "Nightly Warp IBM VM Provisioning" workflow manually via GitHub Actions and verify VM and floating IP creation in IBM Cloud console
Test VM cleanup workflow: After provisioning, trigger the "IBM VM Cleanup" workflow manually and confirm VM and floating IP are properly deleted
Test local warp execution: Run
make test-warp WARP_ARGS="--duration 1m --obj-size 1KB"to verify the new argument passing mechanism worksTest Slack notifications: Verify the
slack_notifier.jsscript works with:node tools/ibm_runner_helpers/slack_notifier.js <webhook_url> success "Test message"Validate cloud-init config: Use
cloud-init schema --config-file .github/ibm-warp-runner-config.yamlto verify configuration syntaxEnd-to-end test: Let the nightly workflow run completely, verify Slack notifications are sent, and verify logs appear in the IBM COS bucket mentioned in the Slack notification
Doc added/updated
Summary by CodeRabbit