Skip to content

api spec issues #535

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

Closed
jessfraz opened this issue Dec 21, 2021 · 6 comments · Fixed by #579
Closed

api spec issues #535

jessfraz opened this issue Dec 21, 2021 · 6 comments · Fixed by #579
Assignees

Comments

@jessfraz
Copy link
Contributor

  1. The spec fails to validate, might want to run a validation on commits that change it, here's a sample github action, take it or leave it:
name: validate-openapi-spec
on:
  pull_request:
    paths:
      - .github/workflows/validate-openapi-spec.yml
      - spec.json
  workflow_dispatch:
    inputs:
jobs:
  format:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - uses: actions/setup-node@v2
        with:
          node-version: '14'
      - name: Install our tools
        shell: bash
        run: |
          npm install -g @apidevtools/swagger-cli
      - name: Run validation
        shell: bash
        run: |
          swagger-cli validate spec.json

Replace with the path to the spec....

  1. Nothing has tags, if we want to generate docs or anything else, tags would be very VERY useful... right now, this makes generating anything super annoying...
@ahl ahl self-assigned this Dec 21, 2021
@ahl
Copy link
Contributor

ahl commented Dec 21, 2021

I opened oxidecomputer/dropshot#223 to address the underlying problem.

And this also spawned this: OAI/OpenAPI-Specification#2833
In short: swagger-cli depends on these JSON Schema files... which aren't particularly accurate.

The invalid endpoints turn out to have this by them:

   // TODO: this should be unpublished, but for now it's convenient for the
   // console to use the generated client for this request

Nevertheless, we should emit valid OpenAPI!

With regard to tags: what are you generating and do you have particular suggestions for how we apply tags? I recall Jared looking at OpenAPI -> doc tools, but I'm not sure if we landed anywhere with that. Do you have a recommendation? I looked in RFD 4 and didn't see a recommendation for how we tag endpoints, but may have missed it. What I was thinking is that we should do the following:

  1. require each endpoint to have exactly one tag
  2. emit a document from the implementation that lists the endpoints under each tag
  3. check in that document and have an expectorate-based test that fails if the document changes to ensure that we don't change it unintentionally and to highlight it for reviewers.

@jessfraz
Copy link
Contributor Author

jessfraz commented Dec 22, 2021 via email

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 7, 2022

Thanks for the tags now we can get somewhere:

Some other notes:

  • Would be nice for the linter goober thing to validate all endpoints have the following:
    • At least 1 tag
    • A description, this is the long form description of the endpoint can be multiple paragraphs
    • Summary, this is short form, think like when you click on an endpoint in the docs sidebar this is what shows up, currently none have this so might be a dropshot change

Then other notes, and this might be a dropshot change, in #579 it is not adding the high level tags object where the tag name and description exist, this is fairly annoying since we can use this data for populating the headings of the tagged endpoints.

Here is an example of the docs https://docs-five-sooty.vercel.app/ , as you can see I'm using the description for the endpoint headings where it should be the summary.

Once we have the above we can have nicely generated docs :) Then likely ill annoy you to add example output for the endpoints, but we can cross that bridge later.

@david-crespo
Copy link
Contributor

for reference this is what redoc does if you just hand it the spec
https://console-git-main.internal.oxide.computer/docs

@jessfraz
Copy link
Contributor Author

jessfraz commented Jan 7, 2022

the other thing is, and idk if you even want this, but if we come up w a keyword like internal and you have endpoints customers might not care about we can skip them but I go back and forth on if that's necessary because who knows if they want it for something random, anyways just saying we could hide based on a tag string

@ahl
Copy link
Contributor

ahl commented Jan 8, 2022

  • Would be nice for the linter goober thing to validate all endpoints have the following:

    • At least 1 tag

That was my intention; see above.

  • A description, this is the long form description of the endpoint can be multiple paragraphs
  • Summary, this is short form, think like when you click on an endpoint in the docs sidebar this is what shows up, currently none have this so might be a dropshot change

Please file this in dropshot. My preference would be to continue to use the Rust doc comment to populate the description and to use the first line of that as the summary by convention. We could then add a lint to, say, validate that the summary was under a certain number of characters in length.

Then other notes, and this might be a dropshot change, in #579 it is not adding the high level tags object where the tag name and description exist, this is fairly annoying since we can use this data for populating the headings of the tagged endpoints.

Yes. Please file that in dropshot with a proposal for how tag metadata might be specified.

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 a pull request may close this issue.

3 participants