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 IRMA to 1.1.2 #782

Merged
merged 12 commits into from
Nov 9, 2023
Merged

Update IRMA to 1.1.2 #782

merged 12 commits into from
Nov 9, 2023

Conversation

jwarnn
Copy link
Contributor

@jwarnn jwarnn commented Nov 3, 2023

Hello,
IRMA just recently had an update. In the update they fixed issue with SRR reads so those comments have been removed from the README and associated code removed from the test layer. The test layer runs both the test_run.sh that is included with IRMA and some test fastq files from an online database. As part of the update CDC also posted an image of IRMA 1.1.2 on their dockerhub; is this an issue having competing images?

Pull Request (PR) checklist:

  • Include a description of what is in this pull request in this message.
  • The dockerfile successfully builds to a test target for the user creating the PR. (i.e. docker build --tag samtools:1.15test --target test docker-builds/samtools/1.15 )
  • Directory structure as name of the tool in lower case with special characters removed with a subdirectory of the version number (i.e. spades/3.12.0/Dockerfile)
    • (optional) All test files are located in same directory as the Dockerfile (i.e. shigatyper/2.0.1/test.sh)
  • Create a simple container-specific README.md in the same directory as the Dockerfile (i.e. spades/3.12.0/README.md)
    • If this README is longer than 30 lines, there is an explanation as to why more detail was needed
  • Dockerfile includes the recommended LABELS
  • Main README.md has been updated to include the tool and/or version of the dockerfile(s) in this PR
  • Program_Licenses.md contains the tool(s) used in this PR and has been updated for any missing

@kapsakcj
Copy link
Collaborator

kapsakcj commented Nov 3, 2023

Thanks for the PR. I can review sometime next week unless someone beats me to it

is this an issue having competing images?

No, it should not be as the code is open-source and licensed as such (IIRC).

It would help to note in the irma/1.1.2/README.md that this docker image is not maintained by CDC & the IRMA developers, so if users encounter issues with this docker image specifically they should file an issue on this github repo and do not contact CDC for support. We can't expect CDC to provide support for this "3rd party" docker image

@jwarnn
Copy link
Contributor Author

jwarnn commented Nov 3, 2023

Added note to the README about the CDC image and who to contact for support.

@erinyoung
Copy link
Contributor

This looks great! Can you add this version of IRMA to the main readme, too?

@jwarnn
Copy link
Contributor Author

jwarnn commented Nov 7, 2023

Added version to README

@jwarnn
Copy link
Contributor Author

jwarnn commented Nov 8, 2023

This was on the StaPH-B slack:

"@Curtis (Theiagen)
There was a bug/regression in v1.1.2 that I have fixed. Recommend using v1.1.3 that was released today. See: Dockerhub and CDC Wonder (edited)"

Should I just update to v1.1.3 and forget about v1.1.2?

@erinyoung
Copy link
Contributor

@jwarnn , adjusting your current dockerfile is fine, or you can add a new Dockerfile for the new version (both would be sent to Dockerhub/quay, but only 1.1.3 would get the 'latest' tag).

@kapsakcj
Copy link
Collaborator

kapsakcj commented Nov 8, 2023

Yes, I would advocate for skipping 1.1.2 and going right to 1.1.3

@jwarnn
Copy link
Contributor Author

jwarnn commented Nov 8, 2023

I added a 1.1.3 dockerfile to go along with what I had already added.

RUN cd /flu-amd/tests && \
./test_run.sh

RUN wget ftp://ftp.sra.ebi.ac.uk/vol1/fastq/SRR179/072/SRR17940172/SRR17940172_1.fastq.gz && gzip -d SRR17940172_1.fastq.gz && \
Copy link
Contributor

Choose a reason for hiding this comment

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

According to https://wonder.cdc.gov/amd/flu/irma/run.html, irma can now be run on fastq.gz files. What happens if you remove the gzip -d?

Copy link
Contributor

@erinyoung erinyoung left a comment

Choose a reason for hiding this comment

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

I approve this PR.

What do you think about changing the tests to remove the decompression step?

Changes that I made:

  • added hyperlink to the main README.md
  • changed the name of the irma 1.1.3 README.md file to README.md
  • formatting in the 1.1.2 README.md

@jwarnn
Copy link
Contributor Author

jwarnn commented Nov 8, 2023

It ran fine. I could also take out for the apt-get step git and gzip.

@erinyoung
Copy link
Contributor

Thanks!

I'm going to

  1. merge this PR
  2. push to dockerhub and quay for both Dockerfiles using 1.1.2 and 1.1.3
  3. overwrite the 'latest' tag for 1.1.3

@erinyoung erinyoung merged commit 5852397 into StaPH-B:master Nov 9, 2023
3 checks passed
@erinyoung
Copy link
Contributor

Thank you for your contribution!

You can check the status of your deploy at https://github.com/StaPH-B/docker-builds/actions/runs/6814733115 and https://github.com/StaPH-B/docker-builds/actions/runs/6814737800

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants