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

use dorny/paths-filter with git commands instead of GitHub API #3969

Merged
merged 10 commits into from
Oct 13, 2023

Conversation

mirpedrol
Copy link
Member

From the docs:

# Personal access token used to fetch a list of changed files
# from GitHub REST API.
# It's only used if action is triggered by a pull request event.
# GitHub token from workflow context is used as default value.
# If an empty string is provided, the action falls back to detect
# changes using git commands.

@edmundmiller
Copy link
Contributor

edmundmiller commented Oct 12, 2023

@adamrtalbot any idea why this is failing? I'm stumped.

What's the point of the check for the checks just for the debug print?

@adamrtalbot
Copy link
Contributor

On my phone but do you mean the confirm-pass? It's the only way of enforcing branch protection I can find at the moment. You can't force checks for variable name tests.

@mirpedrol
Copy link
Member Author

It is detecting too many changed files, no? Testing dumpsoftwareversions, fastqc, snakemake and samtools

@mashehu
Copy link
Contributor

mashehu commented Oct 13, 2023

It is detecting too many changed files, no? Testing dumpsoftwareversions, fastqc, snakemake and samtools

Yes, but somehow only from nf-test, so something not perfectly clean with the setup there.

@adamrtalbot
Copy link
Contributor

adamrtalbot commented Oct 13, 2023

It has detected every file has been added, but the pytest check changes cant create an output object because there are too many (>256), so it fails the test because they did not work.

The check-changes doesn't work correctly yet, it detects all files are new and have been added.

@adamrtalbot
Copy link
Contributor

Pytest changes:
image

@mirpedrol
Copy link
Member Author

After some research, I found that there is a small problem when using git commands to retrieve changed files. There's a PR opened with the fix already. Same user created a fork of paths-filter with the fix.
This is now detecting only files changed in this PR, but it includes the modules coming from merging branch master to the current branch.
Do you think it's a good option to use this fork until the original GHA is fixed?

@mashehu
Copy link
Contributor

mashehu commented Oct 13, 2023

Yes, the changes in the PR look reasonable. Let's go for it

@mirpedrol mirpedrol enabled auto-merge October 13, 2023 14:44
@mirpedrol mirpedrol disabled auto-merge October 13, 2023 17:43
@mirpedrol mirpedrol merged commit e639122 into nf-core:master Oct 13, 2023
75 of 79 checks passed
@mirpedrol mirpedrol deleted the pathsfilter branch October 13, 2023 17:45
lrauschning pushed a commit to lrauschning/modules that referenced this pull request Oct 16, 2023
…re#3969)

* use dorny/paths-filter with git commands instead of GitHub API

* detect changes against master branch

* Revert "detect changes against master branch"

This reverts commit 5b6255e.

* try github.ref as base

* testing frouioui's solution to the same problem

* add todo comment

---------

Co-authored-by: Matthias Hörtenhuber <mashehu@users.noreply.github.com>
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.

4 participants