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

CI - combined OpenAPI doc #87

Merged
merged 22 commits into from
Dec 10, 2020
Merged

CI - combined OpenAPI doc #87

merged 22 commits into from
Dec 10, 2020

Conversation

matthewhanson
Copy link
Collaborator

Related Issue(s): #
#12

Proposed Changes:

  1. Create combined OpenAPI yaml from all conformance classes
  2. Publish combined doc to gh-pages
  3. Include index.html to display rendered versions of openapi

See examples for latest published versions of this branch:
https://radiantearth.github.io/stac-api-spec/dev/
https://radiantearth.github.io/stac-api-spec/dev/core
https://radiantearth.github.io/stac-api-spec/dev/item-search
https://radiantearth.github.io/stac-api-spec/dev/ogcapi-features
https://radiantearth.github.io/stac-api-spec/dev/ogcapi-features/extensions/transaction
https://radiantearth.github.io/stac-api-spec/dev/ogcapi-features/extensions/version

Combining the files was difficult, and was a combination of arguments to bundling with swagger-combine. I had to comment out geometrycollectionGeoJSON in commons.yaml to get the files to build. So that entry is missing for now. I tried a few other merge tools including openapi-merge-cli and just yq but there were issues with each of them.

I think this is good enough for a beta.1, I didn't get time to update the documentation on using swagger-combine. I'd like to create an example a developer might use to merge the published files, with examples on how to exclude certain parameters or paths (this can be specified in swagger-combine config file)if they have not been implemented.

@matthewhanson
Copy link
Collaborator Author

Looks like there is a larger problem here, the combined doc doesn't include ogcapi-features endpoints:
https://radiantearth.github.io/stac-api-spec/dev/

There was a name conflict between core / (root) and ogcapi-features / (root) so I thought I was excluding / from ogcapi-features, but it ended up excluding all paths from that file. Trying to remove root from core to see if that would work I also got an error for conflicting paths /collections/{collectionId}/items and /collections/{collectionId}/items/{featureId}....so this is a more complicated issue as it seems merged paths should not have any common base in the URL.

@matthewhanson
Copy link
Collaborator Author

matthewhanson commented Dec 9, 2020

So for now I removed the transactions extension from the complete openapi.yaml, and also the STAC core root (/) endpoint so it wouldn't conflict with the ogcapi one....although it looks like the OGC one is defined the same way.

https://radiantearth.github.io/stac-api-spec/dev/

@m-mohr
Copy link
Collaborator

m-mohr commented Dec 9, 2020

Oh, so now you are also struggling with the issues I was facing, too. I had hoped you had found a solution for them. I don't think there's an easy way out. The only solution I could think of could be to use the yaml-files tool again for merging the bundled files... If it's not complete, what is the benefit of the combined file?

@matthewhanson
Copy link
Collaborator Author

Well, this at least has all the most common pieces: core, features, item-search.

It seems like each one of the tools has something wrong with it, some are known issues, or other people have requested the feature. swagger-combine is the closest.

@cholmes
Copy link
Collaborator

cholmes commented Dec 9, 2020

although it looks like the OGC one is defined the same way.

We added our conformsTo, so ours is defined a bit differently. But if it's the fastest route forward we could just change ours to be the exact same, with the /conformance - I can change the markdown if that's the way we want to go. I'm inclined to do that if it's the only thing off.

@cholmes
Copy link
Collaborator

cholmes commented Dec 9, 2020

Actually, I think if we're not including transaction we shouldn't include the version extension. Version is still proposal, and swagger looks like it's as important as item-search and features:
Screen Shot 2020-12-09 at 9 17 45 AM

@m-mohr
Copy link
Collaborator

m-mohr commented Dec 9, 2020

We added our conformsTo, so ours is defined a bit differently. But if it's the fastest route forward we could just change ours to be the exact same, with the /conformance - I can change the markdown if that's the way we want to go. I'm inclined to do that if it's the only thing off.

We can solve that easily, let me issue a quick PR.

Edit: It's here: #88

@m-mohr
Copy link
Collaborator

m-mohr commented Dec 9, 2020

Actually, I think if we're not including transaction we shouldn't include the version extension. Version is still proposal, and swagger looks like it's as important as item-search and features:

Sort, Query, ... are also included (but they have no OpenAPI Tags, which render the menu). So removing version doesn't make it "extension-free", which I thought would be a good idea...
We can either choose "no combine" or an incomplete state in-between with some extensions included and some excluded.

Copy link
Collaborator

@m-mohr m-mohr left a comment

Choose a reason for hiding this comment

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

See comment above

core/commons.yaml Outdated Show resolved Hide resolved
@matthewhanson
Copy link
Collaborator Author

matthewhanson commented Dec 9, 2020 via email

@cholmes
Copy link
Collaborator

cholmes commented Dec 9, 2020

Sort, Query, ... are also included (but they have no OpenAPI Tags, which render the menu). So removing version doesn't make it "extension-free", which I thought would be a good idea...

I think the other extensions are fairly well marked - when you see them in the openapi html it's clear they are optional extensions. So overall I like them in. My beef with version is mostly that it's 'proposal'. If we wanted a clear rule I'd say extensions get included when they are at least pilot (potentially a higher bar as we evolve).

I think we can skip the combined doc for release and I can spend some more time figuring out the geometry collection problem

Agreed, let's just leave it out for this release, we can mark it as something that will come in the next release. What's needed to leave it out?

@m-mohr m-mohr added this to the 1.0.0-beta.1 milestone Dec 9, 2020
@matthewhanson
Copy link
Collaborator Author

@cholmes @m-mohr I've reverted the change to the commons.yaml, and disabled the step to combine.

So this is good to merge as is, there's a couple small changes, and it's using swagger-cli now instead of speccy.

The problem with swagger-combine is that is components get lost, and not all references are resolved beforehand. I've got some more things I'd like to try, once we figure out the best tool, or likely tools, to use, then we can create a Node script to do it and remove the bash script.

So this is good to merge, @m-mohr will need to approve as he requested changes

@m-mohr
Copy link
Collaborator

m-mohr commented Dec 9, 2020

https://api.stacspec.org/dev/ is broken - should that load core for now? Just so that for the release there is something loading up at https://api.stacspec.org/v1.0.0-beta.1? There are links pointing to the URL.

Just curious: what's the advantage of swagger-cli over speccy?

@matthewhanson
Copy link
Collaborator Author

There were some nested references due to circular dependencies that speccy didn't dereference...which is OK for the individual files, but it causes swagger-combine to break because it keeps the refs but doesn't carry over components from the individual files.
So it doesn't really matter for now, except that speccy appears to have gone quiet, last update was 2019, so figured it was best to stay with swagger-cli which has been more recently maintained.

Sure, I can drop the core file under dev/

@m-mohr
Copy link
Collaborator

m-mohr commented Dec 9, 2020

Okay, thank you.

I'd just duplicate the index.html for now or do a redirect (we could also add this later manually after the release by commiting directly to the gh-pages branch, so if that is too cumbersome for the CI at the moment). The core folder should still resolve.

@matthewhanson
Copy link
Collaborator Author

@m-mohr ok, it will add the core openapi to the root under the version.

@matthewhanson matthewhanson merged commit 0181d71 into master Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants