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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(repository-json-schema): update to pass openapi validation in azure #3504

Merged
merged 1 commit into from Sep 27, 2019

Conversation

@ericzon
Copy link

ericzon commented Aug 4, 2019

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

This PR is related with my comment in #1466 about problems found during the process of deploy an API in Azure.
With this PR is possible to deploy an API as "App Service" and connect it with the API Manager as OpenAPI.
I added the optional key 'itExtendsCompatibility' (boolean) in JsonSchemaOptions to tackle these cases. By default uses the current behaviour.
Added some tests, applied prettier & linter of the project.

P.S: by the way, trying to sign your CLA gives a 504 Error in Cloudfront.

@ericzon ericzon requested review from bajtos and raymondfeng as code owners Aug 4, 2019
@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Aug 5, 2019

@ericzon Thank you for the patch. I had a similar concern in the past - see #3309 (comment).

@dhmlau

This comment has been minimized.

Copy link
Contributor

dhmlau commented Aug 6, 2019

FYI - the failure on Travis is a timeout error:

1) cloneExampleFromGitHub (SLOW)
       extracts project files:
     Error: Timeout of 20000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/travis/build/strongloop/loopback-next/packages/cli/test/integration/generators/clone-example.integration.js)
      at listOnTimeout (internal/timers.js:531:17)
      at processTimers (internal/timers.js:475:7)
@dhmlau

This comment has been minimized.

Copy link
Contributor

dhmlau commented Aug 6, 2019

@slnode test please

@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Aug 7, 2019

@nabdelgadir Please review the PR.

Copy link
Contributor

nabdelgadir left a comment

Mostly LGTM, just a couple comments.

@dhmlau

This comment has been minimized.

Copy link
Contributor

dhmlau commented Aug 13, 2019

@nabdelgadir, is this good to land?

@nabdelgadir

This comment has been minimized.

Copy link
Contributor

nabdelgadir commented Aug 13, 2019

@dhmlau not yet.

@ericzon, what do you think about @raymondfeng's proposal?

  1. Make schema title compatible by default with a bit less expressivity (IIRC, the title needs to be unique)
  2. Populate schema description to make it readable
@dhmlau

This comment has been minimized.

Copy link
Contributor

dhmlau commented Aug 14, 2019

Thanks @nabdelgadir . Let me assign it to you since you're already helping this PR.

@ericzon

This comment has been minimized.

Copy link
Author

ericzon commented Aug 15, 2019

@dhmlau not yet.

@ericzon, what do you think about @raymondfeng's proposal?

  1. Make schema title compatible by default with a bit less expressivity (IIRC, the title needs to be unique)
  2. Populate schema description to make it readable

@nabdelgadir for me it's ok, compatible by default. @raymondfeng in 2 you propose to add some kind of extra & dynamic description like:

"description": "[original description]... Product excluding fields id, name"

right?

Copy link
Member

bajtos left a comment

Thank you @ericzon for the pull request! I agree we should emit schema titles that are compatible with a wide range of OpenAPI/JSON Schema consumers 馃憤

I disagree with the proposed implementation, let's find a better one.

packages/repository-json-schema/src/build-schema.ts Outdated Show resolved Hide resolved
@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Aug 20, 2019

@ericzon Would you please make the final update based on the team agreement? Thanks!

@ericzon

This comment has been minimized.

Copy link
Author

ericzon commented Aug 21, 2019

@ericzon

This comment has been minimized.

Copy link
Author

ericzon commented Aug 24, 2019

Updated. Now is compatible by default & description includes information about model options.

Example of model names & descriptions:

"TechnicalData"
"Description about TechnicalData model"

"TechnicalDataExcluding_id-createdAt_"
"Description about TechnicalData model (Model options: Excluding[id,createdAt])"

"TechnicalDataPartial"
"Description about TechnicalData model (Model options: Partial)"

"TechnicalDataWithRelations"
"Description about TechnicalData model (Model options: WithRelations)"

I tested in Azure to import the generated definition and is accepted.
...

@bajtos bajtos dismissed their stale review Sep 2, 2019

The major issues have been addressed.

@bajtos bajtos self-assigned this Sep 10, 2019
@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Sep 10, 2019

@ericzon I would love to see this pull request landed soon, let me know how can I help.

@bajtos bajtos referenced this pull request Sep 12, 2019
6 of 6 tasks complete
@raymondfeng raymondfeng force-pushed the ericzon:fix/adapt-to-azure-openapi branch from 2505074 to c0fd6c2 Sep 12, 2019
@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Sep 12, 2019

@ericzon I have rebased your branch against latest master and squashed all commits into one.

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Sep 13, 2019

I would like the code building title suffix to be kept simple with hardcoded start/delimiter/end characters and also improve the code building options description to use JSON.stringify or stringify-object.

I feel the current proposal is adding too much complexity that will make the code more difficult to maintain for us.

@ericzon

This comment has been minimized.

Copy link
Author

ericzon commented Sep 15, 2019

@bajtos pushed a simplified version using stringify-object.
I couldn't reuse stringifyModelSettings from utils because it creates a wrapper with settings field and we want the stringified version in a single line.

@raymondfeng raymondfeng force-pushed the ericzon:fix/adapt-to-azure-openapi branch from 1f8b31e to 7535e69 Sep 16, 2019
@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Sep 16, 2019

@ericzon I have rebased your branch against latest master of strongloop/loopback-next. In the future, please use the following to pull in latest changes from upstream:

git remote add upstream git@github.com:strongloop/loopback-next.git
git fetch --all
git rebase upstream/master
@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Sep 20, 2019

pushed a simplified version using stringify-object.
I couldn't reuse stringifyModelSettings from utils because it creates a wrapper with settings field and we want the stringified version in a single line.

@ericzon I don't see this change being applied to the patch. I don't see any new commits pushed around the time you have posted the comment. Did you perhaps forgot to git push the change?

@ericzon ericzon force-pushed the ericzon:fix/adapt-to-azure-openapi branch 5 times, most recently from 86a1ef9 to fe2119f Sep 22, 2019
@ericzon

This comment has been minimized.

Copy link
Author

ericzon commented Sep 23, 2019

@bajtos now the changes are ready to check. I've squashed and rebase too.

packages/cli/lib/utils.js Outdated Show resolved Hide resolved
Copy link
Member

raymondfeng left a comment

Please address my comments.

@ericzon ericzon force-pushed the ericzon:fix/adapt-to-azure-openapi branch 2 times, most recently from 56f9d60 to 595fc54 Sep 24, 2019
@bajtos
bajtos approved these changes Sep 24, 2019
Copy link
Member

bajtos left a comment

LGTM with two nit-picks that can be ignored.

@raymondfeng WDYT?

@ericzon ericzon force-pushed the ericzon:fix/adapt-to-azure-openapi branch 2 times, most recently from 8d4af5c to a3d8c00 Sep 24, 2019
@ericzon ericzon force-pushed the ericzon:fix/adapt-to-azure-openapi branch from a3d8c00 to f4799ea Sep 24, 2019
Copy link
Member

bajtos left a comment

Almost there!

"@hapi/hoek": "8.x.x"
}
},
"@loopback/build": {

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 26, 2019

Member

Please revert changes in package-lock.json file. Now that you are not adding new dependencies, there is no need to change the lock file either.

For future reference: https://github.com/strongloop/loopback-next/blob/master/docs/site/DEVELOPING.md#working-with-dependencies
Here is the important part: Dependencies resolved locally within the monorepo must be excluded from package-lock files.

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 26, 2019

Member

Nice, this problem was catched by our linter, I forget we have one in place!

See https://travis-ci.com/strongloop/loopback-next/jobs/238791798

Invalid package-lock entries found!
  packages/repository-json-schema/package-lock.json
    -> @loopback/build
    -> @loopback/context
    -> @loopback/core
    -> @loopback/eslint-config
    -> @loopback/metadata
    -> @loopback/repository
    -> @loopback/testlab

Run the following command to fix the problems:

  $ npm run update-package-locks

Please don't run update-package-locks and just revert changes in this file. We prefer to update lock files via https://renovatebot.com

This comment has been minimized.

Copy link
@ericzon

ericzon Sep 26, 2019

Author

Sorry, I followed the suggestion of travis. Reverted.

@@ -15,6 +15,7 @@ import {
import * as debugFactory from 'debug';
import {JSONSchema6 as JSONSchema} from 'json-schema';
import {JSON_SCHEMA_KEY, MODEL_TYPE_KEYS} from './keys';
const nativeUtils = require('util');

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 26, 2019

Member

Please use ES6 imports.

Suggested change
const nativeUtils = require('util');
import {inspect} from 'util';

Or if you want to import everything:

Suggested change
const nativeUtils = require('util');
import * as nativeUtils from 'util';

This comment has been minimized.

Copy link
@ericzon

ericzon Sep 26, 2019

Author

Changes updated

鈥idation in azure
@ericzon ericzon force-pushed the ericzon:fix/adapt-to-azure-openapi branch from f4799ea to c353a63 Sep 26, 2019
@bajtos
bajtos approved these changes Sep 27, 2019
Copy link
Member

bajtos left a comment

馃憦

Let's get approval from @raymondfeng too.

Copy link
Contributor

nabdelgadir left a comment

馃憦

@raymondfeng raymondfeng merged commit b518876 into strongloop:master Sep 27, 2019
3 of 4 checks passed
3 of 4 checks passed
coverage/coveralls Coverage decreased (-0.006%) to 91.746%
Details
Travis CI - Pull Request Build Passed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.