-
-
Notifications
You must be signed in to change notification settings - Fork 54
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
feat: new script and ci for JSON Schema validation #452
feat: new script and ci for JSON Schema validation #452
Conversation
Changes: - added schema validation script - update package.json to match current dependencies
Changes: - Changed code to previous version for a re-attempt for building the solution. - Will start from a fresh approach.
Changes: - code updated to TS and is giving the desired output when run through ts-node. - output is: `error: reference "http://json-schema.org/draft-07/schema" resolves to more than one schema` - tsconfig.json is added because it's required when we run ts-node and execute the script. -ts-node can be installed in the CI at the runtime.
Changes: - modify code with help of Sergio Moya (thank you!) - added test cases of Schema by Sergio - convert from TypeScript to JavaScript
Changes: - categorize file into draft-04 and draft-07 for ease of differentiation. - Code updated to run successfully for the draft-07 files.
This reverts commit 60436bc.
This reverts commit 297480d.
Changes: - categorize file into draft-04 and draft-07 for ease of differentiation. - code updated to run successfully for the draft-07 files. - update package.json
Changes: - updated the dependencies - rename config.json file - separete the validate-schema file into two versions
Changes: - created new `filter-file` script for filtering the files which has draft-04 and draft-07, and to process them separately - updated code, removed irrelevant comments
Changes: - updated `validate-schemas-final.js` to match the `filter-file.js` - `filter-file.js` contains the most accurate code as of now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
Changes: - update workflow to run JSON Schema validator - update `package.json` to include the validator - update directory path to run script from root of the project
I'm using ESLint for the linting of all the JavaScript files. I've seen this being implemented in the Please have a look and provide your feedback on this. Thank you. 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm so happy to see how close we are of having validation of our JSON Schema files in CI!
You did a really great job here 👏 👏 .
I wrote some comments, suggestions, and other things that I believe we must fix. Feel free to ask any question.
@AnimeshKumar923 there is one code smell SonarCloud complains https://sonarcloud.io/project/issues?resolved=false&types=CODE_SMELL&pullRequest=452&id=asyncapi_asyncapi-node&open=AYwW7h_-2LUoXdDhnTTh. It is about simplifying the code basically. I agree, it can be simplified and improved and I invite you to do it, but I would say this can be done in a separate PR. To be honest, I find this code to be readable and simple enough at this stage IMHO. I would say you can create a follow up issue in order to simplify that. Regarding linting, and as we do not have a linter in place, I won't required you to pass any linting. Instead, we will fix those once #467 is done. |
@derberg ping 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you cannot modify .github/workflows/if-nodejs-pr-testing.yml
as it comes from .github
and whatever you modify now can be overwritten later
better just modify test
script in package.json and make it fun the validate-schemas
script, so instead of "test": "npm run build && nyc mocha",
do "test": "npm run build && nyc mocha && npm run validate-schemas",
. This way you do not have to modify .github/workflows/if-nodejs-pr-testing.yml
last but not least, please do not introduce scripts names with -
as we follow approach with :
. So instead of validate-schemas
better make validate:schemas
also did you check if you have proper error handling in script that will fail GitHub action in case of validation fails? so we know that it actually gets red when error happens? I see you have one try/catch that is just doing console.error
and it will for sure not terminate script exacution
It's a new thing for me (the code smell part), still learning from it.
Okay, I see
Yes, I agree. For the code optimization, I would like to do it in a new issue and it might take some time as per my current knowledge about JavaScript and programming in general. 😅
Understood. |
Hi @derberg! 👋 Thank you for your detailed review. I'll definitely look into the things you mentioned. 👍 |
Changes: - Revert 'if-nodejs-pr-testing.yml' to original condition. - Update 'package.json', changed "test" field to include the JSON Schema validation. - Change naming convention, from 'validate-schema' to 'validate:schema'. - Modify error handling 'catch' block on line 79 to terminate script if the schema is not valid. - Applied suggestion from: asyncapi#452 (review)
lgtm, I see also that validation runs: https://github.com/asyncapi/spec-json-schemas/actions/runs/7180191694/job/19557877973?pr=452 did you at some point of time try to simulate an error to see if test will fail? |
@derberg Yes. I created a dummy Contents of {
"$id":"http://foo.bar",
"$schema":"http://json-schema.org/draft-07/schema",
"definitions":{
"http://json-schema.org/draft-07/schema":{},
"person":{
"type":"object",
"properties":{
"name":{
"type":"invalid"
}
}
}
}
} These are the results after running 1.0.0-without-$id.json: JSON Schema is valid!
1.0.0.json: JSON Schema is valid!
1.1.0-without-$id.json: JSON Schema is valid!
1.1.0.json: JSON Schema is valid!
1.2.0-without-$id.json: JSON Schema is valid!
1.2.0.json: JSON Schema is valid!
2.0.0-rc2-without-$id.json: JSON Schema is valid!
2.0.0-rc2.json: JSON Schema is valid!
2.0.0-without-$id.json: JSON Schema is valid!
2.0.0.json: JSON Schema is valid!
2.1.0-without-$id.json: JSON Schema is valid!
2.1.0.json: JSON Schema is valid!
2.2.0-without-$id.json: JSON Schema is valid!
2.2.0.json: JSON Schema is valid!
2.3.0-without-$id.json: JSON Schema is valid!
2.3.0.json: JSON Schema is valid!
2.4.0-without-$id.json: JSON Schema is valid!
2.4.0.json: JSON Schema is valid!
2.5.0-without-$id.json: JSON Schema is valid!
2.5.0.json: JSON Schema is valid!
2.6.0-without-$id.json: JSON Schema is valid!
2.6.0.json: JSON Schema is valid!
3.0.0-without-$id.json: JSON Schema is valid!
3.0.0.json: JSON Schema is valid!
all.schema-store.json: JSON Schema is valid!
0.0.0.json: JSON Schema is not valid: [
{
instancePath: '/definitions/person/properties/name/type',
schemaPath: '#/definitions/simpleTypes/enum',
keyword: 'enum',
params: { allowedValues: [Array] },
message: 'must be equal to one of the allowed values'
},
{
instancePath: '/definitions/person/properties/name/type',
schemaPath: '#/properties/type/anyOf/1/type',
keyword: 'type',
params: { type: 'array' },
message: 'must be array'
},
{
instancePath: '/definitions/person/properties/name/type',
schemaPath: '#/properties/type/anyOf',
keyword: 'anyOf',
params: {},
message: 'must match a schema in anyOf'
}
] and when executed Do we have any ways of testing this dummy in the PR itself for better assurance? 👀 |
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 1 New issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AnimeshKumar923 this PR is already running your test script 😄
@smoya LGTM so if it comes to me, we can merge
/rtm |
@smoya Thank you so much for your guidance throughout this PR! I'm very happy how it turned out in the end. 🙇 🚀 🥳 @derberg Thank you for your in-depth review. It was very crucial for this PR. 🙇 👍 Looking forward to do more contributions here. Cheers! 🥂 |
🎉 This PR is included in version 6.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@asyncapi/bounty_team |
Description
schemas
directory.It also introduces a JavaScript linter.(will be implemented through Add code linting (eslint) as part of CI script #467)Tasks
Integrate JavaScript linter. (ESLint?) Change the existing(Will be converted into a new issue in the future. UPDATE: the issue has been raised Add code linting (eslint) as part of CI script #467 )if-nodejs-pr-testing.yml
to lint JavaScript files.Change the existingInstead, do changes inif-nodejs-pr-testing.yml
to validate incoming schemas. (implemented here)test
field of the script and include execution of the script from there itself (implemented here)Related issue(s)
Fixes #436