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

Fix logic to run tests on pushes #232

Merged
merged 1 commit into from
Oct 16, 2023
Merged

Conversation

nickvergessen
Copy link
Member

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen requested a review from kesselb October 16, 2023 09:13
@nickvergessen nickvergessen self-assigned this Oct 16, 2023
@nickvergessen nickvergessen added 3. to review Waiting for reviews bug Something isn't working labels Oct 16, 2023
@kesselb
Copy link
Contributor

kesselb commented Oct 16, 2023

Well spotted 👍

paths-filter needs a git checkout for push because the api, to fetch changed files, only works for pull requests.

For lint and node we could merge the changes step into the build step because we usually don't need a matrix and run the path filter after the git checkout.

For phpunit* we need a git checkout before running the path filter or continue on error.

Alternative: We don't run the workflow for pushes anymore.

@nickvergessen
Copy link
Member Author

Alternative: We don't run the workflow for pushes anymore.

We want the results of pushes, so it's easier to find the first failing state etc, e.g. when 2 parallel PRs cause issues.

@kesselb
Copy link
Contributor

kesselb commented Oct 16, 2023

- name: Checkout
  if: ${{ github.event_name == 'push' }}
  uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3

Instead of continue_on_error maybe?

@nickvergessen
Copy link
Member Author

Instead of continue_on_error maybe?

We could, but that would mean we don't check the result on each PR anymore, but only such that changed something. Also would mean translation updates don't run CI anymore, so instead I favor to always run the CI checks on such pushes.

Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

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

👍

@nickvergessen nickvergessen merged commit 9b0e0b7 into master Oct 16, 2023
3 checks passed
@nickvergessen nickvergessen deleted the bugfix/noid/fix-pushes branch October 16, 2023 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants