ga4gh / data-repository-service-schemas

A repository for the schemas used for the Data Repository Service.
Apache License 2.0
60 stars 53 forks source link

issue 230: update to openapi and docs build #359

Closed jb-adams closed 3 years ago

jb-adams commented 3 years ago

DRS spec migrated to OpenAPI 3.0

jb-adams commented 3 years ago

It would be great to have some reviewers do an editorial review to make sure no errors have been introduced with the migration (ie. consistent text + formatting, no broken links, etc). In particular anyone involved in writing/maintaining builds of the DRS spec thus far @david4096 @jaeddy @denis-yuen @briandoconnor @dglazer

dglazer commented 3 years ago

Generally looks great. Here are a bunch of relatively small comments:

jb-adams commented 3 years ago

Thanks @dglazer, I've pushed some changes to address your comments:

Items that have been addressed

  • there are some extra blank lines in README.md

Some whitespace removed (where there was more than 1 line between sections)

  • there's a lot of extra vertical whitespace between section headers (e.g. between "DRS API Principles" and "DRS IDs" and the body text)

Solved this by applying the GA4GH 'theme' to the doc (theme has less vertical spacing).

  • none of the intra-spec links seem to work (at least, none of the first several I tried)

Intra-spec links added, I believe I got all of them but always appreciate an additional confirmation

  • in the section on DRS Datatypes, the words blob and bundle should be italicized, and the tokens DRSObject and contents should be in code-style

Fixed italics and code blocks for this section

  • the Appendix: Motivation is meant to be formatted as tables, with the text on the left and the images on the right; it's currently just vertically stacked instead. That's a little annoying for the first two figures, but looks pretty bad for the third figure, which should be a lot skinnier.

Fixed using HTML directly rather than markdown

  • in the third section of that Appendix, 4 of the 5 bullets in the list aren't formatted as bullets

Restored bullet points

  • there's a missing paragraph at the end of that Appendix ("This spec defines ... where my DRS endpoint lives")

Restored this paragraph

  • probably related, the link in "For more information, see the document More Background on Compact Identifiers." doesn't work
  • we used to have a separate "More Background on Compact Identifiers" document, which had fairly arcane information that's not relevant to most spec readers, and that really isn't part of the spec itself. It looks like it ended up as an appendix instead, which bloats the spec; is it possible to move it back to a separate .md file?

I've removed "More Background on Compact Identifiers" from the main spec doc, and added it as its own page. It's being built according to the same process as the main doc. available here and via links from the spec doc (and the more background doc links back to the main spec)

Items that haven't been addressed

the table of contents is only 2 levels deep (see e.g. DRS API Principles > DRS URIs > Hostname-based DRS URIs, where the third level doesn't show up)

I could not find a setting for this, either through the gh-openapi-docs wrapper or the underlying redoc-cli tool. All documentation examples I've seen haven't had higher nesting in the table of contents

how easy is it to add in section numbers on the headings? (It would make it easier to refer to them.)

Same as above, I could not find a setting for section numbers, and I do not see any examples of this.

Final Notes

As a final point, I was able to restore the 'Models' Section with the new build system by following how this was accomplished in the WES spec. Thank you @jaeddy !

@dglazer and @briandoconnor I think we are ready now for an open for comment period. If I get the green light from you I will send out the notice to the mailing list.

dglazer commented 3 years ago

This LGTM Jeremy -- it's ready for wider comments in my opinion.

Thank you for the detailed followup. (And great to see a net deletion of over 7000 lines -- big win for maintenance going forward.) There are still some minor formatting things I'd like to see improved if and when Redocly adds support (e.g. section numbers), but definitely not blocking.

Two small things to look at in parallel with the wider comment period:

briandoconnor commented 3 years ago

@jb-adams this looks great to me! I'd like to get this merged in as soon as possible and then rebase my PR for POST'ing passports ASAP.

jb-adams commented 3 years ago

Made a final cleanup push that makes the following changes:

I replaced the "swagger-ui" links for each DRS release with Swagger Editor, since going forward the new build system doesn't build Swagger-UI pages directly. But we can link out to Swagger Editor (editor.swagger.io) using the URL to the built docs for each release.

A couple notes about the release table, I removed the master row since the links were not functioning correctly. The HTML link currently takes one to a 404 on the Github Pages site. We can add this row back in once we close develop onto master and new docs are generated there.

Also, the develop Swagger Editor link currently doesn't work. That's because the URL referred to by swagger editor is .../preview/develop/openapi.yaml, but the current file is named .../preview/develop/swagger.yaml. Once we merge this PR and develop rebuilds itself under the new system, this file will get renamed to openapi.yaml

daisieh commented 3 years ago

So...I'm pretty late to this party, but I've just started working with our htsget implementation, and am trying to figure out how to bring it up to spec. I see that the python server (which corresponds to the ga4gh-dos-schemas pypi package?) is deprecated and just removed entirely. Does that mean that there is no longer an official DRS Client implementation for us to use?

jb-adams commented 3 years ago

Hi @daisieh , with the recent DRS spec update we removed a lot of the code autogen from this repo, so it hosts only the spec itself.

There is an open DRS client here, but this hasn't been maintained/updated in a while. There are other open source tools available developed by other contributors/orgs. It might be good to send an email to the GA4GH Cloud Work Stream mailing list ga4gh-cloud@ga4gh.org soliciting advice on where some of these tools are located.

ohofmann commented 3 years ago

@daisieh I can help with the connection; this likely also overlaps with something we'e looking at for the upcoming Connect meeting (htsget + DRS + passport as a framework to understand interoperability problems).