aws sqs event schema #8227
aws sqs event schema #8227
Conversation
There was a problem hiding this comment.
@thewizarodofoz great thanks for picking this up!
In general it looks good. It'll be just great to have schema a bit more detailed (see my comment), and also with a schema we tend to remove all inline validation of properties, as with schema it's a superfluous logic that just adds to maintenance cost.
e.g. see what's removed in stream schema PR: https://github.com/serverless/serverless/pull/8201/files
| type: 'object', | ||
| properties: { | ||
| arn: { $ref: '#/definitions/awsArn' }, | ||
| batchSize: { type: 'integer' }, |
There was a problem hiding this comment.
We should configure minimum and maximum constraints as indicated in AWS doc: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-lambda-eventsourcemapping.html#cfn-lambda-eventsourcemapping-batchsize
There was a problem hiding this comment.
Thanks @medikoo .
Regarding removing inline validation - should I also remove all the tests that test those validations?
I assumed they would still apply (expecting ajv to now throw the errors instead of the inline validations), but it seems to me the tests aren't invoking the ajv validation. Meaning, I messed up the schema deliberately and expected that the tests will fail with ajv errors but they didn't.
There was a problem hiding this comment.
Regarding removing inline validation - should I also remove all the tests that test those validations?
Yes, definitely :)
There was a problem hiding this comment.
but it seems to me the tests aren't invoking the ajv validation
Yes, it's a problem with tests. Ideally they should be written with help of runServerless util, but it's old tests, that were introduced before we've changed the approach.
We would welcome the PR that refactors those tests, still that should be not in scope of this PR.
Also we agreed to not test that schema validation works (it increases maintenance cost and seems a bit superfluous when schema validation engine is extensively tested on its own). In that context we should test just valid scenarios, and that already makes a great tests that confirm that by accident we didn't outrule a valid notation.
Codecov Report
@@ Coverage Diff @@
## master #8227 +/- ##
==========================================
- Coverage 88.16% 88.10% -0.07%
==========================================
Files 248 248
Lines 9430 9379 -51
==========================================
- Hits 8314 8263 -51
Misses 1116 1116
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thank you @thewizarodofoz It looks very good. I've just spotted one issue. Please see my comment
| type: 'object', | ||
| properties: { | ||
| arn: { $ref: '#/definitions/awsArn' }, | ||
| batchSize: { type: 'integer', minimum: 1, maximum: 10000 }, |
There was a problem hiding this comment.
If I see correctly in AWS docs, for SQS maximum is 10 -> https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-lambda-eventsourcemapping.html#cfn-lambda-eventsourcemapping-batchsize
|
@thewizarodofoz let me know if it's ready for re-review. We'd love to have that in! |
Closes: #8033