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.

Sign up for GitHub

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

Jump to bottom

Fix bcl2fastq and bclconvert publishDir #157

Merged
merged 4 commits into from
Dec 11, 2023
Merged

Fix bcl2fastq and bclconvert publishDir #157

merged 4 commits into from
Dec 11, 2023

Conversation

Aratz
Copy link
Contributor

@Aratz Aratz commented Dec 11, 2023 •
edited
Loading

This PR addresses the following issues:

  • bcl2fastq's publishDir was set to {Reports, Stats} (note the space before Stats), which caused the Stats folder to be excluded (see here for more details on globs).
  • RunInfo.xml is not listed as an output of the bcl2fastq/bclconvert modules and will not appear in the output directory, even if it is specified in publisDir (which is fine because RunInfo.xml is actually an input file generated by the instrument for the demultiplexer). This PR removes this file from publishDir.
  • Update nf-test to 0.8.2 and update snapshot of compressed files (see calculate md5 hash of .gz files based on file content askimed/nf-test#144 for more details).

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • CHANGELOG.md is updated.

@Aratz
Fix bcl2fastq publishdir
8a6ccd2 
There should be no space after a comma when using `{xx,yy}` in a glob.

This also fixes the bcl2fastq test snapshot
@Aratz
Remove RunInfo.xml from publishDir
00f10ed 
RunInfo.xml is not an output file of bclconvert nor bcl2fastq, it is
generated by the instrument.
@Aratz
Update nf-test to 0.8.2
9029542
@Aratz Aratz changed the title Fix publishdirs Fix bcl2fastq and bclconvert publishDir Dec 11, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 11, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 4553be6

+| ✅ 156 tests passed       |+
#| ❔   2 tests were ignored |#
!| ❗   4 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in README.md: Describe the minimum required steps to execute the pipeline, e.g. how to prepare samplesheets.
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your preferred methods description, e.g. add publication citation for this pipeline
  • pipeline_todos - TODO string in awsfulltest.yml: You can customise AWS full pipeline tests as required
  • pipeline_todos - TODO string in WorkflowDemultiplex.groovy: Optionally add in-text citation tools to this list.

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.10
  • Run at 2023-12-11 16:24:52

@Aratz Aratz marked this pull request as ready for review December 11, 2023 16:23
@Aratz Aratz requested a review from a team as a code owner December 11, 2023 16:23
@Aratz Aratz requested review from glichtenstein and removed request for a team December 11, 2023 16:23
@maxulysse
Copy link
Member

Looking good, thanks @Aratz

@maxulysse
Copy link
Member

Can you maybe update the modules to close #156 while you're at it?

@Aratz
Copy link
Contributor Author

Aratz commented Dec 11, 2023

Yes, I have it ready on my work computer actually, but decided to just start with these commits to make reviewing this PR easier since the module update affects many different files and snapshots. Will push tomorrow 👍

edmundmiller
edmundmiller approved these changes Dec 11, 2023
View reviewed changes
Copy link
Collaborator

@edmundmiller edmundmiller left a comment

Choose a reason for hiding this comment

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

Looks good to me! Let's leave the version bump to seperate PR.

@edmundmiller edmundmiller merged commit a766eb0 into nf-core:dev Dec 11, 2023
6 checks passed
@Aratz Aratz mentioned this pull request Dec 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxulysse maxulysse maxulysse approved these changes

@edmundmiller edmundmiller edmundmiller approved these changes

@glichtenstein glichtenstein Awaiting requested review from glichtenstein glichtenstein is a code owner automatically assigned from nf-core/demultiplex

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Aratz @maxulysse @edmundmiller