Template update for nf-core/tools v2.13.1 #26
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This was added by us but now it's causing a lint error. I don't see this entry in the schema of any other pipelines so it is probably safe to remove.
This change removes the hard-coded local input paths in the top level workflow and replaces them with values read from an actual sample sheet. The test profile has been modified to pass in a minimal sample sheet. The ashlar module was updated to fix a bug that crashed the versions.yml parser. The marker csv URL is not ideal but using the one from test-datasets would have required a more wholesale rewriting of the sample sheet organization. That rewrite had already been implemented in a separate branch to be merged as soon as this template update is accepted.
kbestak
reviewed
Mar 21, 2024
There was a problem hiding this comment.
Overall it looks great, left some small comments.
I just want to highlight that in the Gitpod environment, the pipeline fails (nextflow run . -profile docker,test) due to an ongoing bug with the MultiQC container (fails to pull in Gitpod for versions 1.18 and above).
I also got a linting error in Gitpod for the logo changes (files_unchanged), but it seems to pass the CI linting test.
| } | ||
|
|
||
| process { | ||
| withName: ".*:DEEPCELL_MESMER" { |
There was a problem hiding this comment.
Why is ".*:DEEPCELL_MESMER" required instead of just "DEEPCELL_MESMER"?
There was a problem hiding this comment.
I tried just the simple module name but the settings weren't applied! You can see the failed CI run on the previous commit where I didn't have the wildcard match. 🤣
conf/modules.config
Outdated
| saveAs: { filename -> filename.equals('versions.yml') ? null : filename } | ||
| ] | ||
| } | ||
|
|
||
| withName: CUSTOM_DUMPSOFTWAREVERSIONS { |
There was a problem hiding this comment.
CUSTOM_DUMPSOFTWAREVERSIONS is also removed with the new template so that section can also be excluded here.
There was a problem hiding this comment.
Since fastqc generally won't be used by this pipeline, I think it should be excluded. Nothing critical right now and I understand how keeping it would make soon to be released template updates slightly easier to merge.
There was a problem hiding this comment.
Another good catch, also done.
|
I noticed the local lint failure on the images too. I just ran |
jmuhlich
force-pushed
the
nf-core-template-merge-2.13.1
branch
from
063c5a2
to
fb4273e
Compare
|
Nope, with those updated images the CI lint check fails! I force-pushed the previous commit so all checks should be passing again. |
|
I found some people in Slack who had the same inconsistency issues with nf-core lint and the logos. I submitted an issue with my findings: nf-core/tools#2893 |
|
I can't speak for the details of the groovy code, slowly getting into it, but in general I really like the subworkflows that check input and format output. It is much more intuitive, at least for me :). Great work @jmuhlich |
kbestak
approved these changes
Mar 21, 2024
|
I think the EditorConfig and Prettier tests were previously required but have been phased out with the new tools update, so they'll never be run here. I'll ask some nf-core folks how to proceed. |
Thanks Jose! Those new subworkflows are all from the new nf-core tools release and template updates, so all credit to the tools team. |
|
OK! Maxime adjusted our required checks to remove the stale ones, so we're good to go. |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolved conflicts in automated PR.