-
Notifications
You must be signed in to change notification settings - Fork 47
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
adding nodejs-v2 and python3-v2 #28
base: master
Are you sure you want to change the base?
Conversation
I need to work out why things are breaking in Node 12 and 14 |
It's a little sneaky, but looks like the generation itself is failing in GH Actions..
|
count: { | ||
required: false, | ||
type: 'integer', | ||
}, |
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.
We will probably want an additional parameter to cover the optional required param:
}, | |
}, | |
notRequired: { | |
type: 'string', | |
description: 'this param should be optional' | |
}, |
authorisation: { | ||
required: true, | ||
description: 'the authorisation token', | ||
}, |
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.
Same here, should cover the other branches of implicit and explicit required === false
}, | |
}, | |
explicitOptional: { | |
required: false, | |
description: 'this header is optional', | |
}, | |
implicitOptional: { | |
description: 'this header is implicitly optional', | |
}, |
"license": "ISC", | ||
"devDependencies": { | ||
"serverless-auto-swagger": "file:../../../serverless-auto-swagger", | ||
"serverless-offline": "^7.1.0" |
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.
Shouldn't these also point to the root folder? Would be better to have serverless-offline
installed at the root as a devDep once than for each of Node and Python serverless to install the same version separately
"license": "ISC", | ||
"devDependencies": { | ||
"serverless-auto-swagger": "file:../../../serverless-auto-swagger", | ||
"serverless-offline": "^7.0.0" |
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.
Same q here
expect(paths.length).toEqual(3); | ||
|
||
const definitions = Object.keys(swaggerJson.definitions); | ||
expect(definitions.length).toEqual(4); |
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.
We should probably make a TODO or file an Issue to add actual parameter validation here instead of simply checking the lengths, right?
swaggerJson = JSON.parse(fileText.slice(60)); | ||
// change the 60 char if the comment at the start of the swagger.py file changes |
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.
Could you explain this one to me? There's probably a simpler way to do this, i.e. slicing the file at the index where we find whatever the python comment is
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.
That way it would always be dynamic
process.stdout.write('generating swagger files in all test repos...'); | ||
|
||
// could add some logic in here to decide whether to run the swagger generate or not | ||
// Only required when running the smoke tests. so ignote when running unit tests |
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.
😄
// Only required when running the smoke tests. so ignote when running unit tests | |
// Only required when running the smoke tests. so ignore when running unit tests |
console.log('error'); | ||
resolve(1); |
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.
Don't we want this to reject on error?
console.log('error'); | |
resolve(1); | |
console.error('Error generating swagger', err); | |
reject(err); |
console.log('stderr'); | ||
resolve(1); | ||
return; |
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.
Same question here
console.log('stderr'); | |
resolve(1); | |
return; | |
console.log('Error generating swagger', stderr); | |
reject(stderr); | |
return; |
043c27b
to
32d78d6
Compare
Adding two basic repos. I've tested these and they will both compile and run sls offline to show the swagger.
Later it would be cool to generate the swagger then to do some tests on it to make sure that everything is where it's supposed to be. Kind of like smoke tests.