-
Notifications
You must be signed in to change notification settings - Fork 8
Set environment vars to set vendor-specific dandi-schema used by the DANDI instance #224
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
Conversation
39067a9 to
9aa4e07
Compare
|
@aaronkanzer @kabilar Do you know where, which repo, I can modify to apply the same changes to LINC? |
9aa4e07 to
80c4bb6
Compare
|
@waxlamp please review this PR from your side of infra -- it should not effect running instance but would allow for vendorization on which there is work ongoing in dandi/dandi-schema#294 and from there on. |
Thanks @candleindark. Here is the LINC terraform repo: https://github.com/lincbrain/linc-archive-infrastructure |
80c4bb6 to
9689f9c
Compare
9689f9c to
45c02e1
Compare
terraform/api.tf
Outdated
|
|
||
| additional_django_vars = { | ||
| DANDI_INSTANCE_NAME = "DANDI" | ||
| DANDI_DOI_PREFIX = "10.48324" |
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.
A couple of questions.
- What is the intended use of these vars? They are specified in the
additional_django_varssection, but lack theDJANGO_*prefix on them. - We already have a env var
DJANGO_DANDI_DOI_API_PREFIX, which specifies the same value asDANDI_DOI_PREFIX. So is this additional redundant?
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.
Ah, after looking into it further it seems they are passed to the schema. I do wonder if there's a better place to set these, but I imagine not.
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.
- What is the intended use of these vars? They are specified in the
additional_django_varssection, but lack theDJANGO_*prefix on them.
Just as you noted, they are used to set the configuration in dandischema, https://github.com/dandi/dandi-schema/blob/f2ecf9d9b5cb6cff8ef0a3a1de427044cfb3e59d/dandischema/conf.py#L21-L63. My understanding is that variables defined in additional_django_vars don't have to start with the DJANGO_*. The set configuration in dandischema is be shared among dandi-cli and dandi-archive in order to "vendorize" DANDI, dandi/dandi-schema#294.
- We already have a env var
DJANGO_DANDI_DOI_API_PREFIX, which specifies the same value asDANDI_DOI_PREFIX. So is this additional redundant?
Yes, it is redundant, but since the shared config is not just used in dandi-archive having an env variable starting DJANGO_* may not be appropriate. It may be a good idea to eliminate DJANGO_DANDI_DOI_API_PREFIX in the future though.
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.
Ah, after looking into it further it seems they are passed to the schema. I do wonder if there's a better place to set these, but I imagine not.
The configuration must be set correctly before dandischema.models is imported. Providing those env vars will achieve that in dandi-archive.
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.
The configuration must be set correctly before dandischema.models is imported. Providing those env vars will achieve that in dandi-archive.
Can we not use set_instance_config, instead of the env vars?
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.
@jjnesbitt manually how so that it is in effect at dandischema library import?
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.
Manually via set_instance_config, instead of with environment variables.
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.
Manually via
set_instance_config, instead of with environment variables.
If calling of set_instance_config() is necessitated because of naming of variables. I can introduce aliases to the fields of dandischema.conf.Config so that they can be set via DJANGO_DANDI_DOI_API_PREFIX and DJANGO_DANDI_INSTANCE_NAME.
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.
ok, let's finalize/merge
and then establish needed changes in dandi-archive for .set_instance_config . @jjnesbitt would you do that or should we try a PR?
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.
I can make that PR.
|
@jjnesbitt @waxlamp please chime in on either you want anything else changed. It should prepare our instance for use of vendorized instance of dandischema |
|
Note |
45c02e1 to
7c64641
Compare
7c64641 to
a287e35
Compare
|
@jjnesbitt This PR has been updated and is ready for review. All environment variables used now conform to the |
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.
Pull Request Overview
This PR adds vendor-specific configuration for DANDI instances by introducing environment variables for instance name and identifier. The changes support vendorization of the publisher for DataCite integration and provide runtime configuration for different deployment environments.
Key changes:
- Added DJANGO_DANDI_INSTANCE_NAME environment variable for both staging and production
- Added DJANGO_DANDI_INSTANCE_IDENTIFIER environment variable with RRID identifier
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| terraform/staging_pipeline.tf | Adds instance name "DANDI-STAGING" and RRID identifier for staging environment |
| terraform/api.tf | Adds instance name "DANDI" and RRID identifier for production environment |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
a287e35 to
da7e21b
Compare
da7e21b to
3c1a179
Compare
This change is to match the changes in PR dandi#224
See commit message for details.
This PR closes #223.
TODOs (more like blocks)
dandi-schema vendorization getting "full runtime config" Support dynamic configuration dandi-schema#316to_datacite, Add vendorization of the Publisher for to_datacite dandi-schema#303