-
Notifications
You must be signed in to change notification settings - Fork 1
Add option for custom openapi metadata and dataset template file #277
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
base: main
Are you sure you want to change the base?
Conversation
…-openapi-metadata
| os.getenv("OPENAPI_METADATA_PATH", "/app/openapi_default_files/openapi_metadata.json"), | ||
| "r", | ||
| ) as file: | ||
| openapi_metadata = json.load(file) |
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.
It worries me a bit that you can now add arbitrary arguments to the FastAPI constructor...
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 do not unpack the entire json, and the response model will stop you from adding anything other then strings. If we do unpack the json somwhere that would indeed open some more concerns.
I can't see any other issue then this at the moment.
| "title": "EUMETNET Members", | ||
| "type": "text/html" | ||
| "type": "text/html", | ||
| "href": "https://www.eumetnet.eu/about-us" |
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.
Contact info now points towards Met Norway, but the "about" info doesn't.
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.
@StuartJMatthews @vegark73 Is this intended?
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.
@Teddy-1000 @lukas-phaf. Yes. We mention EUMETNET Members in the description, but don't list them anywhere. The list of Members is reasonably stable but more countries are expected in the coming years so we wanted to link to the latest list of Members.
| "concepts": [ | ||
| { | ||
| "id": "air_temperature", | ||
| "title": "Air temperature", | ||
| "url": "http://vocab.nerc.ac.uk/standard_name/air_temperature/" | ||
| }, | ||
| { | ||
| "id": "wind_speed", | ||
| "title": "Wind Speed", | ||
| "url": "http://vocab.nerc.ac.uk/standard_name/wind_speed/" | ||
| }, | ||
| { | ||
| "id": "wind_to_direction", | ||
| "title": "Wind to diection", | ||
| "url": "http://vocab.nerc.ac.uk/standard_name/wind_to_direction/" | ||
| } | ||
| ], | ||
| "scheme": "https://vocab.nerc.ac.uk/standard_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.
This seems like a pretty incomplete list?
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.
@StuartJMatthews Could you comment this?
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 agree the list isn't as full as it could be. We plan to add more, but wanted to start with something.
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.
@Teddy-1000, @lukas-phaf. I expect the E-SOH discovery metadata, as well as the other discovery data published in WIS 2.0 will evolve, as best practice is developed.
Co-authored-by: Lukas Phaf <lukas.phaf@knmi.nl>
Co-authored-by: Lukas Phaf <lukas.phaf@knmi.nl>
…URODEO/e-soh into add-option-for-custom-openapi-metadata
No description provided.