Skip to content

Conversation

@LuisUrrutia
Copy link
Contributor

@LuisUrrutia LuisUrrutia commented Oct 3, 2025

Summary

Force run of docker scan, since there is a known bug of if conditions not working when previous steps (ci in this case) are skipped.

Testing Process

We need to test it by merging it.

Checklist

  • Add a reference to related issues in the PR description.
  • Add unit tests if applicable.

Summary by CodeRabbit

  • Chores
    • Improved CI workflow reliability by refining conditions for when the Docker security scan runs.
    • Ensures scans only execute after core checks complete successfully, reducing false positives and unnecessary runs.
    • Clarifies job dependencies within the pipeline for more predictable outcomes and cleaner build reports.

@LuisUrrutia LuisUrrutia requested a review from a team as a code owner October 3, 2025 07:22
@coderabbitai
Copy link

coderabbitai bot commented Oct 3, 2025

Walkthrough

The GitHub Actions workflow updates modify the docker-scan job’s conditional to include always() and an explicit check for needs.ci.result == 'success', ensuring docker-scan evaluates only after CI completes and runs only when CI succeeds, the PR is not a draft, and relevant docker files changed.

Changes

Cohort / File(s) Summary of changes
CI workflow gating
\.github/workflows/ci.yaml
Expanded docker-scan job conditions: added always() wrapper and explicit needs.ci.result == 'success', while retaining checks for non-draft PR and changed docker files; applied consistently wherever docker-scan is triggered.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Dev as Developer
  participant GH as GitHub Actions
  participant CI as Job: ci
  participant DS as Job: docker-scan

  Dev->>GH: Open/Update PR
  GH->>CI: Run CI job
  CI-->>GH: Result (success/failure)

  Note over GH: Evaluate docker-scan condition<br/>always() && needs.ci.result == 'success'<br/>&& !draft && docker-files changed

  alt CI success AND conditions met
    GH->>DS: Start docker-scan
    DS-->>GH: docker-scan result
  else Otherwise
    GH-->>DS: Skip docker-scan
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I thump my paws at pipelines neat,
Gates aligned, dependencies meet.
When CI’s green, I gladly scan,
If not—I'll nap, a prudent plan.
Hop, hop! Conditions in a row—
Only then to Docker I go. 🐇🚀

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The description adheres to the repository’s template by including a Summary, Testing Process, and Checklist, but it remains too generic: it lacks a reference to a related issue, the testing steps are placeholder prose rather than concrete instructions, and the checklist items are left unchecked. Please add a link or identifier for the related bug or issue, expand the Testing Process section with concrete verification steps, and mark or complete the checklist items to demonstrate that issue references and tests have been addressed.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title “chore: Force run of docker-scan” directly reflects the main change of altering the CI workflow to always trigger the docker-scan job and clearly communicates the purpose in a concise, conventional format.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch plat-6955-docker-scan-step-is-not-running-when-dockerfiles-are

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

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d60a019 and 94eec28.

📒 Files selected for processing (1)
  • .github/workflows/ci.yaml (1 hunks)
⏰ 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). (5)
  • GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
  • GitHub Check: Analyze (rust)
  • GitHub Check: Redirect rules - openzeppelin-relayer
  • GitHub Check: Header rules - openzeppelin-relayer
  • GitHub Check: Pages changed - openzeppelin-relayer

- ci
if: |
${{ github.event.pull_request.draft == false && needs.changed_files.outputs.changed-docker-files == 'true' }}
${{ always() && github.event.pull_request.draft == false && needs.changed_files.outputs.changed-docker-files == 'true' && needs.ci.result == 'success' }}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Condition still blocks docker-scan when ci is skipped

The new guard still requires needs.ci.result == 'success'. When ci is skipped (the scenario called out in the PR description), its result is 'skipped', so this expression evaluates to false and the job is skipped again. The added always() never gets to help. To actually force the scan when ci doesn’t run, allow 'skipped' as an acceptable result (e.g. in(['success','skipped'], needs.ci.result)) or drop the equality altogether.

🤖 Prompt for AI Agents
.github/workflows/ci.yaml around line 291: the job’s if condition still requires
needs.ci.result == 'success', so when the CI job is skipped the expression is
false and docker-scan is skipped; update the condition to allow a skipped CI by
replacing that equality with a membership check (e.g. in(['success','skipped'],
needs.ci.result)) or remove the needs.ci.result check entirely so docker-scan
runs when changed-docker-files is true and the other guards pass.

@son-oz
Copy link
Contributor

son-oz commented Oct 3, 2025

@LuisUrrutia actually I'm thinking about moving docker-scan to the release-docker.yml workflow instead.
@tirumerla wdyt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants