Skip to content
This repository has been archived by the owner on Jul 27, 2020. It is now read-only.

OpenAPI specification format #168

Merged
merged 119 commits into from
Jul 23, 2019
Merged

OpenAPI specification format #168

merged 119 commits into from
Jul 23, 2019

Conversation

gr2m
Copy link
Contributor

@gr2m gr2m commented Jun 5, 2018

Preview 👀

Base of discussion. Starting out with specifications for issues (list, get, create, delete) to see if the format is usable across Octokit .js, .net and .rb

Continuously deployed to https://octokit-routes-openapi.now.sh/

Breaking changes

Keeping track of things that will change besides the format

  • The repos/get-commit-ref-sha will be removed, as it’s not a separate endpoint but only sets a custom media format header comparet to repos/get-commit

The plan

✅ OpenAPI specification

Created by hand. Goal is to have a reference for the next steps

✅ Generate OpenAPI specification

Use the reference OpenAPI specification for the issue endpoints and generate it the way `index.json` is currently generated

See #328

✅ Adapt @octokit/rest to use OpenAPI specification

See WIP PR: https://github.com/octokit/rest.js/pull/1375

Proof of concept how Octokit libraries can be generated from the Open API spec.

  • read OpenAPI spec (APIDevTools/json-schema-ref-parser)
  • generate plugins/rest-api-endpoints/routes.json file with same format
  • add parameters from requestBody
  • ignore legacy routes
  • enum values
  • required api preview headers
  • ignore GitHub Cloud-only routes
  • .allowNull for parameters
  • deprecations for endpoint idName changes
  • deprecations for parameter name changes
  • objectify nested parameters such as output.annotations[].message which currently use "output.annotations[].message" as key, instead of output: { annotations: { items: { message } } } } (simplified)
  • correct response status, not hard coded to 200
  • parameters mapTo
  • custom headers: "content-type": "text/plain; charset=utf-8" for markdown.renderRaw
  • param validation, e.g. /projects/columns/:column_id/moves and /projects/columns/cards/:card_id/moves
  • adapt usage for @octokit/rest docs
  • add "enabledForApps" flag
  • add "default" for parameters (e.g. page, per_page)

🏗 Merge openapi branch into master

🔜 Clean up implementation

It’s currently chaotic because the internal methods all scrape the data into the previous format, then at the end we transform the previous format to OpenAPI. We have to get rid of the previous format throughout the code base in order to make the code base maintainable and lower to barrier for outside contribution

🔮 Backlog

  • Adapt other octokit/* repositories to new OpenAPI specification

After merge

  • Install Greenkeeper app

See also https://github.com/octokit/routes/milestone/1

openapi.json Outdated
"type": "string",
"enum": ["*"]
}
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Getting the "oneOf" schema automatically from scraping the docs might be tricky, but if it’s useful we will find a way

@gr2m
Copy link
Contributor Author

gr2m commented Jun 5, 2018

The OpenAPI will force us to have unique routes per endpoint because of the way they are defined

{
  "paths": {
    "/repos/:owner/:repo/issues": {
      "get": {
        ...
      }
    }
  }
}

For that we would need to change

as the paths are technically the same.

@gr2m
Copy link
Contributor Author

gr2m commented Jun 6, 2018

run npm install & npm routes:server :) Requires Node 8+

image

"pull-request-summary": "",
"issue": "id,number"
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryangribble could you add an explanation on how you use that property for octokit.net?

Choose a reason for hiding this comment

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

In the Issue.cs response model, we use it to generate this property:

internal string DebuggerDisplay => string.Format(CultureInfo.InvariantCulture, "Issue Id: {Id}, Number: {Number}");

Choose a reason for hiding this comment

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

for each response object (schema) we need to know what are the "important" fields that should be shown when this object is being shown (eg in debugger window or mouse over)

Copy link

@ryangribble ryangribble Jun 8, 2018

Choose a reason for hiding this comment

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

there was no suitable field on the actual properties openAPI entry, so I went down the path of using OpenAPI extensions. They can only apply to certain places though, and under schemas isnt one of those... so although its not ideal to have them at the top info level, there wasn't a more suitable place really.

If the goal is to generate everything from "the source" then I think we will need to end up adding certain additional info into the openAPI description, because maintaining something like this on the octokit client side means a manual update whenever a new response schema is added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the "important" fields are the URL parameters in general? So e.g. for GET /repos/:owner/:repo/issues/:number it would be ["owner", "repo", "number"]?

Copy link

@ryangribble ryangribble Jun 8, 2018

Choose a reason for hiding this comment

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

It's a mismatch though, because these objects are declared in schemas area, so there is no url/parameters there (those are under operations/responses)

@kytrinyx
Copy link
Contributor

the paths are technically the same.

I think I'm missing something.

GET /repos/:owner/:repo/git/refs/:ref

vs

GET /repos/:owner/:repo/git/refs

These are not the same, right?

But then... looking more closely, GET /repos/:owner/:repo/git/refs/tags where tags can be interpreted as a :ref. Is that what you meant?

@kytrinyx
Copy link
Contributor

As far as I can tell, this would work well for generating the Ruby client.

@gr2m
Copy link
Contributor Author

gr2m commented Jun 12, 2018

@kytrinyx for example, say I have a branch "feature", I can get it with

GET /repos/:owner/:repo/git/refs/heads/feature

Response is an object

{
  "ref": "refs/heads/feature",
  "node_id": "MDM6UmVmcmVmcy9oZWFkcy9mZWF0dXJlQQ==",
  "url": "https://api.github.com/repos/octocat/Hello-World/git/refs/heads/feature",
  "object": {
    "type": "commit",
    "sha": "aa218f56b14c9653891f9e74264a383fa43fefbd",
    "url": "https://api.github.com/repos/octocat/Hello-World/git/commits/aa218f56b14c9653891f9e74264a383fa43fefbd"
  }
}

Now, some time passes, the feature branch was merged and deleted. I work on two features now with the branches feature/hello and feature/bar. The exact same requests from above

GET /repos/:owner/:repo/git/refs/heads/feature

Now returns an array

[
  {
    "ref": "refs/heads/feature/hello",
    "node_id": "MDM6UmVmcmVmcy9oZWFkcy9tYXN0ZXI=",
    "url": "https://api.github.com/repos/octocat/Hello-World/git/refs/heads/feature/hello",
    "object": {
      "type": "commit",
      "sha": "aa218f56b14c9653891f9e74264a383fa43fefbd",
      "url": "https://api.github.com/repos/octocat/Hello-World/git/commits/aa218f56b14c9653891f9e74264a383fa43fefbd"
    }
  },
  {
    "ref": "refs/heads/feature/world",
    "node_id": "MDM6UmVmcmVmcy9oZWFkcy9naC1wYWdlcw==",
    "url": "https://api.github.com/repos/octocat/Hello-World/git/refs/heads/feature/world",
    "object": {
      "type": "commit",
      "sha": "612077ae6dffb4d2fbd8ce0cccaa58893b07b5ac",
      "url": "https://api.github.com/repos/octocat/Hello-World/git/commits/612077ae6dffb4d2fbd8ce0cccaa58893b07b5ac"
    }
  }
]

Both requests use the same route, but receive different type of responses depending on wether the URL part after /repos/:owner/:repo/git/refs/ matches the exact name of a reference or a prefix of multiple references.

The tricky part in Get all references is this

You can also request a sub-namespace

And in the 2nd example /heads/feature is treated as a sub-namespace

@kytrinyx
Copy link
Contributor

kytrinyx commented Jun 14, 2018

@gr2m Thank you, this is exactly what I was missing.

This is a problem also because the same endpoint can return two types (array vs object). We definitely need to look at the API design for this.

@gr2m

This comment has been minimized.

@gr2m gr2m force-pushed the openapi branch 4 times, most recently from 862beb8 to 58b60f4 Compare June 20, 2018 20:38
@gr2m

This comment has been minimized.

@ryangribble

This comment has been minimized.

@gr2m

This comment has been minimized.

@MikeRalphson

This comment has been minimized.

@gr2m

This comment has been minimized.

@gr2m

This comment has been minimized.

@MikeRalphson

This comment has been minimized.

@gr2m

This comment has been minimized.

@MikeRalphson

This comment has been minimized.

@MikeRalphson

This comment has been minimized.

@github-actions
Copy link
Contributor

🎉 This PR is included in version 21.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released Released via semantic-release label Jul 23, 2019
@zeke
Copy link

zeke commented Jul 30, 2019

So awesome to see this ship! Thank you @gr2m 🚀

@gr2m
Copy link
Contributor Author

gr2m commented Jul 30, 2019

And @d11n! Derek from dojo4 (who took on maintainership of octokit.rb) greatly helped in the past weeks 👏

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
released Released via semantic-release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants