-
Notifications
You must be signed in to change notification settings - Fork 12
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
Internal publishing service #1363
Conversation
ced1433
to
7f79198
Compare
whoops I forgot to check if tests were passing before opening this... |
7f79198
to
b830d85
Compare
a77bb8c
to
24f9758
Compare
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.
One comment but overall looks good. The structure does bug me a bit (bouncing between the publish service and tasks files), but I'm not sure of a good way to address that.
dandiapi/api/tests/test_version.py
Outdated
@@ -654,8 +654,8 @@ def test_version_rest_publish_zarr( | |||
f'/api/dandisets/{draft_version.dandiset.identifier}' | |||
f'/versions/{draft_version.version}/publish/' | |||
) | |||
assert resp.status_code == 400 | |||
assert resp.json() == ['Cannot publish dandisets which contain zarrs'] | |||
assert resp.status_code == 403 |
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.
Just a reminder to back out this one change we discussed.
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.
👍
2a753df
to
4e2c10c
Compare
Yeah I'm still not completely happy with how that turned out :( |
ac75bf5
to
16589bf
Compare
🚀 PR was released in |
Refactors publishing into a service layer module.