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

Template update 3.0.1 #472

Merged
merged 10 commits into from
Oct 11, 2024
Merged

Template update 3.0.1 #472

merged 10 commits into from
Oct 11, 2024

Conversation

nschcolnicov
Copy link
Contributor

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!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/smrnaseq branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nf-test test main.nf.test -profile test,docker).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@nschcolnicov nschcolnicov changed the base branch from master to dev October 8, 2024 20:20
@nschcolnicov nschcolnicov mentioned this pull request Oct 8, 2024
Closed
@nschcolnicov nschcolnicov changed the title Nf core/tools/3.0.0 Template update 3.0.1 Oct 9, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 9, 2024 •
edited
Loading

nf-core pipelines lint overall result: Passed ✅ ⚠️

Posted for pipeline commit debb5a7

+| ✅ 242 tests passed       |+
#| ❔   1 tests were ignored |#
!| ❗   3 tests had warnings |!

❗ Test warnings:

❔ Tests ignored:

  • nextflow_config - Config default ignored: params.fastp_known_mirna_adapters

✅ Tests passed:

Run details

  • nf-core/tools version 3.0.1
  • Run at 2024-10-11 10:46:24

@nschcolnicov
Copy link
Contributor Author

nschcolnicov commented Oct 9, 2024 •
edited
Loading

Besides pulling from the template, I also added a new config profile for setting the resources in CI tests, since I had to remove the max resources params and the checkmax function with the new template
I also had to update the .stat file snaps because after having to update the resource definitions for the pipeline, it affects the alignment results.

@nf-core nf-core deleted a comment from github-actions bot Oct 9, 2024
@nschcolnicov
Copy link
Contributor Author

nf-core linting seems to be failing due to an issue with the new template that is not picking up the igenomes_base value from the ignore params array. I have already reported this issue, I'll update the PR once this is fixed

@nschcolnicov nschcolnicov mentioned this pull request Oct 9, 2024
Closed
@apeltzer
Copy link
Member

Hm, not sure I find the issue on tools @nschcolnicov which ticket did you create?

@nschcolnicov
Copy link
Contributor Author

Hi @apeltzer I didn't, I reached out on slack https://nfcore.slack.com/archives/CE6PELWR4/p1728478362119159

@atrigila atrigila marked this pull request as ready for review October 10, 2024 16:10
@atrigila atrigila requested a review from apeltzer October 10, 2024 17:45
@nschcolnicov
Copy link
Contributor Author

nschcolnicov commented Oct 10, 2024 •
edited
Loading

The issue has already been fixed in dev, and will be released soon: https://nfcore.slack.com/archives/CE6PELWR4/p1728478362119159
On the meantime @atrigila fixed this issue temporarily by adding the ignore params to the nextflow_schema in case we want to merge this ASAP

@nschcolnicov nschcolnicov requested a review from atrigila October 10, 2024 18:58
@nschcolnicov
Copy link
Contributor Author

@atrigila We will be merging this, while we wait for nf-core to merge the fix for the ignore params issue, for now we added the igenomes_base to the schema instead of ignoring it, we can simply remove it once its fixed

@atrigila atrigila requested a review from jfy133 October 10, 2024 19:04
@charles-plessy
Copy link
Contributor

Sorry for the direct commit; I thought that it would materialize as a simple suggestion. I will check the GitHub docs in more details…

charles-plessy
charles-plessy reviewed Oct 10, 2024
View reviewed changes
Copy link
Contributor

@charles-plessy charles-plessy 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; I only had minor comments.

conf/base.config Outdated Show resolved Hide resolved
conf/test_contamination.config Show resolved Hide resolved
@apeltzer apeltzer merged commit b684cc8 into dev Oct 11, 2024
6 checks passed
@apeltzer apeltzer deleted the nf-core/tools/3.0.0 branch October 11, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LouisLeNezet LouisLeNezet LouisLeNezet left review comments

@charles-plessy charles-plessy charles-plessy left review comments

@apeltzer apeltzer apeltzer approved these changes

@atrigila atrigila Awaiting requested review from atrigila

@jfy133 jfy133 Awaiting requested review from jfy133

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
@nschcolnicov @apeltzer @charles-plessy @LouisLeNezet @atrigila