-
Notifications
You must be signed in to change notification settings - Fork 28
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
Include LINC API as a supported instance type for DANDI CLI #1527
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1527 +/- ##
==========================================
- Coverage 88.57% 87.65% -0.92%
==========================================
Files 78 78
Lines 10589 10591 +2
==========================================
- Hits 9379 9284 -95
- Misses 1210 1307 +97
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @aaronkanzer. This looks good to me. I tested these changes using your instructions with https://lincbrain.org/dandiset/000044/draft. I can verify that I was only able to download and upload when using the correct lincbrain.org API key.
Let's go ahead and include updates to the docs in this pull request. See notes here: #1517 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately we have some "manually" curated dumps of --help for commands and thus need to extend with this new instances the docs/source/cmdline/instances.rst file. Please add listing there too
Updated! |
@yarikoptic @jwodder -- did this not get fully released to PyPI? Just noticing that there is no new patch version at https://pypi.org/project/dandi/#history |
@aaronkanzer The PR didn't have the "release" label, so no release was triggered. |
@jwodder -- would the preferred route to release then be that I push a trivial change with those labels then? |
merged with |
🚀 PR was released in |
Relates to #1517
Follow-up of #1519 -- used a different fork since tags were not copied over in the prior fork in #1519
Cc @kabilar
Steps to properly test:
pip uninstall dandi -y
to remove DANDI CLI locally (can use avenv
too if desired)pip install -e git+https://github.com/lincbrain/dandi-cli.git@ak-linc#egg=dandi
-- reference my forked branch here as thedandi
CLI versiondandi download https://lincbrain.org/dandiset/000045/draft
-- the CLI should prompt for an API value here000045
folderdandi upload -i linc
Prior to running any steps, I ran
dandi instances
to see that the proper values were listed@jwodder @yarikoptic -- mind a brief re-review here?
I think we should be fine here -- it seems that sometimes the CI tests for Ubuntu can be flaky -- you'd know better than I if this is a common issue over time -- I know sometimes GitHub runners can be unpredictable with longer, parallel jobs