+
Skip to content

Conversation

MatthiasZepper
Copy link
Member

@MatthiasZepper MatthiasZepper commented Jan 8, 2025

In #3351, I refactored the nf-core pipelines download test to use $GITHUB_OUTPUT instead of the environment for improved security.

However, splitting the test into two independent jobs introduced the possibility of them running on different runners, which resulted in the second job lacking the correct setup. I have now resolved this issue by ensuring that all necessary setup setups are retained within the main job. Only the potentially vulnerable step remains isolated from the main job, but also does not require any setup.

The new version of the CI pipeline was successfully tested in my Test pipeline repository. Any further independent tests are highly appreciated.

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

@ewels ewels added this to the 3.1.2 milestone Jan 9, 2025
@MatthiasZepper MatthiasZepper force-pushed the no_env_in_download_action branch from e03dbc0 to 5aee4b1 Compare January 9, 2025 12:05
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.16%. Comparing base (570ae2b) to head (737b515).
Report is 5 commits behind head on dev.

Additional details and impacted files

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MatthiasZepper MatthiasZepper marked this pull request as ready for review January 10, 2025 13:18
@MatthiasZepper MatthiasZepper force-pushed the no_env_in_download_action branch from 5aee4b1 to c67027d Compare January 10, 2025 13:20
Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

@MatthiasZepper
Copy link
Member Author

Great! I think, it was by now already manually incorporated to the development version of the Taxprofiler pipeline, so that also suggests it does now work.

Also tested with the nf-core/testpipeline https://github.com/nf-core/testpipeline/actions/runs/12744809455/job/35517501873?pr=101

Did you by chance also test the conditional part? Because I have never created a pipeline with a cherry-picked template, and there is this {% if test_config %} clause in there that I do not understand at all. I vaguely recall you one mentioned that it allows customizing the template in the creation process, but I have not tested the non-default action. Would that then be only the upper part without everying enclosed between {% if test_config %} and {% endif %}?

@mirpedrol
Copy link
Member

Exactly, it won't contain anything that is between {% if test_config %} and {% endif %}. I haven't tested that case, but it should be ok as it will only test the first part of the action, which is passing on the tests pe made

@MatthiasZepper MatthiasZepper force-pushed the no_env_in_download_action branch from c67027d to 51720db Compare January 13, 2025 14:38
@MatthiasZepper MatthiasZepper force-pushed the no_env_in_download_action branch from 51720db to 737b515 Compare January 13, 2025 17:11
@MatthiasZepper MatthiasZepper merged commit 65969d9 into nf-core:dev Jan 13, 2025
88 checks passed
@MatthiasZepper MatthiasZepper deleted the no_env_in_download_action branch January 13, 2025 18:26
fbdtemme added a commit to nf-core/pixelator that referenced this pull request Jan 16, 2025
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

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载