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

OpenAPI 3.0 #2

Merged
merged 11 commits into from Apr 5, 2019

Conversation

Projects
None yet
4 participants
@raymondfeng
Copy link
Member

raymondfeng commented Mar 11, 2019

Description

Upgrade https://github.com/strongloop/loopback-connector-swagger to support OpenAPI 3.0.

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@raymondfeng raymondfeng force-pushed the openapi-3.0 branch from af5bf8e to bbca244 Mar 11, 2019

@raymondfeng raymondfeng marked this as a duplicate of #1 Mar 11, 2019

@raymondfeng raymondfeng reopened this Mar 11, 2019

@raymondfeng raymondfeng marked this as not a duplicate of #1 Mar 11, 2019

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Mar 12, 2019

The patch is pretty large (+1,900 −1,271 lines of code), I don't have bandwidth and energy to review it in detail again. I already posted quite a few comments in strongloop/loopback-connector-swagger#52, I think many of them applies to this pull request too.

My main concern is about the inclusion of an LB4 example app as a standalone project inside this repository. How are you integrating npm install, npm run build and possibly also npm test into the CI build pipeline of the connector project?

IMO, the LB4 example app should live in loopback-next monorepo.

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Mar 14, 2019

The main purpose of the embedded LB4 app is for testing OpenAPI 3.0 compliant apis. I already make it part of the test as follows:

  1. In https://github.com/strongloop/loopback-connector-openapi/blob/openapi-3.0/package.json#L10, there is a pretest script to build the LB4 app.

  2. The LB4 app is programmatically started/stopped as part of the test - https://github.com/strongloop/loopback-connector-openapi/blob/openapi-3.0/test/test-connector-openapi3.js#L177. There is no extra CI logistics involved.

I'm open to use an existing LB4 example published to npm and declare a devDep to it in this module.

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Mar 14, 2019

@bajtos I switched to @loopback/example-todo as a dev dependency for testing.

@raymondfeng raymondfeng force-pushed the openapi-3.0 branch 2 times, most recently from 0f6056a to 1fbc01b Mar 15, 2019

@raymondfeng raymondfeng requested a review from bajtos Mar 18, 2019

@raymondfeng raymondfeng force-pushed the openapi-3.0 branch from 040d598 to 7ec821f Mar 18, 2019

@bajtos
Copy link
Member

bajtos left a comment

I switched to @loopback/example-todo as a dev dependency for testing.

Great idea! The code is much easier to review now.

var should = require('should');
var loopback = require('loopback');
// eslint-disable-next-line no-unused
/* eslint-disable camelcase */

This comment has been minimized.

Copy link
@bajtos

bajtos Mar 19, 2019

Member

Are these two eslint comments still relevant?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Mar 27, 2019

Author Member

Yes. should is unused and there are a few non-camelcase keys.

This comment has been minimized.

Copy link
@bajtos

bajtos Mar 28, 2019

Member

Well if should is not used, then why are we keeping it around? Can you simply remove L10?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Apr 2, 2019

Author Member

I believe should is required so that *.should works.

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 5, 2019

Member

Makes sense 👍

Show resolved Hide resolved test/test-connector-auth.js Outdated
Show resolved Hide resolved test/test-connector-openapi3.js Outdated
Show resolved Hide resolved test/test-connector-openapi3.js Outdated
Show resolved Hide resolved test/test-connector-openapi3.js Outdated

@raymondfeng raymondfeng force-pushed the openapi-3.0 branch from 7ec821f to 153a12d Mar 27, 2019

@raymondfeng raymondfeng requested a review from bajtos Mar 27, 2019

@jannyHou

This comment has been minimized.

Copy link

jannyHou commented Mar 27, 2019

Great to see the upgrade! I may not have time to review it this week(no need wait for my approve if it's ready to land), will take a look in April.

@bajtos
Copy link
Member

bajtos left a comment

Almost there!

Please get at least one more person to review & approve the changes too.

var should = require('should');
var loopback = require('loopback');
// eslint-disable-next-line no-unused
/* eslint-disable camelcase */

This comment has been minimized.

Copy link
@bajtos

bajtos Mar 28, 2019

Member

Well if should is not used, then why are we keeping it around? Can you simply remove L10?

Show resolved Hide resolved test/test-connector-auth.js Outdated

@raymondfeng raymondfeng force-pushed the openapi-3.0 branch from 153a12d to ada424e Apr 2, 2019

@raymondfeng raymondfeng requested a review from bajtos Apr 2, 2019

cb(new Error('Invalid swagger specification type'), null);
});
exports.resolve = function resolveSpec(spec, options, cb) {
if (typeof options === 'function' && !cb) {

This comment has been minimized.

Copy link
@jannyHou

jannyHou Apr 3, 2019

Any reason deleting the old code started from here ?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Apr 4, 2019

Author Member

The old logic is already supported by swagger-parser module.

@jannyHou
Copy link

jannyHou left a comment

I am not very familiar with the code base. I reviewed the test cases and new fixtures and don't find any problem, so I would trust the tests results. Great effort and LGTM 👍

@bajtos

bajtos approved these changes Apr 5, 2019

@raymondfeng raymondfeng merged commit c4a3096 into master Apr 5, 2019

2 checks passed

Commit Linter commits are all properly formatted
Details
PR Linter PR is up to date
Details

@raymondfeng raymondfeng deleted the openapi-3.0 branch Apr 5, 2019

@bajtos bajtos added the enhancement label Apr 9, 2019

raymondfeng added a commit that referenced this pull request Apr 12, 2019

4.0.1
 * OpenAPI 3.0 (#2) (Raymond Feng)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.