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

Move s3_dst config to top level config #215

Closed
wants to merge 1 commit into from
Closed

Conversation

j23414
Copy link
Contributor

@j23414 j23414 commented Oct 27, 2023

Description of proposed changes

Nested configs for s3 URLs are challenging to override by the Snakemake --config option, as discussed in the following comment:

nextstrain/dengue#13 (comment)

This moves the url to the top level as a s3_dst config value, and propagate those changes to the relevant Snakefiles.
It's important to note that I haven't conducted thorough testing, and there may be nuances in the implementation meant by the comment. Suggestions or clarification welcome.

Additionally, I was unsure regarding whether the s3_dst config should be a required value or an optional default (Snakemake styleguide: config values).

Related issue(s)

Checklist

  • Checks pass

Nested configs for s3 URLs are challenging to override by the Snakemake
`--config` option, as discussed in the following comment:

nextstrain/dengue#13 (comment)

This moves the url to the top level, and propagate those changes to the relevant Snakefiles.
@j23414 j23414 requested review from joverlee521, corneliusroemer and a team October 27, 2023 17:15
@j23414 j23414 changed the title Make s3_dst config to top level config Move s3_dst config to top level config Oct 27, 2023
@j23414
Copy link
Contributor Author

j23414 commented Oct 27, 2023

Since we can use: --config upload='{"s3": {"dst": "NEW URL"}}'

nextstrain/dengue#13 (comment)

This PR is no longer necessary.

@j23414 j23414 closed this Oct 27, 2023
@victorlin victorlin deleted the top_level_s3_dst branch October 30, 2023 17:53
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.

1 participant