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

issue 230: update to openapi and docs build #359

Merged
merged 27 commits into from
Jul 26, 2021

Conversation

jb-adams
Copy link
Member

DRS spec migrated to OpenAPI 3.0

  • new docs build system (using gh-openapi-docs) is working
  • built documentation can be viewed here
  • front matter and appendices have been migrated from asciidoc to markdown
  • multiple unused modules have been deleted, including python demo server, gradle tasks, and miscellaneous scripts (superseded by gh-openapi-docs)
  • front matter and appendix documentation should be identical to the current develop branch docs

@jb-adams jb-adams changed the title Feature/issue 230 update to openapi and docs build issue 230: update to openapi and docs build Jun 28, 2021
@jb-adams jb-adams linked an issue Jun 28, 2021 that may be closed by this pull request
@jb-adams
Copy link
Member Author

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
Copy link
Member

dglazer commented Jun 29, 2021

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

  • there are some extra blank lines in README.md
  • 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)
  • there's a lot of extra vertical whitespace between section headers (e.g. between "DRS API Principles" and "DRS IDs" and the body text)
  • how easy is it to add in section numbers on the headings? (It would make it easier to refer to them.)
  • none of the intra-spec links seem to work (at least, none of the first several I tried)
  • probably related, the link in "For more information, see the document More Background on Compact Identifiers." doesn't work
  • 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
  • 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.
  • in the third section of that Appendix, 4 of the 5 bullets in the list aren't formatted as bullets
  • there's a missing paragraph at the end of that Appendix ("This spec defines ... where my DRS endpoint lives")
  • 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?

@jb-adams
Copy link
Member Author

jb-adams commented Jul 5, 2021

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
Copy link
Member

dglazer commented Jul 5, 2021

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:

  • can you update (or remove) DOCSBUILD.md? It feels very stale.
  • in travis.yml, are you sure about changing skip_cleanup to skip-cleanup? (The web seems to say that neither are needed anymore, since the default is now to skip cleanup.)

@briandoconnor
Copy link
Contributor

@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 jb-adams force-pushed the feature/issue-230-update-to-openapi-and-docs-build branch from 393efdf to 034afaf Compare July 23, 2021 17:29
Jeremy Adams added 2 commits July 23, 2021 13:34
@jb-adams
Copy link
Member Author

Made a final cleanup push that makes the following changes:

  • remove deprecated info about how to use the old docs build system
  • updates to the markdown table of releases in README.md

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

@jb-adams jb-adams merged commit ca1b267 into develop Jul 26, 2021
@jb-adams jb-adams deleted the feature/issue-230-update-to-openapi-and-docs-build branch July 26, 2021 17:11
This was referenced Jul 26, 2021
@daisieh
Copy link

daisieh commented Aug 26, 2021

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
Copy link
Member Author

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 [email protected] soliciting advice on where some of these tools are located.

@ohofmann
Copy link

@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).

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.

Update to OpenAPI 3.0
5 participants