-
Notifications
You must be signed in to change notification settings - Fork 717
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
Dev -> Master for v3.6 release #773
Conversation
Bump pipeline version to 3.6dev
Co-authored-by: Harshil Patel <[email protected]>
Patch RSEM_PREPAREREFERENCE configuration
nf-core modules update
Sync pipeline with nf-core/tools dev to fix linting
Pipeline updates / addressing open issues
Make --outdir mandatory, tin.py opt-in and fix #750
Bump pipeline version to 3.6
|
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 good job! Made a comment about the outdir directory, probably this has been discussed already and I missed it.
@@ -19,7 +19,7 @@ Learn more about contributing: [CONTRIBUTING.md](https://github.com/nf-core/rnas | |||
- [ ] If you've added a new tool - have you followed the pipeline conventions in the [contribution docs](https://github.com/nf-core/rnaseq/tree/master/.github/CONTRIBUTING.md) | |||
- [ ] If necessary, also make a PR on the nf-core/rnaseq _branch_ on the [nf-core/test-datasets](https://github.com/nf-core/test-datasets) repository. | |||
- [ ] Make sure your code lints (`nf-core lint`). | |||
- [ ] Ensure the test suite passes (`nextflow run . -profile test,docker`). | |||
- [ ] Ensure the test suite passes (`nextflow run . -profile test,docker --outdir <OUTDIR>`). |
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.
I guess this has been discussed already, but why not set the parameter in the test.config
? I don't see the point of providing it several times in the ci.yml
file, plus as user, the command is simpler to run tests
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.
After, reviewing the whole code, I see that the reason is that outdir
param is removed from the nextflow.config
but still I think it should be set by default in the test.config
.
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.
If we set it to ./results
in the test.config
then we end up in the same position where we end up using relative paths when -profile test
is used in the Cloud? The main problem is that there isn't a sensible default that works yet.
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.
I see the cloud part is what I missed, fair enough
No description provided.