-
Notifications
You must be signed in to change notification settings - Fork 42
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
Update models, endpoints and responses to 1.0.0 #380
Conversation
a59292a
to
00da5df
Compare
Codecov Report
@@ Coverage Diff @@
## master #380 +/- ##
==========================================
- Coverage 90.27% 90.22% -0.06%
==========================================
Files 54 55 +1
Lines 2386 2404 +18
==========================================
+ Hits 2154 2169 +15
- Misses 232 235 +3
Continue to review full report at Codecov.
|
0dd0557
to
b872fe4
Compare
0d5af29
to
9991937
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.
Should versions also be included in the index meta-database?
Why isn't Edit: I found your "thought" on this in the OP :) |
I think we've now ironed out all differences between schemas and the specification, except the way we specify the content type of the versions endpoint, which I'll have a go at later. |
We also need to change a lot of our test data so that the tests pass, e.g. |
7e46503
to
24b4f9c
Compare
Pending these test failures, I think that's everything done, except changing the content of |
- This URL is now used when generating the OpenAPI schemas
- This commit removes the `index_base_url` from the models and updates all tests, routers and config files accordingly. - `index_base_url` is now a top-level config option.
f0fb738
to
9b293c9
Compare
I've just spent a while meticulously thematically splitting this PR into just a few commits, each of which should pass tests individually. If you're reviewing, it might make sense to break it down per commit. |
/versions
endpointThere 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.
We should add homepage
to the implementation
configurations as well, I think?
All in all, pretty great work @ml-evs !
@@ -73,13 +73,16 @@ class ServerConfig(BaseSettings): | |||
), | |||
description="Introspective information about this OPTIMADE implementation", | |||
) | |||
index_base_url: Optional[AnyHttpUrl] = Field( |
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.
Why do we have this now?
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.
We can remove it, sure, it's only (optionally) used in the landing page now
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.
Right. Is there a way for us to automagically retrieve it from the links
endpoint data JSON file? Then there is only one source of truth and we're not mixing concepts. Especially, when it's using a recently outdated valid field name.
4a694dc
to
83176d4
Compare
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
83176d4
to
65e5136
Compare
Closes #379. This PR brings our models, responses and endpoints up to date with newly released 1.0.0 of the specification.
I've split this up into 4 commits, which should each individually pass the tests:
vMajor
router addition to a separate function. This means the generated schemas no longer contain version numbers, as desired./versions
endpoint from the unversioned base URL for both index and regular db's. This should now have the correct schema response (matching the released version).index_base_url
field, as it has been from the schema. I've moved the original config option we had for this to the top-level, and had to adjust our tests accordingly.meta
now being mandatory, but several sub-fields becoming optional,Union[jsonapi.Links, AnyUrl]
).version
/api_version
numbers