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

Refactor how versions are handled in pipeline #252

Merged
merged 20 commits into from
Jan 18, 2024
Merged

Refactor how versions are handled in pipeline #252

merged 20 commits into from
Jan 18, 2024

Conversation

drpatelh
Copy link
Member

@drpatelh drpatelh commented Jan 17, 2024 •
edited
Loading

Bringing in changes in-line with those I made on nf-aggregate to refactor the way versions are handled.

  • Need to push the same changes in this PR to nf-core/modules for any nf-core modules / subworkflows
  • Add nf-test for subworkflows/local/utils_nfcore_fetchngs_pipeline/main.nf
  • Test --email functionality works
  • Test --hook_url functionality works

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 17, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit e3d81da

+| ✅ 134 tests passed       |+
#| ❔  14 tests were ignored |#
!| ❗   6 tests had warnings |!

❗ Test warnings:

  • files_exist - File not found: assets/multiqc_config.yml
  • files_exist - File not found: conf/igenomes.config
  • files_exist - File not found: .github/workflows/awstest.yml
  • files_exist - File not found: .github/workflows/awsfulltest.yml
  • files_exist - File not found: lib/WorkflowFetchngs.groovy
  • 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

❔ Tests ignored:

  • files_exist - File is ignored: conf/modules.config
  • files_exist - File is ignored: lib/NfcoreTemplate.groovy
  • files_exist - File is ignored: lib/Utils.groovy
  • files_exist - File is ignored: lib/WorkflowMain.groovy
  • files_exist - File is ignored: lib/nfcore_external_java_deps.jar
  • files_unchanged - File ignored due to lint config: assets/email_template.html
  • files_unchanged - File ignored due to lint config: assets/email_template.txt
  • files_unchanged - File ignored due to lint config: assets/sendmail_template.txt
  • files_unchanged - File does not exist: lib/nfcore_external_java_deps.jar
  • files_unchanged - File does not exist: lib/NfcoreTemplate.groovy
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore or pyproject.toml
  • actions_ci - actions_ci
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/fetchngs/fetchngs/.github/workflows/awstest.yml
  • multiqc_config - 'assets/multiqc_config.yml' not found

✅ Tests passed:

Run details

  • nf-core/tools version 2.11.1
  • Run at 2024-01-18 09:45:29

@drpatelh
Copy link
Member Author

@nf-core-bot fix linting

@drpatelh @robsyme
Update subworkflows/nf-core/utils_nfcore_pipeline/main.nf
ca88a5e 
Co-authored-by: Robert Syme <rob.syme@gmail.com>
adamrtalbot
adamrtalbot reviewed Jan 17, 2024
View reviewed changes
Copy link
Contributor

@adamrtalbot adamrtalbot left a comment

Choose a reason for hiding this comment

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

Some comments. I think the flow of the versions stuff is a bit clunky as it is.

subworkflows/local/utils_nfcore_fetchngs_pipeline/main.nf Outdated Show resolved Hide resolved
workflows/sra/main.nf Outdated Show resolved Hide resolved
subworkflows/nf-core/utils_nfcore_pipeline/main.nf
@@ -217,7 +264,7 @@ def completionEmail(summary_params, monochrome_logs = true) {
Map colors = logColours(monochrome_logs)
if (email_address) {
try {
if (params.plaintext_email) { throw GroovyException('Send plaintext e-mail, not HTML') }
if (plaintext_email) { throw GroovyException('Send plaintext e-mail, not HTML') }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is plaintext_email supported at all? The error here does not match the if statement.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is bonkers old email code from the email template and desperately needs refactoring. I actually don't know how we can test the email functionality in nf-core pipelines cos it requires a chunk of set-up...

@drpatelh
Copy link
Member Author

Tried to switch to using nf-validation to read in the samplesheet via fromSamplesheet but encountered this issue:
nextflow-io/nf-schema#10

@drpatelh
Copy link
Member Author

Done with this PR @adamrtalbot @maxulysse. Please take a look and lemme know if I've missed anything. We need to push these changes to nf-core/modules where required and reinstall in the pipeline but that can happen in a follow up PR.

@drpatelh drpatelh mentioned this pull request Jan 18, 2024
Merged
@maxulysse
Copy link
Member

cf nf-core/modules#4766 for the update of the utils subworkflows

@drpatelh drpatelh merged commit b9f8300 into nf-core:dev Jan 18, 2024
18 checks passed
@maxulysse maxulysse mentioned this pull request Jan 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamrtalbot adamrtalbot adamrtalbot left review comments

@robsyme robsyme robsyme left review comments

@maxulysse maxulysse maxulysse approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@drpatelh @maxulysse @robsyme @adamrtalbot @nf-core-bot