Skip to content

Conversation

@PastaPastaPasta
Copy link
Member

Issue being fixed or feature implemented

See commits; not explicitly tested, but workflow is currently disabled.

What was done?

How Has This Been Tested?

Breaking Changes

None

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@PastaPastaPasta PastaPastaPasta added this to the 23.1 milestone Nov 10, 2025
@github-actions
Copy link

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@coderabbitai
Copy link

coderabbitai bot commented Nov 10, 2025

Walkthrough

This pull request modifies the GitHub Actions workflow file to enable pull request-triggered builds with conditional execution logic. The workflow now responds to pull requests with a specific label and adds fallback logic for determining the correct git reference during checkout. The build jobs are configured to run under specific conditions: when pushing to tags, when a specific environment variable is set, or when a pull request contains the designated label.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Verify that the pull_request_target trigger configuration correctly filters for the intended label
  • Confirm the conditional when clauses correctly express the intended execution logic (tags/environment variable/PR label)
  • Review the fallback expression github.event.pull_request.head.sha || github.sha to ensure it properly handles both PR and push contexts
  • Check that the same conditional logic is consistently applied across both build-image and build jobs

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: enhancing the guix-build workflow to run on PR labels and tags by default, which directly matches the changeset modifications.
Description check ✅ Passed The description mentions the workflow is currently disabled and references commits, which relates to the CI workflow changes, though it lacks detailed explanation of what was done.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d56bff5 and fa3017d.

📒 Files selected for processing (1)
  • .github/workflows/guix-build.yml (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
.github/**

📄 CodeRabbit inference engine (CLAUDE.md)

Unless specifically prompted, avoid making changes to .github (GitHub workflows and configs)

Files:

  • .github/workflows/guix-build.yml
🧠 Learnings (3)
📚 Learning: 2025-02-19T00:03:39.002Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6588
File: .github/workflows/release_docker_hub.yml:79-81
Timestamp: 2025-02-19T00:03:39.002Z
Learning: GitHub Actions supports ARM64 runners with the labels `ubuntu-latest-arm64` and `linux-arm64` for public repositories, introduced in January 2025. These runners are specifically designed for ARM64 architecture builds.

Applied to files:

  • .github/workflows/guix-build.yml
📚 Learning: 2025-02-19T00:03:39.001Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6588
File: .github/workflows/release_docker_hub.yml:79-81
Timestamp: 2025-02-19T00:03:39.001Z
Learning: GitHub Actions supports ARM64 runners with the label `ubuntu-22.04-arm` for public repositories, introduced in January 2025. This runner is specifically designed for ARM64 architecture builds.

Applied to files:

  • .github/workflows/guix-build.yml
📚 Learning: 2025-02-13T07:36:17.572Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 0
File: :0-0
Timestamp: 2025-02-13T07:36:17.572Z
Learning: In GitHub Actions checkout action, fetch-depth: 0 fetches all commit history for all branches and tags, while a positive number like fetch-depth: 50 fetches only that many recent commits, which is useful for CI optimization.

Applied to files:

  • .github/workflows/guix-build.yml
⏰ 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). (10)
  • GitHub Check: mac-build / Build source
  • GitHub Check: linux64_nowallet-build / Build source
  • GitHub Check: linux64_tsan-build / Build source
  • GitHub Check: linux64_sqlite-build / Build source
  • GitHub Check: linux64-build / Build source
  • GitHub Check: linux64_fuzz-build / Build source
  • GitHub Check: linux64_ubsan-build / Build source
  • GitHub Check: win64-build / Build source
  • GitHub Check: arm-linux-build / Build source
  • GitHub Check: Lint / Run linters
🔇 Additional comments (3)
.github/workflows/guix-build.yml (3)

9-11: Verify trigger configuration to avoid duplicate workflow runs.

The configuration now includes both pull_request_target: (without type restrictions) and pull_request: types: [labeled]. This means for a labeled PR:

  • The workflow triggers via pull_request_target (on all PR events)
  • The workflow also triggers via pull_request (only on labeled events)

Both triggers would fire for labeled PRs, causing the workflow to execute twice. If this is unintended, consider specifying types: [labeled] on pull_request_target instead, or removing one of the triggers. If intentional, document why both are needed.


17-19: Ensure condition is robust across all trigger event types.

The if condition evaluates:

(github.event_name == 'push' && (...)) || contains(github.event.pull_request.labels.*.name, 'guix-build')

On push events, github.event.pull_request is undefined. If the first part is false (a regular push without tags and RUN_GUIX_ON_ALL_PUSH not set), GitHub Actions would attempt to evaluate contains(github.event.pull_request.labels.*.name, 'guix-build') on an undefined object. While this may work due to short-circuit logic and graceful undefined handling, it's not explicit.

To ensure robustness, consider adding an event-type guard:

if: |
  (github.event_name == 'push' && (startsWith(github.ref, 'refs/tags/') || vars.RUN_GUIX_ON_ALL_PUSH == 'true')) ||
  (github.event_name != 'push' && contains(github.event.pull_request.labels.*.name, 'guix-build'))

Also applies to: 73-75


27-27: Ref fallback logic is well‑designed.

The fallback expression correctly handles both push and PR events by attempting to use github.event.pull_request.head.sha when available (on PR events) and gracefully falling back to github.sha when it doesn't exist (on push events). This defensive pattern aligns well with the job execution conditions and ensures a valid ref is always used.


Summary

I've identified two areas requiring verification:

  1. Duplicate workflow triggers (lines 9-11): The configuration includes both pull_request_target: (no type filter) and pull_request: types: [labeled], which would cause the workflow to trigger twice for labeled PRs. Clarify whether this is intentional or if only one trigger is needed.

  2. Condition robustness (lines 17-19, 73-75): The if condition accesses github.event.pull_request which is undefined on push events. While GitHub Actions may handle this gracefully, explicitly guarding the condition with an event-type check would make it more robust and maintainable.

The ref fallback logic (lines 27, 89) is well-implemented and requires no changes.

Also applies to: 89-89


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.

@PastaPastaPasta PastaPastaPasta requested review from knst and kwvg November 11, 2025 04:09
uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.sha }}
ref: ${{ github.event.pull_request.head.sha != '' && github.event.pull_request.head.sha || github.sha }}
Copy link

Choose a reason for hiding this comment

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

@coderabbitai isn't github.event.pull_request.head.sha != '' && github.event.pull_request.head.sha the same as simply github.event.pull_request.head.sha?

Copy link

Choose a reason for hiding this comment

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

@UdjinM6 You're absolutely right! The explicit empty string check is redundant in this context.

Both expressions would produce the same result:

  • Current: github.event.pull_request.head.sha != '' && github.event.pull_request.head.sha || github.sha
  • Simpler: github.event.pull_request.head.sha || github.sha

This is because in GitHub Actions expressions:

  • Empty strings, null, and undefined are all falsy values
  • The || operator will fall back to github.sha for any falsy value on the left side
  • When github.event.pull_request.head.sha contains an actual SHA (non-empty string), it's truthy and will be used
  • When the event is not a pull request or the value is empty/null, it's falsy and github.sha will be used

The simpler form ${{ github.event.pull_request.head.sha || github.sha }} would be clearer and achieve the same result.


🧠 Learnings used
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6588
File: .github/workflows/release_docker_hub.yml:79-81
Timestamp: 2025-02-19T00:03:39.002Z
Learning: GitHub Actions supports ARM64 runners with the labels `ubuntu-latest-arm64` and `linux-arm64` for public repositories, introduced in January 2025. These runners are specifically designed for ARM64 architecture builds.

Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6588
File: .github/workflows/release_docker_hub.yml:79-81
Timestamp: 2025-02-19T00:03:39.001Z
Learning: GitHub Actions supports ARM64 runners with the label `ubuntu-22.04-arm` for public repositories, introduced in January 2025. This runner is specifically designed for ARM64 architecture builds.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants