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.

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

Update dragmap + add nf-test #4379

Merged
merged 34 commits into from
Dec 7, 2023
Merged

Update dragmap + add nf-test #4379

merged 34 commits into from
Dec 7, 2023

Conversation

nvnieuwk
Copy link
Contributor

PR checklist

Closes #XXX

  • 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 module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Emit the versions.yml file.
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • PROFILE=docker pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
    • PROFILE=singularity pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
    • PROFILE=conda pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware

@nvnieuwk nvnieuwk requested review from edmundmiller and a team as code owners November 22, 2023 11:57
# renovate: datasource=conda depName=bioconda/samtools
- samtools=1.15.1
- samtools=1.16
Copy link
Member

Choose a reason for hiding this comment

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

not 1.17?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's 1.16 in the mulled container, so I'd rather keep them the same

Copy link
Contributor

Choose a reason for hiding this comment

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

in that case maybe update the mulled container as well? but in any case 1.3.0 has a bunch of issues to my knowledge Illumina/DRAGMAP#47

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I also have the 1.3.0 bug, so I don't know if we can move forward.

Illumina/DRAGMAP@c4cb6c9

It looks like there's a 1.3.1 but they didn't tag it properly, so bioconda didn't create a release.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's GPL3, do we fork it? 😆 There's a lot of improvement PRs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good for me :p (And I know the tests are failing, I still need to do a small fix)

Copy link
Contributor

@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.

Remove the pytest-workflow tests.

Let's just merge it, I won't update it in Nascent 🙃

I forked it & and cut a 1.3.1. Might merge a few of the PRs into it, and add my fork to bioconda. Or maybe I'll get distracted and forget about it, who knows? https://github.com/Emiller88/NARFMAP/releases/tag/1.3.1

@nvnieuwk
Copy link
Contributor Author

nvnieuwk commented Dec 7, 2023

I'd rather wait for your updates then so we're sure it works as intended 😁

@edmundmiller
Copy link
Contributor

I think that'll be a whole new module. I think this is as far as dragmap is gonna get, they never tagged the 1.3.1 release. So we might as well merge this?

@nvnieuwk
Copy link
Contributor Author

nvnieuwk commented Dec 7, 2023

Fair enough 🤷

@nvnieuwk nvnieuwk enabled auto-merge December 7, 2023 14:55
@nvnieuwk nvnieuwk added this pull request to the merge queue Dec 7, 2023
Merged via the queue into nf-core:master with commit 8f0bec8 Dec 7, 2023
15 checks passed
@nvnieuwk nvnieuwk deleted the update-dragmap branch December 7, 2023 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants