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

Support /asset/<asset id>/download/ URLs #748

Merged
merged 1 commit into from
Aug 4, 2021
Merged

Support /asset/<asset id>/download/ URLs #748

merged 1 commit into from
Aug 4, 2021

Conversation

jwodder
Copy link
Member

@jwodder jwodder commented Aug 2, 2021

Closes #723.

Note that the dandi-api Docker images are currently not building for some reason, and the latest image lacks support for the /asset/<asset id>/ endpoint. Hence, the tests will fail under the Docker build is fixed.

@jwodder jwodder added the minor Increment the minor version when merged label Aug 2, 2021
@codecov
Copy link

codecov bot commented Aug 2, 2021

Codecov Report

Merging #748 (dfec671) into master (64ac1ef) will decrease coverage by 0.02%.
The diff coverage is 86.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #748      +/-   ##
==========================================
- Coverage   84.85%   84.82%   -0.03%     
==========================================
  Files          59       59              
  Lines        5975     6003      +28     
==========================================
+ Hits         5070     5092      +22     
- Misses        905      911       +6     
Flag Coverage Δ
unittests 84.82% <86.15%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
dandi/tests/test_dandiarchive.py 100.00% <ø> (ø)
dandi/dandiarchive.py 83.58% <76.66%> (-2.05%) ⬇️
dandi/dandiapi.py 93.22% <93.33%> (+0.12%) ⬆️
dandi/tests/test_download.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64ac1ef...dfec671. Read the comment docs.

@yarikoptic
Copy link
Member

Note that the dandi-api Docker images are currently not building for some reason, and the latest image lacks support for the /asset/<asset id>/ endpoint. Hence, the tests will fail under the Docker build is fixed.

@satra will enable paid account to manage the builds in the future. Meanwhile I could not even see exactly which Dockerfile was used to build those images!

with a little investigation
(git)lena:~/proj/dandi/dandi-api[master]dev
$> diff -Naur django.Dockerfile django-public.Dockerfile
--- django.Dockerfile	2021-04-27 15:55:47.888520613 -0400
+++ django-public.Dockerfile	2021-03-05 16:18:19.618554500 -0500
@@ -3,22 +3,11 @@
 # * psycopg2
 RUN apt-get update && \
     apt-get install --no-install-recommends --yes \
-    libpq-dev gcc libc6-dev && \
+        libpq-dev gcc libc6-dev && \
     rm -rf /var/lib/apt/lists/*
 
-ENV PYTHONDONTWRITEBYTECODE 1
 ENV PYTHONUNBUFFERED 1
 
-# Copy versioneer.py so it can be accessed by setup.py
-COPY ./versioneer.py /opt/django-project/versioneer.py
-
-# Only copy the setup.py and setup.cfg, it will still force all install_requires to be installed,
-# but find_packages() will find nothing (which is fine). When Docker Compose mounts the real source
-# over top of this directory, the .egg-link in site-packages resolves to the mounted directory
-# and all package modules are importable.
-COPY ./setup.cfg /opt/django-project/setup.cfg
-COPY ./setup.py /opt/django-project/setup.py
-RUN pip install --editable /opt/django-project[dev]
-
-# Use a directory name which will never be an import name, as isort considers this as first-party.
-WORKDIR /opt/django-project
+WORKDIR /opt/django
+COPY . /opt/django/
+RUN pip install -e .[dev]
(git)lena:~/proj/dandi/dandi-api[master]dev
$> docker run -it --rm --entrypoint bash dandiarchive/dandiarchive-api
root@3d9c3a9356a1:/opt/django# export | grep PYTHONDONTWRITEBYTECODE
root@3d9c3a9356a1:/opt/django# ls /opt/django-project/versioneer.py
ls: cannot access '/opt/django-project/versioneer.py': No such file or directory
root@3d9c3a9356a1:/opt/django# ls /opt/django/
DEVELOPMENT.md	README.md    dandiapi.egg-info	docker-compose.override.yml  mypy.ini	       runtime.txt  tox.ini
MANIFEST.in	__pycache__  dev		docker-compose.yml	     pyproject.toml    setup.cfg    versioneer.py
Procfile	dandiapi     doc		manage.py		     requirements.txt  setup.py
root@3d9c3a9356a1:/opt/django# ls -ld /opt/django*
drwxr-xr-x 1 root root 4096 Jul 23 16:24 /opt/django

I would assume that it was the https://github.com/dandi/dandi-api/blob/master/dev/django-public.Dockerfile , will build/compare (for paranoia) and push manually for now.

@yarikoptic
Copy link
Member

@jwodder fresh dandiarchive-api image built! please retrigger tests etc

@jwodder
Copy link
Member Author

jwodder commented Aug 4, 2021

@yarikoptic Tests rerun successfully.

@yarikoptic yarikoptic added the release Create a release when this pr is merged label Aug 4, 2021
@yarikoptic
Copy link
Member

Great. Thank you @jwodder. Since it fixes the download issue , let's also release, added the label.

@yarikoptic yarikoptic merged commit 0479cce into master Aug 4, 2021
@yarikoptic yarikoptic deleted the gh-723 branch August 4, 2021 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged release Create a release when this pr is merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support downloading from top level /assets endpoint
2 participants