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.

Sign up for GitHub

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

Jump to bottom

aws sqs event schema #8227

Merged
merged 4 commits into from Sep 22, 2020
Merged

aws sqs event schema #8227

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
300657f
aws sqs event schema
Sep 14, 2020
13db6c9
add min/max validation to batchSize
Sep 14, 2020
36c85d6
remove schema validation tests
Sep 15, 2020
3e8e8af
fix sqs batch size limit
thewizarodofoz Sep 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 13 additions & 2 deletions lib/plugins/aws/package/compile/events/sqs/index.js
View file
Open in desktop
Expand Up @@ -12,9 +12,20 @@ class AwsCompileSQSEvents {
'package:compileEvents': this.compileSQSEvents.bind(this),
};

// TODO: Complete schema, see https://github.com/serverless/serverless/issues/8033
this.serverless.configSchemaHandler.defineFunctionEvent('aws', 'sqs', {
anyOf: [{ type: 'string' }, { type: 'object' }],
oneOf: [
{ $ref: '#/definitions/awsArn' },
{
type: 'object',
properties: {
arn: { $ref: '#/definitions/awsArn' },
batchSize: { type: 'integer' },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding removing inline validation - should I also remove all the tests that test those validations?

Yes, definitely :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

enabled: { type: 'boolean' },
},
required: ['arn'],
additionalProperties: false,
},
],
});
}

Expand Down