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

common.smk: Conditionally include --no-sign-request option #49

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joverlee521
Copy link
Contributor

@joverlee521 joverlee521 commented Jun 4, 2024

Description of proposed changes

Conditionally include --no-sign-request option for download rules if the s3_src config param is pointing to the public Nextstrain S3 bucket (s3://nextstrain-data/).

This means AWS credentials are no longer necessary for the public h5n1-cattle-outbreak genome build.

After running nextstrain update docker, I can successfully run the h5n1-cattle-outbreak build with:

nextstrain build . \
    --snakefile Snakefile.genome \
    --config s3_src=s3://nextstrain-data/files/workflows/avian-flu/h5n1

Or you can point to the specific image

nextstrain build --image nextstrain/base:build-20240612T230750Z . \
    --snakefile Snakefile.genome \
    --config s3_src=s3://nextstrain-data/files/workflows/avian-flu/h5n1

Related issue(s)

Dependent on nextstrain/docker-base#215
Follow up to #47

Checklist

  • Checks pass

@joverlee521

This comment was marked as outdated.

@joverlee521
Copy link
Contributor Author

From @jameshadfield in nextstrain/docker-base#213 (comment):

(Or we could just change avian-flu to use curl, right?)

Yes, as long as we are fine with maintaining separate download rules for the default avian flu builds and the h5n1 cattle outbreak build.

@trvrb
Copy link
Member

trvrb commented Jun 5, 2024

I think this needs to work from Docker and not just Conda. I don't have a preference for curl vs aws s3 cp, but curl seems like a potentially easy fix.

@joverlee521
Copy link
Contributor Author

joverlee521 commented Jun 6, 2024

Docker issue will be resolved by nextstrain/docker-base#214 nextstrain/docker-base#215

Conditionally include `--no-sign-request` option for download rules
if the `s3_src` config param is pointing to the public Nextstrain S3
bucket (s3://nextstrain-data/).

This means AWS credentials are no longer necessary for the public
h5n1-cattle-outbreak genome build.
@joverlee521 joverlee521 marked this pull request as ready for review June 13, 2024 01:07
Returns the `--no-sign-request` option for AWS CLI if
the requested `S3_SRC` poins to the NEXTSTRAIN_PUBLIC_BUCKET.
"""
NEXTSTRAIN_PUBLIC_BUCKET = "s3://nextstrain-data/"
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 would be more robust to call aws s3api get-bucket-policy-status to check if the bucket is public or not rather than relying on a hard-coded bucket here.

I'm going to punt on that and say we can implement it in the ingest/download-from-s3 script and update this workflow to call download-from-s3 later.

@joverlee521 joverlee521 requested a review from a team June 13, 2024 01:19
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.

None yet

2 participants