-
Notifications
You must be signed in to change notification settings - Fork 189
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
Pipeline template: Modify download test action to capture the number of containers #3182
Pipeline template: Modify download test action to capture the number of containers #3182
Conversation
7713dea
to
0b7b14c
Compare
0b7b14c
to
9eb7cfb
Compare
NXF_SINGULARITY_HOME_MOUNT: true | ||
run: nextflow run ./${{ env.REPOTITLE_LOWERCASE }}/$( sed 's/\W/_/g' <<< ${{ env.REPO_BRANCH }}) -stub -profile test,singularity --outdir ./results | ||
- name: Run the downloaded pipeline (stub run not supported) | ||
id: run_pipeline | ||
if: ${{ job.steps.stub_run_pipeline.status == failure() }} | ||
env: | ||
NXF_SINGULARITY_CACHEDIR: ./ | ||
NXF_SINGULARITY_CACHEDIR: ./singularity_container_images | ||
NXF_SINGULARITY_HOME_MOUNT: true | ||
run: nextflow run ./${{ env.REPOTITLE_LOWERCASE }}/$( sed 's/\W/_/g' <<< ${{ env.REPO_BRANCH }}) -profile test,singularity --outdir ./results{% endraw %}{% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can move the {% endif %}
to the end of the file to skip these tests. This is because skipping test_config
will not include the config files in the pipeline, test
and test_full
, so we can't run it using -profile test
. For the non-nf-core pipelines which don't have these profiles, we can only test downloading the containers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. I always wondered what that was actually for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after my comment is addressed :)
9eb7cfb
to
89961d6
Compare
else | ||
echo "The pipeline can be downloaded successfully!" | ||
fi | ||
{% endif %}{% endraw %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last comment, the order is important, otherwise the template would contain the raw {% endif %}
without interpreting it 🙂
{% endif %}{% endraw %} | |
{% endraw %}{% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ehm, then I screwed up anyway. Because the {% if test_config %}
is now also inside the {%raw%}
... 🫣...I know I had not understood how Jinja works.
The Download Test fails, if there are obvious issues while downloading a pipeline, e.g.
nf-core pipeline download
errors out.But it did not assert that all required containers were downloaded. Since it is impossible to take Github Action Runners offline for a certain step, it is quite difficult to test, if a pipeline would actually run offline.
But because Nextflow will pull container images at runtime that have not been downloaded correctly, the number of container images in the
$NXF_SINGULARITY_CACHEDIR
will change and this can be captured in the test.I have been testing that action in my fork of the testpipeline after manually removing the
{%raw%}
markup, but maybe there is a better way?PS: I have been adding the
{%raw%}
markup mannually to the new steps. I have no idea if the new steps should also be wrapped in the{% if test_config %}
or not.PR checklist
CHANGELOG.md
is updated