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

refactor: generate openapi directly instead of legacy format #482

Merged
merged 9 commits into from
Aug 14, 2019
Merged

refactor: generate openapi directly instead of legacy format #482

merged 9 commits into from
Aug 14, 2019

Conversation

d11n
Copy link
Member

@d11n d11n commented Aug 5, 2019 •
edited
Loading

fixes #481

removing the legacy format entirely means adding/changing features will be done directly to the openapi document, instead of first modifying the legacy route format and then converting that to an openapi operation...a handful of issues will be quick to resolve once this is merged...the goal of this pr is to get to 100% tests passing without changing any openapi docs, just the doc generation algorithm

@todo
Copy link

todo bot commented Aug 5, 2019

find a better place to define parameter examples

routes/lib/endpoint/add-code-samples.js

Lines 6 to 11 in 8f49c51

// TODO: find a better place to define parameter examples
const PARAMETER_EXAMPLES = {
owner: 'octocat',
repo: 'hello-world',
issue_number: 1,
title: 'title'

This comment was generated by todo based on a TODO: comment in 8f49c51 in #482. cc @octokit.

@todo
Copy link

todo bot commented Aug 5, 2019

handle optional preview blocks

routes/lib/endpoint/find-accept-header.js

Lines 7 to 12 in 8f49c51

// TODO: handle optional preview blocks
const requiredPreviewHeaderBlocks = previewBlocks.filter(block => block.required)
const defaultAcceptHeader = requiredPreviewHeaderBlocks.length
? requiredPreviewHeaderBlocks.map(block => `application/vnd.github.${block.preview}-preview+json`).join(',')
: 'application/vnd.github.v3+json'
const acceptHeaderParam = {

This comment was generated by todo based on a TODO: comment in 8f49c51 in #482. cc @octokit.

@todo
Copy link

todo bot commented Aug 5, 2019

put examples from docs in operation

routes/lib/endpoint/find-parameters.js

Lines 276 to 281 in 8f49c51

// TODO: put examples from docs in operation
// route.operation.parameters[i || XXX].example = example.data
// route.operation['x-code-samples'].XXX = example.data
}
})
}

This comment was generated by todo based on a TODO: comment in 8f49c51 in #482. cc @octokit.

@d11n
Copy link
Member Author

d11n commented Aug 5, 2019

at present:

 PASS  test/unit/options-urls-test.js 9 OK 818ms
 PASS  test/integration/landing-page-test.js 1 OK 1s
 FAIL  test/integration/endpoints-test.js 21s
Suites:   1 failed, 2 passed, 3 of 3 completed
Asserts:  378 failed, 634 passed, of 1012
Time:     21s

current failures are primarily around operation sorting and x-changes

gr2m
gr2m reviewed Aug 5, 2019
View reviewed changes
lib/endpoint/find-accept-header.js
const previewBlocks = state.blocks.filter(block => block.type === 'preview')

state.routes.forEach(route => {
// TODO: handle optional preview blocks
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment is obsolete and can be removed

Copy link
Member Author

Choose a reason for hiding this comment

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

currently, there are 73 routes on api.github.com that come through with previewBlocks that have required: false

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry for that long comment...the non-required accept headers for those routes are being ignored right now...after a cursory look, they seem to generally be used for adding extra info in the response...which if that's true, we could just always just get everything...but some analysis may be necessary to figure out exactly how to treat these, and even ensure if they're being parsed correctly as required: false

@todo
Copy link

todo bot commented Aug 8, 2019

Make code samples respect parameter types and really just be less weird

routes/lib/endpoint/add-code-samples.js

Lines 77 to 82 in 36fa241

// TODO: Make code samples respect parameter types and really just be less weird
let props, prefix
if (paramSchema.type === 'object') {
props = paramSchema.properties
prefix = paramName
} else if (paramSchema.type === 'array' && paramSchema.items.type === 'object') {

This comment was generated by todo based on a TODO: comment in 36fa241 in #482. cc @octokit.

@todo
Copy link

todo bot commented Aug 8, 2019

Remove sorting (after openapi docs are regenerated by legacy-less algo)

routes/test/integration/endpoints-test.js

Lines 35 to 40 in 36fa241

// TODO: Remove sorting (after openapi docs are regenerated by legacy-less algo)
function sortByPathThenMethod (a, b) {
switch (true) {
case a.path < b.path:
return -1
case a.path > b.path:

This comment was generated by todo based on a TODO: comment in 36fa241 in #482. cc @octokit.

@d11n
Copy link
Member Author

d11n commented Aug 8, 2019 •
edited
Loading

after part 2 commit:

222 failed, 790 passed, of 1012

@todo
Copy link

todo bot commented Aug 9, 2019

Remove temporary workarounds that help gen the same OpenAPI docs

routes/lib/endpoint/overrides/workarounds.js

Lines 82 to 87 in 840c761

// TODO: Remove temporary workarounds that help gen the same OpenAPI docs
delete operation.deprecated
delete operation['x-github'].triggersNotification
if ([
'teams-create-or-update-id-p-group-connections',
'teams-list-id-p-groups',

This comment was generated by todo based on a TODO: comment in 840c761 in #482. cc @octokit.

@d11n
Copy link
Member Author

d11n commented Aug 9, 2019

14 failures left that are not related to x-changes or x-code-samples

@d11n
Copy link
Member Author

d11n commented Aug 9, 2019

36 failures in x-code-samples and x-changes is still always empty

@d11n
Copy link
Member Author

d11n commented Aug 10, 2019

only thing left is adding x-changes (114 failures)

@todo
Copy link

todo bot commented Aug 12, 2019

Don't override provided response descriptions

routes/lib/endpoint/find-response.js

Lines 90 to 95 in 3af64bd

// TODO: Don't override provided response descriptions
const newDescription = intCode === 204
? 'Empty response'
: intCode === 418
? 'Response definition missing'
: intCode === 200

This comment was generated by todo based on a TODO: comment in 3af64bd in #482. cc @octokit.

@todo
Copy link

todo bot commented Aug 12, 2019

Only add content object if there's a response body

routes/lib/endpoint/find-response.js

Lines 101 to 106 in 3af64bd

// TODO: Only add content object if there's a response body
responses[intCode].content = {
'application/json': { schema }
}
if (body) {
// WORKAROUND: speccy does not like {"type": null}

This comment was generated by todo based on a TODO: comment in 3af64bd in #482. cc @octokit.

@todo
Copy link

todo bot commented Aug 12, 2019

kebab-case scope

routes/lib/endpoint/html-to-json.js

Lines 28 to 33 in 3af64bd

// TODO: kebab-case scope
const scope = documentationUrl.match(/\/v3\/([^/#]+)/).pop()
const state = {
baseUrl,
gheVersion,
pageOrigin,

This comment was generated by todo based on a TODO: comment in 3af64bd in #482. cc @octokit.

@d11n d11n changed the title WIP refactor: generate openapi directly instead of legacy format refactor: generate openapi directly instead of legacy format Aug 13, 2019
@d11n
Copy link
Member Author

d11n commented Aug 13, 2019

present state of things:

$ git status

On branch d11n/481-remove-legacy
Your branch is up to date with 'origin/d11n/481-remove-legacy'.

nothing to commit, working tree clean

$ npm run routes:lint

> @octokit/routes@0.0.0-development routes:lint /home/derek/code/octokit/routes
> speccy lint openapi/api.github.com/index.json --skip openapi-tags

Specification is valid, with 0 lint errors

$ npm run test:ci

> @octokit/routes@0.0.0-development test:ci /home/derek/code/octokit/routes
> node bin/run-tests.js

TAP version 13
...
ok 1 - unit tests # time=999.07ms
...
ok 2 - api.github.com integration tests # time=10187.444ms
...
ok 3 - ghe-2.15 integration tests # time=9856.876ms
...
ok 4 - ghe-2.16 integration tests # time=9742.17ms
...
ok 5 - ghe-2.17 integration tests # time=10246.14ms

1..5
# time=41091.007ms

$ git status

On branch d11n/481-remove-legacy
Your branch is up to date with 'origin/d11n/481-remove-legacy'.

nothing to commit, working tree clean

$ npm run update -- --cached

> @octokit/routes@0.0.0-development update /home/derek/code/octokit/routes
> node bin/octokit-rest-routes.js update "--cached"

🤖  Looking for sections at 755 URLs
...
🏁  done

$ git status

On branch d11n/481-remove-legacy
Your branch is up to date with 'origin/d11n/481-remove-legacy'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   openapi/api.github.com/operations/teams/add-member.json
	modified:   openapi/api.github.com/operations/teams/get-member.json
	modified:   openapi/api.github.com/operations/teams/remove-member.json

no changes added to commit (use "git add" and/or "git commit -a")

$ git diff openapi/api.github.com/operations/teams/ add-member.json

diff --git a/openapi/api.github.com/operations/teams/add-member.json b/openapi/api.github.com/operations/teams/add-member.json
index e5efabd5f..4323d056f 100644
--- a/openapi/api.github.com/operations/teams/add-member.json
+++ b/openapi/api.github.com/operations/teams/add-member.json
@@ -58,5 +58,6 @@
     "enabledForApps": true,
     "githubCloudOnly": false
   },
-  "x-changes": []
+  "x-changes": [],
+  "deprecated": true
 }

$ git diff openapi/api.github.com/operations/teams/ get-member.json

diff --git a/openapi/api.github.com/operations/teams/get-member.json b/openapi/api.github.com/operations/teams/get-member.json
index b2dd5dbb9..e399859a4 100644
--- a/openapi/api.github.com/operations/teams/get-member.json
+++ b/openapi/api.github.com/operations/teams/get-member.json
@@ -58,5 +58,6 @@
     "enabledForApps": true,
     "githubCloudOnly": false
   },
-  "x-changes": []
+  "x-changes": [],
+  "deprecated": true
 }

$ git diff openapi/api.github.com/operations/teams/ remove-member.json

diff --git a/openapi/api.github.com/operations/teams/remove-member.json b/openapi/api.github.com/operations/teams/remove-member.json
index 714319fbd..2ea7d432f 100644
--- a/openapi/api.github.com/operations/teams/remove-member.json
+++ b/openapi/api.github.com/operations/teams/remove-member.json
@@ -58,5 +58,6 @@
     "enabledForApps": true,
     "githubCloudOnly": false
   },
-  "x-changes": []
+  "x-changes": [],
+  "deprecated": true
 }

looks like the test checks weren't triggered...at least there's no indication whether they passed or failed on the checks tab...not sure what to do about this...but it's ready for review @gr2m

and just fyi...the gratuitous amounts of todos need not become issues...the important ones will be done in short order after this is merged...it's just helpful to know where in the code certain changes need to be made...some of them are effectively duplicates of existing issues

@octokit octokit deleted a comment from todo bot Aug 13, 2019
@octokit octokit deleted a comment from todo bot Aug 13, 2019
@octokit octokit deleted a comment from todo bot Aug 13, 2019
@octokit octokit deleted a comment from todo bot Aug 13, 2019
@octokit octokit deleted a comment from todo bot Aug 13, 2019
@octokit octokit deleted a comment from todo bot Aug 13, 2019
@octokit octokit deleted a comment from todo bot Aug 13, 2019
@octokit octokit deleted a comment from todo bot Aug 13, 2019
@octokit octokit deleted a comment from todo bot Aug 13, 2019
@octokit octokit deleted a comment from todo bot Aug 13, 2019
@octokit octokit deleted a comment from todo bot Aug 13, 2019
@todo
Copy link

todo bot commented Aug 14, 2019

Don't override provided response descriptions

routes/lib/endpoint/find-response.js

Lines 90 to 95 in 50acbf0

// TODO: Don't override provided response descriptions
const newDescription = intCode === 204
? 'Empty response'
: intCode === 418
? 'Response definition missing'
: intCode === 200

This comment was generated by todo based on a TODO: comment in 50acbf0 in #482. cc @octokit.

@todo
Copy link

todo bot commented Aug 14, 2019

Only add content object if there's a response body

routes/lib/endpoint/find-response.js

Lines 101 to 106 in 50acbf0

// TODO: Only add content object if there's a response body
responses[intCode].content = {
'application/json': { schema }
}
if (body) {
// WORKAROUND: speccy does not like {"type": null}

This comment was generated by todo based on a TODO: comment in 50acbf0 in #482. cc @octokit.

@todo
Copy link

todo bot commented Aug 14, 2019

Don't override provided response descriptions

routes/lib/endpoint/find-response.js

Lines 90 to 95 in ff733a9

// TODO: Don't override provided response descriptions
const newDescription = intCode === 204
? 'Empty response'
: intCode === 418
? 'Response definition missing'
: intCode === 200

This comment was generated by todo based on a TODO: comment in ff733a9 in #482. cc @octokit.

@todo
Copy link

todo bot commented Aug 14, 2019

Only add content object if there's a response body

routes/lib/endpoint/find-response.js

Lines 101 to 106 in ff733a9

// TODO: Only add content object if there's a response body
responses[intCode].content = {
'application/json': { schema }
}
if (body) {
// WORKAROUND: speccy does not like {"type": null}

This comment was generated by todo based on a TODO: comment in ff733a9 in #482. cc @octokit.

gr2m
gr2m approved these changes Aug 14, 2019
View reviewed changes
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Great work 👍

@gr2m gr2m merged commit 0249f51 into master Aug 14, 2019
@delete-merged-branch delete-merged-branch bot deleted the d11n/481-remove-legacy branch August 14, 2019 04:32
This was referenced Aug 14, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@github-actions
Copy link
Contributor

🎉 This PR is included in version 21.0.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released Released via semantic-release label Aug 14, 2019
@d11n d11n mentioned this pull request Aug 14, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gr2m gr2m gr2m approved these changes

Assignees
No one assigned
Labels
released Released via semantic-release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OpenAPI: parse as OpenAPI instead of converting legacy format to OpenAPI
2 participants
@d11n @gr2m