Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: run tasks conditionally #3159

Merged
merged 13 commits into from
Jan 31, 2025

Conversation

wescopeland
Copy link
Member

@wescopeland wescopeland commented Jan 30, 2025

This PR makes an adjustment to our pull request CI workflow.

The meat of the PR is here:

changes:
  runs-on: ubuntu-22.04
  outputs:
    php: ${{ steps.filter.outputs.php }}
    node: ${{ steps.filter.outputs.node }}
  steps:
    - name: Check changed files
      uses: dorny/paths-filter@v2
      id: filter
      with:
        filters: |
          php:
            - '**/*.php'
            - 'composer.json'
            - 'composer.lock'
            - 'phpstan.neon'
            - 'pint.json'
          node:
            - '**/*.js'
            - '**/*.ts'
            - '**/*.tsx'
            - 'package.json'
            - 'pnpm-lock.yaml'
            - '*eslint*'
            - 'tailwind.config.json'

If something in the back-end layer is touched, PHP tasks will run.
If something in the front-end layer is touched, Node.js tasks will run.
Otherwise, tasks are marked as skipped (verifiable by clicking on the task icons in this PR):

Screenshot 2025-01-30 at 6 12 33 PM

The output of composer types is the only glue between Laravel and the React UI, and changes there will always be caught by this filter (requiring all Node.js tasks to pass).

We should probably make a similar change to our pre-push job, but given the logic there will likely be very different to achieve the same functionality, let's push that to a subsequent PR.

@wescopeland wescopeland marked this pull request as ready for review January 30, 2025 23:01
@wescopeland wescopeland marked this pull request as draft January 30, 2025 23:01
@wescopeland wescopeland requested a review from a team January 30, 2025 23:14
@wescopeland wescopeland marked this pull request as ready for review January 30, 2025 23:14
@wescopeland
Copy link
Member Author

I've verified this is functional and now ready for review.

@Jamiras
Copy link
Member

Jamiras commented Jan 31, 2025

Is this going to block merging?
image

@wescopeland
Copy link
Member Author

I don't think so. Based on my research, GitHub Actions is smart enough to detect that a "depends on" task has been skipped. Things may look different after this PR is merged to master, and if it's still complaining I can investigate further.

@wescopeland wescopeland merged commit f035d6e into RetroAchievements:master Jan 31, 2025
5 checks passed
@wescopeland wescopeland deleted the conditional-ci-tasks branch January 31, 2025 22:04
@wescopeland
Copy link
Member Author

I've made a follow-up tweak in cc82868 to ensure we can keep our 6 jobs as required.

Rather than skipping the required jobs, they now execute and instantly succeed if they're not needed for the branch.

@wescopeland
Copy link
Member Author

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