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

Bypass/mock ncbi datasets call in ci #46

Closed
corneliusroemer opened this issue Jul 17, 2024 · 1 comment · Fixed by #47
Closed

Bypass/mock ncbi datasets call in ci #46

corneliusroemer opened this issue Jul 17, 2024 · 1 comment · Fixed by #47

Comments

@corneliusroemer
Copy link
Member

corneliusroemer commented Jul 17, 2024

It's great we are running ingest as part of pathogen ci.

But we shouldn't rely on external services in our CI. Right now, with our setup, if NCBI datasets is down, CI fails, because ingest includes a call to their infrastructure.

I think we should not use ncbi datasets in CI, and instead use an archived zip file that is identical to what datasets would produce.

Based on a comment on PR #45:

In principle great to run the real ingest - but I worry a little about ci failures when NCBI datasets are down - which has happened quite a bit in the past, a couple of days a year.

We could have a flag in ingest that bypasses the call to ncbi-datasets and gets the output files from say s3 to not rely on others for our tests?

Originally posted by @corneliusroemer in #45 (review)

@tsibley
Copy link
Member

tsibley commented Jul 17, 2024

I think we should not use ncbi datasets in CI, and instead use an archived zip file that is identical to what datasets would produce.

The tradeoff is that we'll have to keep the example NCBI dataset file up-to-date with upstream changes, else we won't be testing what NCBI's actually providing and will drift over time.

But we shouldn't rely on external services in our CI.

Really? We rely on a ton of external services in CI, not just NCBI. I understand where you're coming from with not using external services, but I think it's unrealistic.

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 a pull request may close this issue.

2 participants