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

Allow location fields to have localhost URLs #33

Merged
merged 3 commits into from
Jun 10, 2021
Merged

Conversation

dchiquito
Copy link
Contributor

Development/test environments use an object store that is generally
served at localhost:9000. URLs that point to objects in this store
obviously have the form http://localhost:9000/{...}. The pydantic
field HttpUrl requires a TLD, which excludes localhost. Instead, we
should use AnyHttpUrl which allows localhost.

Development/test environments use an object store that is generally
served at `localhost:9000`. URLs that point to objects in this store
obviously have the form `http://localhost:9000/{...}`. The pydantic
field `HttpUrl` requires a TLD, which excludes `localhost`. Instead, we
should use `AnyHttpUrl` which allows `localhost`.
@dchiquito dchiquito requested a review from satra June 10, 2021 15:52
@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #33 (f85a302) into master (a3d81db) will decrease coverage by 0.09%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
- Coverage   94.74%   94.65%   -0.10%     
==========================================
  Files          11       11              
  Lines         876      879       +3     
==========================================
+ Hits          830      832       +2     
- Misses         46       47       +1     
Flag Coverage Δ
unittests 94.65% <75.00%> (-0.10%) ⬇️

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

Impacted Files Coverage Δ
dandischema/models.py 91.68% <75.00%> (-0.20%) ⬇️

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 a3d81db...f85a302. Read the comment docs.

@satra
Copy link
Member

satra commented Jun 10, 2021

@dchiquito - so this seems like a dev only hack rather than what should be in the schema, right? so perhaps we can use an environment variable and make HttpUrl map to AnyHttpUrl, as the official validation should not allow a non TLD ?

@dchiquito
Copy link
Contributor Author

My thought was that all of these fields are calculated by the API server, so it would be acceptable for these fields to accept any possible calculated value from any possible deployment. Providing a separate schema that is only used for dev/testing deployments sounds very difficult and much more complicated.

@satra
Copy link
Member

satra commented Jun 10, 2021

all this would take is something like this:

if os.environ["DEV_ENV"]:
    HttpUrl = AnyHttpUrl

this is only a pydantic dataclass issue as in the json schema there is no distinction of the URI types.

@dchiquito
Copy link
Contributor Author

Changing the result of importing a module based on environment variables sounds to me like it would cause a lot of headaches down the line, but if that is your preferred solution I can do that.

@satra
Copy link
Member

satra commented Jun 10, 2021

@dchiquito - would you like a release to go alongside this? in which case please label this with release, patch.

@yarikoptic yarikoptic added the patch Increment the patch version when merged label Jun 10, 2021
@dchiquito dchiquito added the release Create a release when this pr is merged label Jun 10, 2021
dchiquito added a commit to dandi/dandi-archive that referenced this pull request Jun 10, 2021
The default dandischema pydantic model does not allow localhost in URLs,
which is a requirement for test and dev environments. To get around
this, the dandischema module will subtly tweak the model to be more
permissive with URLs if the environment variable
DANDI_ALLOW_LOCALHOST_URLS is set.

See dandi/dandi-schema#33
@satra satra merged commit e898d99 into master Jun 10, 2021
@satra satra deleted the any-manifest-location branch June 10, 2021 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch 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.

3 participants