search

nextstrain / pathogen-repo-guide

3 stars 1 forks source link

Reconsider including `vendored` scripts in ingest template #12

Open joverlee521 opened 11 months ago

joverlee521 commented 11 months ago

This didn't occur to me until I was reviewing the https://github.com/nextstrain/dengue/pull/13 and saw the vendored scripts were pulled in separately. This is nice because then the vendored/.gitrepo file is set up correctly from the start. The seasonal-cov repo was copied from this template, so the vendored/.gitrepo file points to a parent commit in this repo.

This means when you try to pull in the latest vendored scripts from within seasonal-cov, you run into an error: 

$ cd seasonal-cov
$ git subrepo pull ingest/vendored
git-subrepo: Command failed: 'git rev-list --reverse --ancestry-path --topo-order 143581871f1fdcc3ce0d020e1c3ddddffef2ffa9..HEAD'.

I had two main reasons for including the vendored scripts in the template.

  1. I didn't want to require users to set up the subrepo
  2. I wanted to make sure that the use of the scripts within the template matched the version of the vendored scripts included.

If we keep the vendored scripts in the template, we just need to document the workaround for the error. Technically the workaround is already documented in the vendored README, so we just need to point those docs for troubleshooting.

On the other hand, if we don't include the vendored scripts in the template, then we would need additional steps for setting up the ingest workflow:

  1. User needs to install git-subrepo
  2. User needs to set up the subrepo from a specific commit hash with: 
    git subrepo clone https://github.com/nextstrain/ingest ingest/vendored -b <commit_hash>

If they ever want to update the subrepo to the latest version in the future, they would also need to specify the branch in the pull:

git subrepo pull ingest/vendored -b main
joverlee521 commented 11 months ago

I've convinced myself that we should keep the vendored scripts in the template after writing everything out 😄

joverlee521 commented 7 months ago

Reopening this to remove the vendored scripts from ingest once we move all the potential augur curate scripts to official Augur commands. This will reduce burden on external users to install and manage the subrepo.

Prompted by discussion around the future ingest tutorials.