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: Update module_name filter step #7106

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

edmundmiller
Copy link
Contributor

Created a small test script to prevent re-running into edge-cases and to quickly add new test cases. If we don't hate the idea I think we can wire it up to the javascript file to avoid any copy-paste errors.

@edmundmiller edmundmiller force-pushed the modules_filter_function branch from 701fd07 to f75b3fa Compare November 27, 2024 16:27
mashehu
mashehu previously approved these changes Nov 27, 2024
@edmundmiller edmundmiller marked this pull request as ready for review November 27, 2024 16:40
@edmundmiller edmundmiller requested a review from a team as a code owner November 27, 2024 16:40
@edmundmiller edmundmiller marked this pull request as draft November 27, 2024 16:40
MatthiasZepper
MatthiasZepper previously approved these changes Nov 27, 2024
Copy link
Member

@MatthiasZepper MatthiasZepper left a comment

Choose a reason for hiding this comment

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

I have now only tested the script in isolation, but the test cases pass for me locally as well. Thanks for the solid work!

@MatthiasZepper
Copy link
Member

Looking at the CI failure in this PR...can it be that it was only the missing bracket in the .map() function that caused all the issues?

      - name: Get module name
        uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7
        id: module_names
        with:
          script: |
            return [...new Set(${{ steps.filter.outputs.modules_files }}
                .map(path => path
                .replace('tests/', '')
                .replace('modules/nf-core/', '')
                .split('/')
                .slice(0, 2)
                .filter(x => !x.startsWith('main.nf') && x !== 'tests' && x !== 'meta.yml' && x !== 'environment.yml')
                .join('/'))
              )
            ];

@edmundmiller edmundmiller force-pushed the modules_filter_function branch from 9711ad4 to b016c5a Compare December 12, 2024 14:54
- Added detailed debug logging in get_module_path.js to trace input processing and error handling.
- Updated lint.yml to log raw and parsed module files, as well as the result from the script execution.

These changes improve visibility into the workflow's operation and facilitate troubleshooting.
Modified the conditional statement in the lint.yml workflow to ensure it checks for non-empty module files using '&&' instead of '||'. This change enhances the accuracy of the workflow's execution criteria, ensuring it only runs when valid module files are present.
@edmundmiller edmundmiller force-pushed the modules_filter_function branch from c776fbe to ec6e796 Compare December 12, 2024 16:15
@edmundmiller edmundmiller force-pushed the modules_filter_function branch from ec6e796 to a460e24 Compare December 12, 2024 16:20
- Updated get_module_path.js to ensure proper JSON formatting and added logging for the final output.
- Modified lint.yml to parse and log the module names output correctly, improving visibility into the workflow's execution.

These changes improve error handling and debugging capabilities in the CI workflow.
@edmundmiller edmundmiller marked this pull request as ready for review December 12, 2024 17:25
@edmundmiller edmundmiller dismissed stale reviews from MatthiasZepper and mashehu via 8f619f6 December 13, 2024 18:05
Copy link
Member

@MatthiasZepper MatthiasZepper left a comment

Choose a reason for hiding this comment

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

I think, that will do. Nice work!

Zizmor nags that steps.module_names.outputs.result may expand into attacker-controllable code, but I don't really see how that could be exploited, because the inputs must be a valid file path. And in the worst case, nf-core modules lint is just called with some invalid module.

@@ -1,3 +1,3 @@
process {
ext.args = { "--proband earlycasualcaiman --father hugelymodelbat --mother slowlycivilbuck --af-tag AF regions" }
ext.args = { "--proband earlycasualcaiman --father hugelymodelbat --mother slowlycivilbuck --af-tag AF regions" }
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how this ended up in the PR, but otherwise that looks good to me.

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