-
Notifications
You must be signed in to change notification settings - Fork 19
Add vendorization support #2584
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
Changes from 24 commits
51fd7f9
70c1afb
689faaa
d963767
bfa9b08
80c99ba
ca72a6d
5acd73e
ead592c
4e6c527
4ba8be9
e487003
4ccc663
ee161a2
72e56a5
9043b2e
eb7d9d6
8007762
421f756
064b5ad
fe5084e
1c82a67
1d46c87
fb97bbd
7fccf48
ffabf98
35b16fd
c8b3f10
386e705
f8c4dd2
62a641d
943fd62
173504d
0a59286
7616dca
54b996c
4ebd692
aad2338
78f180b
fa08c1e
ca9a214
67ca98a
155b8b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,23 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import importlib | ||
| from typing import TYPE_CHECKING | ||
| from uuid import uuid4 | ||
|
|
||
| from dandischema.conf import get_instance_config as get_schema_instance_config | ||
jjnesbitt marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| if TYPE_CHECKING: | ||
| import datetime | ||
|
|
||
| _SCHEMA_INSTANCE_CONFIG = get_schema_instance_config() | ||
jjnesbitt marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| class PublishableMetadataMixin: | ||
| @classmethod | ||
| def published_by(cls, now: datetime.datetime): | ||
| instance_name = _SCHEMA_INSTANCE_CONFIG.instance_name | ||
| instance_identifier = _SCHEMA_INSTANCE_CONFIG.instance_identifier | ||
|
|
||
| return { | ||
| 'id': uuid4().urn, | ||
| 'name': 'DANDI publish', | ||
|
|
@@ -20,10 +28,10 @@ def published_by(cls, now: datetime.datetime): | |
| 'wasAssociatedWith': [ | ||
| { | ||
| 'id': uuid4().urn, | ||
| 'identifier': 'RRID:SCR_017571', | ||
| 'name': 'DANDI API', | ||
| **({'identifier': instance_identifier} if instance_identifier else {}), | ||
jjnesbitt marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 'name': f'{instance_name} API', | ||
| # TODO: version the API | ||
| 'version': '0.1.0', | ||
| 'version': importlib.metadata.version('dandiapi'), | ||
|
||
| 'schemaKey': 'Software', | ||
| } | ||
| ], | ||
|
|
||
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.
@waxlamp Sorry for accidentally deleting your comment about this line you posted earlier.
Your question was regarding why other environment variables are not included to set
dandischema.conf.Config.There are five fields in
dandischema.conf.Configto set. TheDJANGO_DANDI_INSTANCE_NAMEenv var setsinstance_name. There is alreadyDJANGO_DANDI_WEB_APP_URLenv var that setsinstance_url.licensesdefaults to the value for DANDI Archive. The remaining fields to be set areinstance_identifieranddoi_prefix, and they can be set byDJANGO_DANDI_INSTANCE_IDENTIFIERandDJANGO_DANDI_DOI_API_PREFIXenv var respectively. However, the frond end code doesn't seem to require those values.If you want to input set the remaining field, you may want to pick from those in https://github.com/dandi/dandi-infrastructure/pull/224/files#diff-1a87a9189dbcd06318fab560eeb95c27d4c7dd16ef4574ffdf368cb2b63d7e30.
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.
Even though this is the frontend CI, it still runs the application, and we should be consistent in how the application is invoked.
I pushed a commit that makes the
DJANGO_DANDI_INSTANCE_IDENTIFIERenv var required in the Django app, and as such, also added that env var here.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.
@yarikoptic Is making
DJANGO_DANDI_INSTANCE_IDENTIFIERrequired an issue if one of your goals is to support people running DANDI instances in isolation without support for publishing?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.
IMHO it should not be required, as we would just default to the adhoc one and instance should just rely on what schema tells it to be.
But overall I think it is fine as long as the
infrastructurechanges are up to date (or beyond) with the server for which they are intended for. So infrastructure could/should just introduce them before (ie. now) even adopting a new version of the dandi-archive which requires it. And for DOI we have other more DOI-specific settings to enable or disable it.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.
There is no default for the instance identifier, the default you're referring to is for the instance name.
Regardless, the reason for requiring these values be specified is not for the sake of
dandi-schema, it's for the sake of operating the archive. We rely on those values in various parts of the code base, and to make those values optional would introduce buggy and uncertain behavior in multiple spots.They need not be set to "real world" values, they can be set to dummy values, as is done in the development setup. Any downstream DANDI instances can change these value to whatever they like, or make them optional by modifying the settings files (and accepting the consequences).