Skip to content

Conversation

@jiridanek
Copy link
Member

@jiridanek jiridanek commented Dec 19, 2025

Description

Enhanced bash script robustness in .github/actions/trivy-scan-action/action.yml with comprehensive improvements:

Error Handling:

  • Added set -Eeuo pipefail to all three bash script blocks (lines 71, 87, 130)
  • Ensures immediate failure on errors, undefined variables, and pipeline failures

Variable Initialization:

  • Added default initialization for conditionally-set variables SCAN_ARGS="" and PODMAN_ARGS="" (lines 95-96)
  • Prevents unbound variable errors when using set -u

Variable Quoting:

  • Quoted all variable assignments and expansions throughout
  • Applied to: REPORT_FOLDER, REPORT_FILE, REPORT_TEMPLATE, SCAN_TARGET, SCAN_TYPE
  • Quoted all GitHub Actions inputs and step outputs
  • Quoted command substitutions: basename
  • Quoted file operations: mkdir, cp, cat
  • Quoted podman run arguments including volume mounts and image references

Benefits:

  • Prevents word splitting and globbing issues with paths containing spaces/special characters
  • Catches silent failures immediately
  • Maintains compatibility while adding production-grade robustness
  • Safe handling of all edge cases with unbound variables

How Has This Been Tested?

Self checklist (all need to be checked):

  • Ensure that you have run make test (gmake on macOS) before asking for review
  • Changes to everything except Dockerfile.konflux files should be done in odh/notebooks and automatically synced to rhds/notebooks. For Konflux-specific changes, modify Dockerfile.konflux files directly in rhds/notebooks as these require special attention in the downstream repository and flow to the upcoming RHOAI release.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Summary by CodeRabbit

  • New Features

    • Added a reusable Trivy vulnerability scanner action for GitHub Actions workflows, enabling consistent security scanning with configurable parameters (scan type, target, version, and report generation).
  • Tests

    • Added test workflow for Trivy scan action, covering image and filesystem scan scenarios.
  • Chores

    • Refactored build workflows to use the new reusable Trivy scanning action, improving maintainability.

✏️ Tip: You can customize this high-level summary in your review settings.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 19, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@jiridanek jiridanek requested a review from caponetto December 19, 2025 14:16
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 19, 2025

Warning

Rate limit exceeded

@jiridanek has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 24 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between f021316 and e4779b8.

📒 Files selected for processing (3)
  • .github/actions/trivy-scan-action/action.yml (1 hunks)
  • .github/workflows/build-notebooks-TEMPLATE.yaml (1 hunks)
  • .github/workflows/test-trivy-scan-action.yaml (1 hunks)

Walkthrough

Adds a new reusable composite GitHub Action for Trivy scans, integrates it into the build-notebooks workflow (replacing inline scan steps), and adds a test workflow that exercises image and filesystem scan modes.

Changes

Cohort / File(s) Summary
New Trivy Scanner Action
\.github/actions/trivy-scan-action/action\.yml
Adds a composite GitHub Action "Trivy Vulnerability Scanner" with inputs (scan-type, scan-target, trivy-version, podman-socket, workspace-path, report-template, scanners, ignore-unfixed, timeout, exit-code) and output report-file. Implements steps: setup (report dir/template), conditional Trivy run (image vs filesystem) via Podman, and appending report to job summary.
Workflow Integration
\.github/workflows/build-notebooks-TEMPLATE\.yaml
Replaces inline Podman/Trivy orchestration with a step that invokes the local ./.github/actions/trivy-scan-action composite action, passing resolved scan-type/scan-target and related inputs.
New Test Workflow
\.github/workflows/test-trivy-scan-action\.yaml
Adds a workflow to validate the action with two jobs: test-scan-type-image (sets up Podman, ACLs, pulls test image, runs action against image) and test-scan-type-fs (runs action against filesystem ./). Triggers: workflow_dispatch, pull_request, push (path filters).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Review the composite action's conditional logic for scan-type and volume/socket mounting.
  • Validate Podman socket handling, ACL commands, and permissions in the action and test workflow.
  • Confirm report template copying, output emission, and job-summary append behavior.

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive The description provides detailed context about bash script improvements with specific error handling, variable initialization, and quoting enhancements. However, the testing section is incomplete with unchecked merge criteria and no actual testing details provided. Complete the self-checklist items and provide concrete testing details (environment, test commands run, verification results) to demonstrate manual testing of the changes.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main change: adding a Trivy scan action for vulnerability scanning. It's concise, specific, and clearly summarizes the primary contribution.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

@github-actions github-actions bot added the review-requested GitHub Bot creates notification on #pr-review-ai-ide-team slack channel label Dec 19, 2025
@openshift-ci openshift-ci bot added the size/l label Dec 19, 2025
@jiridanek
Copy link
Member Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 19, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@openshift-ci openshift-ci bot added size/l and removed size/l labels Dec 19, 2025
Copy link
Contributor

@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: 1

🧹 Nitpick comments (1)
.github/actions/trivy-scan-action/action.yml (1)

127-136: Summary step correctly handles missing report file.

The step appropriately warns when the report file is not found rather than failing, which is useful when the scan step fails. Consider adding if: always() to ensure the summary is added even when the scan step fails.

🔎 Optional improvement
    - name: Add report to job summary
+     if: always()
      shell: bash
      run: |
📜 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 52ff70e and 827dcf7.

📒 Files selected for processing (3)
  • .github/actions/trivy-scan-action/action.yml (1 hunks)
  • .github/workflows/build-notebooks-TEMPLATE.yaml (1 hunks)
  • .github/workflows/test-trivy-scan-action.yaml (1 hunks)
🔇 Additional comments (5)
.github/workflows/build-notebooks-TEMPLATE.yaml (1)

364-371: Clean refactoring to use the reusable action.

The integration properly delegates Trivy scanning to the new composite action with all required inputs. The workflow maintains consistency by using the existing TRIVY_VERSION environment variable.

.github/workflows/test-trivy-scan-action.yaml (2)

27-43: Good conditional dependency installation.

The approach of collecting missing packages into an array before running apt-get is efficient and avoids unnecessary apt-get calls when dependencies are already present.


45-57: Proper socket access configuration.

The ACL configuration correctly grants the runner user access to the rootful podman socket. Execute permission on the directory for traversal and read-write on the socket for communication are the minimum required permissions.

.github/actions/trivy-scan-action/action.yml (2)

67-81: Well-structured setup step with proper error handling.

Good use of set -Eeuo pipefail and proper variable quoting throughout. The template is correctly copied to the report folder for access within the Trivy container.


111-123: Verify unquoted variable expansions are intentional.

The variables $PODMAN_ARGS (line 112) and $SCAN_ARGS (line 116) are intentionally unquoted to allow word splitting for multiple arguments. This works correctly as long as paths don't contain spaces. Given that GitHub workspace paths and podman socket paths typically don't contain spaces, this is acceptable.

Ensure that inputs.podman-socket and inputs.workspace-path don't contain spaces in actual usage. The default values are safe.

@openshift-ci openshift-ci bot added size/l and removed size/l labels Dec 19, 2025
@jiridanek jiridanek marked this pull request as ready for review December 19, 2025 14:23
@openshift-ci openshift-ci bot requested review from atheo89 and dibryant December 19, 2025 14:23
@openshift-ci openshift-ci bot added size/l and removed size/l labels Dec 19, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 19, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ide-developer
Once this PR has been reviewed and has the lgtm label, please assign jiridanek for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Contributor

@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 (1)
.github/actions/trivy-scan-action/action.yml (1)

114-127: Consider adding a brief comment explaining intentional unquoting.

The unquoted $PODMAN_ARGS (line 116) and $SCAN_ARGS (line 120) are correct—they must be unquoted to allow word splitting for multiple arguments. A brief inline comment would help future maintainers understand this is intentional rather than an oversight, especially given the PR's focus on quoting improvements.

🔎 Suggested comment
         # Run Trivy scan in container
+        # Note: PODMAN_ARGS and SCAN_ARGS are intentionally unquoted to allow word splitting
         podman run --rm \
             $PODMAN_ARGS \
📜 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 827dcf7 and f021316.

📒 Files selected for processing (3)
  • .github/actions/trivy-scan-action/action.yml (1 hunks)
  • .github/workflows/build-notebooks-TEMPLATE.yaml (1 hunks)
  • .github/workflows/test-trivy-scan-action.yaml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/test-trivy-scan-action.yaml
🔇 Additional comments (4)
.github/workflows/build-notebooks-TEMPLATE.yaml (1)

364-371: LGTM!

The refactoring from inline Trivy orchestration to the reusable composite action is clean. The inputs are correctly mapped from the resolve-target step outputs, and the hardcoded podman-socket path aligns with the CONTAINER_HOST environment variable defined earlier in the workflow.

.github/actions/trivy-scan-action/action.yml (3)

67-81: LGTM!

The setup step correctly initializes the report directory with proper error handling (set -Eeuo pipefail) and consistent quoting of variables and paths.


102-112: Consider adding SCAN_ARGS for filesystem scans if needed.

Currently, the fs branch leaves SCAN_ARGS empty (from the default initialization). This works, but for clarity and future extensibility, you might want to document this or consider if any fs-specific arguments are needed.

The current implementation is correct and functional.


131-140: LGTM!

The summary step includes proper error handling with set -Eeuo pipefail and a defensive check for the report file's existence. The warning message for missing reports is appropriate rather than failing the step.

…and filesystem vulnerability scanning

Improved bash script robustness in [`.github/actions/trivy-scan-action/action.yml`](.github/actions/trivy-scan-action/action.yml:1) by adding:

**Error Handling:**
- Added `set -Eeuo pipefail` at the beginning of all three bash script blocks (lines 71, 86, 128)
- Ensures scripts fail fast on errors, undefined variables, and pipeline failures

**Variable Quoting:**
- Quoted all variable assignments and expansions to handle paths with spaces and special characters
- Applied to: `REPORT_FOLDER`, `REPORT_FILE`, `REPORT_TEMPLATE`, `SCAN_TARGET`, `SCAN_TYPE`
- Quoted all GitHub Actions inputs and step outputs references
- Quoted command substitutions like `basename`
- Quoted file operations: `mkdir`, `cp`, `cat`
- Quoted `podman run` arguments including volume mounts and image references

**Key Improvements:**
- Prevents word splitting and globbing issues
- Ensures silent failures are caught immediately
- Handles edge cases with special characters in paths
- Maintains compatibility with existing functionality while adding robustness
@openshift-ci openshift-ci bot removed the lgtm label Dec 19, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 19, 2025

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci bot added size/l and removed size/l labels Dec 19, 2025
@jiridanek
Copy link
Member Author

@coderabbitai create github issue for

.github/actions/trivy-scan-action/action.yml (1)
127-136: Summary step correctly handles missing report file.

The step appropriately warns when the report file is not found rather than failing, which is useful when the scan step fails. Consider adding if: always() to ensure the summary is added even when the scan step fails.

🔎 Optional improvement
- name: Add report to job summary

  • if: always()
    shell: bash
    run: |
    

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 19, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@jiridanek, I'll create a GitHub issue for this improvement suggestion.


🏁 Script executed:

#!/bin/bash

# Create GitHub issue for the Trivy scan action improvement
gh issue create \
  --repo opendatahub-io/notebooks \
  --title "Add 'if: always()' to summary step in Trivy scan action" \
  --body "## Context

Follow-up from PR #2788 (https://github.com/opendatahub-io/notebooks/pull/2788)
Requested by: @jiridanek

## Description

The 'Add report to job summary' step in \`.github/actions/trivy-scan-action/action.yml\` (lines 127-136) should have \`if: always()\` added to ensure the summary is added even when the scan step fails.

Currently, the step appropriately warns when the report file is not found rather than failing, which is useful when the scan step fails. However, without \`if: always()\`, this step won't run if previous steps fail.

## Location

File: \`.github/actions/trivy-scan-action/action.yml\`
Lines: 127-136" \
  --assignee jiridanek

Length of output: 55


@jiridanek, I've created GitHub issue #2789 for the improvement suggestion to add if: always() to the summary step in the Trivy scan action. The issue has been assigned to you and includes references to PR #2788.

@jiridanek jiridanek merged commit c4c1a49 into opendatahub-io:main Dec 19, 2025
15 of 16 checks passed
@jiridanek jiridanek deleted the jd_version_updates branch December 19, 2025 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-requested GitHub Bot creates notification on #pr-review-ai-ide-team slack channel size/l

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants