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

#257: Update OpenAPI definition to catalog / dataset definition #271

Merged
merged 10 commits into from
Oct 10, 2018

Conversation

m-mohr
Copy link
Collaborator

@m-mohr m-mohr commented Oct 9, 2018

Fixing #257. Please review it carefully, not sure whether I missed something.

I'll also address #139 here and implement a title for links. This also makes links more consistent with assets.

It adopts to the smaller catalog spec and adds the collection/dataset fields to the WFS-based /collections/... endpoints.

@m-mohr m-mohr added the prio: must-have required for release associated with label Oct 9, 2018
@m-mohr m-mohr added this to the 0.6.0-RC1 milestone Oct 9, 2018
@m-mohr m-mohr requested review from cholmes and a user October 9, 2018 13:45
@m-mohr m-mohr changed the title [WIP] #247: Update OpenAPI definition to catalog / dataset definition #247: Update OpenAPI definition to catalog / dataset definition Oct 9, 2018
@m-mohr m-mohr changed the title #247: Update OpenAPI definition to catalog / dataset definition #257: Update OpenAPI definition to catalog / dataset definition Oct 9, 2018
@m-mohr m-mohr requested a review from hgs-msmith October 9, 2018 15:32
Copy link
Contributor

@cholmes cholmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm +1 on merging, though I don't have an implementation that these drive, so it was a more superficial review.

There's two things that I think may be outside the scope of this PR, but would be good to address.

One is that the OpenAPI 'title' of WFS3+STAC document says '(standalone)' instead of 'WFS3'. See

title: The SpatioTemporal Asset Catalog API (standalone)

The other is that the /stac/ endpoint example could use a bit more explanation/text. It'd be good to explain that it can return a Catalog or a Dataset. And that the most common return would be a Catalog that has sub-catalogs of Datasets. And explain a bit that the intent is for users to browse with STAC Browser or for search engines to crawl. So all data exposed in search should be linked to from /stac/, organized in some way.

@m-mohr
Copy link
Collaborator Author

m-mohr commented Oct 10, 2018

@cholmes I addressed both issues, but of course descriptions and explanations could still be improved further in a seperate PR.

We now have two reviews and would be ready to merge, but I'd still like @hgs-truthe01 to review this PR before as he was the person behind most of the work for the API spec. Do you have some spare time for this, Tim?

@ghost
Copy link

ghost commented Oct 10, 2018

I think the basic idea is for the generated files to go in /api-spec/file.yaml and they should be generated from a file of the same name in /definitions/file.yaml. The yaml fragments that contribute to each definition would come from /extensions.

So I would probably move the collections fragment to /extensions.

The WFScore.fragment.yaml file kind of breaks the rule because it isn't really a STAC extension, but it is a fragment. I'm not sure I really like it in definitions, but I wasn't sure where else to put it.

Otherwise looks good to me.

Thanks and sorry I have not been as responsive!

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm going to approve this as is. I think another discussion on fragment organization might be on the table, but I don't think that is needed for 0.6.

@m-mohr
Copy link
Collaborator Author

m-mohr commented Oct 10, 2018

Okay, thanks.
I agree fragment organization doesn't seem very intuitive. I may just vote to put everything in one directory or so ;)

@m-mohr m-mohr merged commit 40ecc99 into dev Oct 10, 2018
@m-mohr m-mohr deleted the update-api-spec branch October 10, 2018 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio: must-have required for release associated with
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants