Skip to content

Conversation

@Neon-White
Copy link
Contributor

@Neon-White Neon-White commented Sep 30, 2025

Explain the Changes

  • Created a simple Node.js script for sending success/failure notifications to Slack
  • Created a bash script that:
    • Builds the NooBaa tester image
    • Runs 3-hour Warp tests with random object sizes
    • Uploads logs to IBM COS
    • Sends a Slack notification about the run
  • Added bash -c wrapper to properly handle WARP_ARGS expansion, allowing to pass dynamic test parameters to run_warp.js through the Makefile targets

Issues:

  • Gap: The workflow currently builds the tester image on the IBM runner - we should build it on GitHub and then download it with the runner (whether directly or by publishing from GitHub to Quay and then downloading from there)

Testing Instructions:

  • Test local warp execution: Run make test-warp WARP_ARGS="--duration 1m --obj-size 1KB" to verify the new argument passing mechanism works

  • Test Slack notifications: Verify the slack_notifier.js script 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.yaml to verify configuration syntax

  • Doc added/updated

Summary by CodeRabbit

  • New Features

    • Added --obj-randsize flag for Warp tests.
    • Cloud-runner bootstrap to prepare runtime, install tooling, set environment, and auto-schedule shutdown.
    • Cloud orchestration to run Warp, upload logs to object storage, and send Slack notifications on success/failure.
  • Tests

    • Warp test runner now forwards all command-line arguments and centralizes logs for upload.
  • Chores

    • Test invocations updated to accept parameterized arguments via shell execution.

@coderabbitai
Copy link

coderabbitai bot commented Sep 30, 2025

Walkthrough

Adds a GitHub cloud-init config to bootstrap an IBM Warp Runner VM, introduces orchestration and Slack notifier scripts to run Warp tests and upload logs to IBM COS, updates Makefile and container wrapper scripts to forward CLI args, and adds a new --obj-randsize flag to the Node warp runner.

Changes

Cohort / File(s) Summary of Changes
IBM Warp Runner cloud-init
/.github/ibm-warp-runner-config.yaml
New cloud-init scaffold: updates packages, installs docker/git/make/unzip/jq/curl/nodejs/npm/awscli, writes /etc/warp.env, schedules auto-shutdown, adds ubuntu to docker group, enables Docker, installs global npm slack package, clones repo, creates /home/ubuntu/tests, installs wrapper scripts, and launches the runner wrapper.
Make targets for Warp
Makefile
Modified test-warp and test-nc-warp invocations to run wrappers via bash -c "… $(WARP_ARGS)", enabling forwarding of WARP_ARGS into the container-runner invocation.
Warp container wrappers (arg passthrough)
src/test/external_tests/warp/run_warp_on_test_container.sh, src/test/external_tests/warp/run_nc_warp_on_test_container.sh
Updated wrappers to forward all CLI args ("$@") into run_warp.js (node invocation), enabling argument passthrough from outer callers.
Warp runner enhancement
src/test/external_tests/warp/run_warp.js
Adds boolean CLI flag --obj-randsize (default false), documents it, reads argv.obj_randsize, and conditionally appends --obj.randsize to the constructed Warp CLI when set.
Cloud runner orchestration
tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh
New orchestrator script: strict bash options, timestamped log dir, build tester image, run Warp via make test-warp with WARP_ARGS, trap ERR/EXIT to upload logs to IBM COS and notify Slack, then shutdown VM.
Slack notifier utility
tools/ibm_runner_helpers/slack_notifier.js
New Node script to send Slack webhook notifications: validates args, prefixes emoji based on status, sends text payload, logs errors, and exits 0 after attempt.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant CloudInit as Cloud-Init (VM)
  participant Runner as run_containerized_warp_on_cloud_runner.sh
  participant Docker as Docker Daemon
  participant TesterImg as NooBaa Tester Image
  participant Container as Test Container
  participant Warp as Warp CLI (in container)
  participant COS as IBM COS
  participant Slack as Slack Webhook

  CloudInit->>Runner: Start runner script
  activate Runner
  Runner->>Docker: Build tester image (make tester)
  Docker-->>TesterImg: Image available
  Runner->>Docker: Run container with WARP_ARGS (make test-warp)
  Docker->>Container: Start test container
  Container->>Warp: Execute tests
  alt Success
    Warp-->>Runner: Exit 0
    Runner->>COS: Upload test logs to s3://${WARP_LOGS_BUCKET}/${log_dir}/
    Runner->>Slack: Send success notification
  else Failure
    Warp-->>Runner: Non-zero exit
    Runner->>COS: Upload logs/cloud-init to s3://${WARP_LOGS_BUCKET}/${log_dir}/
    Runner->>Slack: Send failure notification
  end
  Runner->>CloudInit: Trigger VM shutdown (EXIT trap)
  deactivate Runner
Loading
sequenceDiagram
  autonumber
  participant Make as Make (test-warp / test-nc-warp)
  participant Shell as bash -c
  participant Wrapper as run_*_warp_on_test_container.sh
  participant Node as node run_warp.js
  participant Warp as Warp CLI

  Make->>Shell: Invoke wrapper via bash -c with $(WARP_ARGS)
  Shell->>Wrapper: Execute wrapper script with "$@"
  Wrapper->>Node: node run_warp.js "$@"
  Node->>Warp: warp ... [includes --obj.randsize if --obj-randsize passed]
  Warp-->>Node: Exit code
  Node-->>Wrapper: Propagate exit
  Wrapper-->>Shell: Propagate exit
  Shell-->>Make: Propagate exit
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • Add Warp on IBM Cloud docs #9231 — Implements IBM Cloud Warp runner (cloud-init, Slack notifier, runner scripts, Makefile changes) closely related to these additions.

Suggested reviewers

  • alphaprinz
  • romayalon
  • liranmauda

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title “Add Warp-specific IBM Cloud VM nightly runs tooling” clearly summarizes the primary change by indicating the introduction of tooling for nightly Warp test runs on IBM Cloud VMs, is concise and specific to the changeset, and avoids extraneous detail or generic phrasing.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (7)
tools/ibm_runner_helpers/slack_notifier.js (1)

23-28: Consider logging to stderr and using non-zero exit code on failure.

The script currently logs errors to stderr but always exits with code 0, even on failure. This prevents calling scripts from detecting notification failures.

Apply this diff to enable failure detection:

   } catch (error) {
     console.error('Notification failed:', error.message);
+    process.exit(1);
   }
-
-  process.exit(0);
 }

If silent failures are intentional (e.g., to prevent CI pipeline failures due to notification issues), consider adding a command-line flag like --fail-on-error to make this behavior configurable.

.github/ibm-warp-runner-config.yaml (1)

3-3: Consider pinning package versions for reproducibility.

Installing packages without version constraints can lead to inconsistent behavior across VM instances if upstream packages are updated. This is particularly important for Docker, Node.js, and npm, which have breaking changes between major versions.

Consider specifying versions in the format package=version, e.g.:

-packages: [docker.io, git, make, unzip, jq, curl, ca-certificates, nodejs, npm]
+packages: [docker.io=24.0.5-0ubuntu1, git, make, unzip, jq, curl, ca-certificates, nodejs=18.x, npm]

Alternatively, document the tested/expected versions in a comment for reference.

tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh (5)

9-12: Consider using UTC explicitly for timestamp consistency.

The timestamp uses the system's default timezone, which should be UTC on cloud VMs but isn't explicitly guaranteed. For consistency and clarity across different environments, consider using UTC explicitly.

-timestamp=$(date +"%Y-%m-%d-%H-%M-%S")
+timestamp=$(date -u +"%Y-%m-%d-%H-%M-%S")

14-22: Consider logging notification failures for debugging.

The || true pattern correctly prevents notification failures from stopping the script. However, silent failures make it difficult to diagnose why notifications weren't received.

 send_failure_notification() {
     local msg="$1"
-    node /usr/local/bin/slack_notifier.js "${SLACK_NIGHTLY_RESULTS_URL}" failure "Warp failed: ${msg}" || true
+    node /usr/local/bin/slack_notifier.js "${SLACK_NIGHTLY_RESULTS_URL}" failure "Warp failed: ${msg}" || echo "Warning: Failed to send Slack notification"
 }
 
 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
+    node /usr/local/bin/slack_notifier.js "${SLACK_NIGHTLY_RESULTS_URL}" success "Warp success - logs available at: ${log_location}" || echo "Warning: Failed to send Slack notification"
 }

31-36: Consider uploading test logs on failure as well.

Currently, handle_failure only uploads cloud-init logs. If the test fails after producing partial logs, those logs would be valuable for debugging but aren't uploaded.

 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}"
+    # Upload test logs if they exist
+    if [ -d "${test_logs_path}" ] && [ "$(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}/test-logs/" --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}/"
 }

38-41: Unnecessary sudo in shutdown_vm.

The script runs as root (via cloud-init), so sudo is unnecessary. However, this is harmless and doesn't affect functionality.

 shutdown_vm() {
     echo "Shutting down VM..."
-    sudo shutdown -P now
+    shutdown -P now
 }

56-56: Consider parameterizing WARP_ARGS for flexibility.

The PR objectives mention "enabling dynamic test parameters," but WARP_ARGS is hardcoded here. If different test configurations are needed (e.g., shorter duration for testing, different object sizes), the script would need to be modified.

Consider sourcing WARP_ARGS from /etc/warp.env or defaulting to current values:

+# Use environment variable if set, otherwise use defaults
+WARP_ARGS="${WARP_ARGS:---duration 3h --obj-size 10m --obj-randsize}"
+
 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_ARGS}"

Then add WARP_ARGS=${WARP_ARGS} to the cloud-init config's /etc/warp.env file.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f792c8c and 45e2cae.

📒 Files selected for processing (7)
  • .github/ibm-warp-runner-config.yaml (1 hunks)
  • Makefile (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.js
  • src/test/external_tests/warp/run_warp_on_test_container.sh
  • src/test/external_tests/warp/run_nc_warp_on_test_container.sh
🧠 Learnings (2)
📚 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:

  • .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:

  • .github/ibm-warp-runner-config.yaml
⏰ 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 (15)
src/test/external_tests/warp/run_nc_warp_on_test_container.sh (1)

73-73: LGTM! Correct argument forwarding implementation.

The use of "$@" properly forwards all command-line arguments to the Node.js runner while preserving argument boundaries and handling spaces correctly.

src/test/external_tests/warp/run_warp_on_test_container.sh (1)

73-73: LGTM! Correct argument forwarding implementation.

The use of "$@" properly forwards all command-line arguments to the Node.js runner while preserving argument boundaries.

src/test/external_tests/warp/run_warp.js (3)

31-35: LGTM! Clear documentation for the new flag.

The usage documentation correctly describes the new --obj-randsize flag with appropriate formatting and default value.


78-78: LGTM! Proper default value handling.

The variable declaration correctly uses argv.obj_randsize with a falsy default, consistent with other boolean flags in the script.


92-92: No change needed: --obj.randsize is a valid Warp CLI flag. Confirmed via the official Minio Warp documentation.

tools/ibm_runner_helpers/slack_notifier.js (2)

1-11: LGTM! Clean argument parsing and graceful early exit.

The script correctly parses command-line arguments and exits gracefully when no webhook URL is provided, which is appropriate for optional notification scenarios.


13-21: LGTM! Simple and correct Slack message construction.

The emoji-based status indicator and text-only payload are appropriate for notifications. The webhook integration is straightforward and correct.

Makefile (2)

360-360: Quote WARP_ARGS to handle spaces and special characters safely.

The current implementation expands $(WARP_ARGS) without quoting, which could cause issues if WARP_ARGS contains spaces or special characters. While the downstream script uses "$@" correctly, the Makefile should quote the variable expansion.

Apply this diff to add proper quoting:

-	$(CONTAINER_ENGINE) run $(CPUSET) --privileged --user root --network noobaa-net --name noobaa_$(GIT_COMMIT)_$(NAME_POSTFIX) --env "SUPPRESS_LOGS=$(SUPPRESS_LOGS)" --env "POSTGRES_HOST=coretest-postgres-$(GIT_COMMIT)-$(NAME_POSTFIX)" --env "POSTGRES_USER=noobaa" --env "DB_TYPE=postgres" --env "POSTGRES_DBNAME=coretest" -v $(PWD)/logs:/logs $(TESTER_TAG) bash -c "./src/test/external_tests/warp/run_warp_on_test_container.sh $(WARP_ARGS)"
+	$(CONTAINER_ENGINE) run $(CPUSET) --privileged --user root --network noobaa-net --name noobaa_$(GIT_COMMIT)_$(NAME_POSTFIX) --env "SUPPRESS_LOGS=$(SUPPRESS_LOGS)" --env "POSTGRES_HOST=coretest-postgres-$(GIT_COMMIT)-$(NAME_POSTFIX)" --env "POSTGRES_USER=noobaa" --env "DB_TYPE=postgres" --env "POSTGRES_DBNAME=coretest" -v $(PWD)/logs:/logs $(TESTER_TAG) bash -c './src/test/external_tests/warp/run_warp_on_test_container.sh $(WARP_ARGS)'

This uses single quotes around the bash -c command string, allowing $(WARP_ARGS) to be expanded by bash at runtime, which better handles spaces and preserves the shell's argument parsing.

Likely an incorrect or invalid review comment.


368-368: Quote WARP_ARGS to handle spaces and special characters safely.

The same quoting issue applies here as in the test-warp target.

Apply this diff:

-	$(CONTAINER_ENGINE) run $(CPUSET) --privileged --user root --name noobaa_$(GIT_COMMIT)_$(NAME_POSTFIX) --env "SUPPRESS_LOGS=$(SUPPRESS_LOGS)" -v $(PWD)/logs:/logs $(TESTER_TAG) bash -c "./src/test/external_tests/warp/run_nc_warp_on_test_container.sh $(WARP_ARGS)"
+	$(CONTAINER_ENGINE) run $(CPUSET) --privileged --user root --name noobaa_$(GIT_COMMIT)_$(NAME_POSTFIX) --env "SUPPRESS_LOGS=$(SUPPRESS_LOGS)" -v $(PWD)/logs:/logs $(TESTER_TAG) bash -c './src/test/external_tests/warp/run_nc_warp_on_test_container.sh $(WARP_ARGS)'

Likely an incorrect or invalid review comment.

.github/ibm-warp-runner-config.yaml (4)

5-14: LGTM - secrets handling follows established pattern.

The permissions (0600) and file location (/etc/warp.env) align with the established pattern documented in the learnings. The variable substitution syntax is correct for cloud-init.

Based on learnings.


18-18: Verify shutdown timing buffer is sufficient.

The VM is scheduled to shut down after 240 minutes (4 hours), but the Warp test runs for 3 hours. This leaves only 1 hour for:

  • VM provisioning and setup (docker, AWS CLI, git clone, image build)
  • Log upload to COS
  • Slack notification

Consider whether this buffer is sufficient, especially if the tester image build is slow or network operations are delayed.

You can validate the typical setup and teardown time by reviewing historical run logs or adding timing instrumentation.


34-38: LGTM - script installation and execution flow is correct.

The scripts are installed with appropriate permissions and ownership. The main script execution on line 38 runs as root (via cloud-init), which correctly allows it to read /etc/warp.env (mode 0600, owned by root) and perform privileged operations like VM shutdown.


22-22: Verify Docker daemon readiness in GitHub runner

While systemctl enable --now docker starts the service, Docker may not immediately accept connections under runner images where timeout isn’t available. Before invoking make tester on line 38, add an explicit readiness loop:

#!/bin/bash
# Poll for Docker readiness up to 60s
for i in {1..60}; do
  docker info &>/dev/null && break
  sleep 1
done
docker info &>/dev/null || {
  echo "Docker daemon failed to start"
  exit 1
}

Insert this check into the main script (or as a preceding runcmd) to avoid build races.

tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh (2)

1-7: LGTM - environment setup follows best practices.

The strict bash options (-euxo pipefail) ensure errors are caught and the script fails fast. The set -a pattern correctly exports all variables from /etc/warp.env. Setting HOME to /home/ubuntu is appropriate since cloud-init runs this as root.

Based on learnings.


43-58: LGTM - error handling and execution flow are robust.

The trap setup ensures that:

  • Any command failure triggers handle_failure (ERR trap)
  • The VM always shuts down (EXIT trap), even if error handling fails
  • The execution flow is straightforward and easy to follow

The -o tester flag correctly optimizes by skipping the rebuild after line 49.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
tools/ibm_runner_helpers/slack_notifier.js (2)

23-25: Consider logging the full error and exiting with a non-zero code on failure.

The current implementation logs only error.message and always exits with code 0, even when the notification fails. This makes debugging difficult and prevents the caller from detecting failures.

Apply this diff to improve error visibility:

   } catch (error) {
-    console.error('Notification failed:', error.message);
+    console.error('Notification failed:', error);
+    process.exit(1);
   }
-
-  process.exit(0);

Alternatively, if graceful failure is intentional (to avoid blocking workflows), consider adding a comment explaining the rationale for exiting with code 0.


27-28: Remove redundant process.exit(0) call.

The explicit process.exit(0) on line 27 is unnecessary because Node.js will naturally exit when the async main() function completes and there are no pending operations. This explicit exit can prevent proper cleanup of resources.

Apply this diff to remove the redundant exit:

   } catch (error) {
     console.error('Notification failed:', error.message);
   }
-
-  process.exit(0);
 }

If the explicit exit is intentional to ensure the script terminates immediately (e.g., to avoid hanging on unclosed connections), consider adding a comment explaining the rationale.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 45e2cae and 876d2c4.

📒 Files selected for processing (7)
  • .github/ibm-warp-runner-config.yaml (1 hunks)
  • Makefile (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 (6)
  • src/test/external_tests/warp/run_warp_on_test_container.sh
  • src/test/external_tests/warp/run_nc_warp_on_test_container.sh
  • .github/ibm-warp-runner-config.yaml
  • Makefile
  • src/test/external_tests/warp/run_warp.js
  • tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Neon-White
PR: noobaa/noobaa-core#9229
File: .github/workflows/ibm-nightly-provision-dispatcher.yaml:13-13
Timestamp: 2025-09-30T08:56:55.452Z
Learning: In the noobaa-core repository, PR #9229 (nightly IBM VM provision dispatcher) has a dependency on `.github/ibm-warp-runner-config.yaml` which is provided in PR #9230, requiring PR #9230 to be merged first.
⏰ 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
🔇 Additional comments (1)
tools/ibm_runner_helpers/slack_notifier.js (1)

13-21: LGTM!

The webhook initialization and message sending logic is correct. The status-based emoji selection and text construction are straightforward and appropriate for a notification tool.

@Neon-White Neon-White force-pushed the warp-helpers branch 2 times, most recently from 65ce483 to a605615 Compare October 2, 2025 09:07
Copy link
Contributor

@alphaprinz alphaprinz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
tools/ibm_runner_helpers/slack_notifier.js (1)

8-10: Exit with non-zero code on validation failure.

The script exits with code 0 when required arguments are missing. This prevents callers from detecting misconfiguration. Exit with code 1 to signal an error.

Apply this diff:

   if (!webhookUrl || !status || !message) {
     console.error('Usage: node slack_notifier.js <webhook_url> <status> <message>');
-    process.exit(0);
+    process.exit(1);
   }
.github/ibm-warp-runner-config.yaml (1)

24-24: Consider pinning the npm package version.

Installing @slack/webhook without a version specifier pulls the latest version, which could introduce breaking changes in future runs. While the VM is ephemeral, pinning reduces unexpected failures.

Apply this diff to pin to a specific version (check current latest and update accordingly):

-  - [ npm, -g, install, '@slack/webhook' ]
+  - [ npm, -g, install, '@slack/[email protected]' ]

Or use a semver range like '@slack/webhook@^7.0.0' to allow patch updates.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a605615 and 76f0d8f.

📒 Files selected for processing (7)
  • .github/ibm-warp-runner-config.yaml (1 hunks)
  • Makefile (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 (5)
  • src/test/external_tests/warp/run_warp_on_test_container.sh
  • Makefile
  • tools/ibm_runner_helpers/run_containerized_warp_on_cloud_runner.sh
  • src/test/external_tests/warp/run_warp.js
  • src/test/external_tests/warp/run_nc_warp_on_test_container.sh
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Neon-White
PR: noobaa/noobaa-core#9229
File: .github/workflows/ibm-nightly-provision-dispatcher.yaml:13-13
Timestamp: 2025-09-30T08:56:55.478Z
Learning: In the noobaa-core repository, PR #9229 (nightly IBM VM provision dispatcher) has a dependency on `.github/ibm-warp-runner-config.yaml` which is provided in PR #9230, requiring PR #9230 to be merged first.
Learnt from: Neon-White
PR: noobaa/noobaa-core#9230
File: tools/ibm_runner_helpers/slack_notifier.js:4-4
Timestamp: 2025-09-30T10:27:57.996Z
Learning: In the noobaa-core repository, the `tools/ibm_runner_helpers/slack_notifier.js` script depends on `slack/webhook`, which is installed globally via `npm -g install 'slack/webhook'` in `.github/ibm-warp-runner-config.yaml` (cloud-init configuration) rather than being declared in package.json, since the script runs on IBM Cloud VMs provisioned by cloud-init.
📚 Learning: 2025-09-30T10:27:57.996Z
Learnt from: Neon-White
PR: noobaa/noobaa-core#9230
File: tools/ibm_runner_helpers/slack_notifier.js:4-4
Timestamp: 2025-09-30T10:27:57.996Z
Learning: In the noobaa-core repository, the `tools/ibm_runner_helpers/slack_notifier.js` script depends on `slack/webhook`, which is installed globally via `npm -g install 'slack/webhook'` in `.github/ibm-warp-runner-config.yaml` (cloud-init configuration) rather than being declared in package.json, since the script runs on IBM Cloud VMs provisioned by cloud-init.

Applied to files:

  • .github/ibm-warp-runner-config.yaml
  • tools/ibm_runner_helpers/slack_notifier.js
📚 Learning: 2025-09-30T08:56:55.478Z
Learnt from: Neon-White
PR: noobaa/noobaa-core#9229
File: .github/workflows/ibm-nightly-provision-dispatcher.yaml:13-13
Timestamp: 2025-09-30T08:56:55.478Z
Learning: In the noobaa-core repository, PR #9229 (nightly IBM VM provision dispatcher) has a dependency on `.github/ibm-warp-runner-config.yaml` which is provided in PR #9230, requiring PR #9230 to be merged first.

Applied to files:

  • .github/ibm-warp-runner-config.yaml
  • tools/ibm_runner_helpers/slack_notifier.js
📚 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:

  • .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:

  • .github/ibm-warp-runner-config.yaml
⏰ 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
🔇 Additional comments (1)
tools/ibm_runner_helpers/slack_notifier.js (1)

23-27: Silent failure: notification errors don't propagate.

The script exits with code 0 even when the webhook send fails. This prevents callers from detecting notification failures. If notification failure should be silent (so it doesn't break the main job), this is acceptable. Otherwise, exit with code 1 on error.

Current behavior: notification failures are logged but don't fail the script. Is this intentional?

If notification failures should propagate:

   } catch (error) {
     console.error('Notification failed:', error.message);
+    process.exit(1);
   }
-
-  process.exit(0);

@Neon-White Neon-White merged commit ec74a8b into noobaa:master Oct 4, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants