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: Fix paths filter #5212

Closed
wants to merge 1 commit into from
Closed

ci: Fix paths filter #5212

wants to merge 1 commit into from

Conversation

relrelb
Copy link
Contributor

@relrelb relrelb commented Sep 3, 2021

Multiple patterns on one filter uses OR - it's enough that any file is matched by at least one pattern, and the filter becomes true.
This means the previous Rust filter became true even when only Web files were modified.

Use an extended glob feature to make the patterns work like an AND.

Multiple patterns on one filter uses OR - it's enough that any file
is matched by at least one pattern, and the filter becomes `true`.
This means the previous Rust filter became `true` even when only
Web files were modified.

Use an extended glob feature to make the patterns work like an AND.
@midgleyc
Copy link
Contributor

midgleyc commented Sep 4, 2021

This fixes the problem with checks running when they shouldn't.

There are still two problems with this, as you can see by the failed checks ;).

First is that we should probably have the web checks run when the web workflow gets modified (and the rust checks run when the rust workflow gets modified, which already happens), because that is a "change" to how the web build works.

Second is that the required web checks aren't marked as run, because the skipped check just looks like

Test Web / Test Node.js ${{ matrix.node_version }} / Rust ${{ matrix.rust_version }} / ${{ matrix.os }}
instead of having the full name.

There are two things I can think of doing:

  • move the path filter under the build matrix, then add ifs to all remaining steps to skip them if the filters didn't match.
  • create more builds with the names Test Node.js 12 / Rust stable / ubuntu-latest for each version / os combo, and run them only if the filters didn't match
  • duplicate the strategy matrix, and have a second build named Test Web / Test Node.js ${{ matrix.node_version }} / Rust ${{ matrix.rust_version }} / ${{ matrix.os }} that runs only if the filters match.

The last two rely on GH requiring a build with the name Test Node.js 12 / Rust stable / ubuntu-latest to pass, so they run a separate build with the same name.

@adrian17
Copy link
Collaborator

adrian17 commented Sep 6, 2021

FYI, Mike disabled check requirement for beta windows/mac.
Does this mean we can go back to #5103 , or are there any other leftover issues?
Edit: oh right, nvm, this doesn't really help with this at all doesn't it 😅 Sorry for confusion.

@relrelb relrelb closed this Sep 20, 2021
@relrelb relrelb deleted the ci branch September 20, 2021 12:41
@n0samu n0samu mentioned this pull request Aug 26, 2023
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.

3 participants