Define SNS event schema validation#8112
Conversation
medikoo
left a comment
medikoo
left a comment
There was a problem hiding this comment.
@fredericbarthelet thank you, that's outstanding!
This one seems a bit more challenging, due to more convoluted configuration options logic. Please see my comments and let me know what you think
531c29c to
d1b0cab
Compare
|
@medikoo I updated my PR with your comments. I still have 2 remaining questions regarding your comments where I need additional information from you. Could you help me with those :) ? Thanks |
Codecov Report
@@ Coverage Diff @@
## master #8112 +/- ##
==========================================
+ Coverage 88.18% 88.21% +0.02%
==========================================
Files 248 248
Lines 9444 9416 -28
==========================================
- Hits 8328 8306 -22
+ Misses 1116 1110 -6
Continue to review full report at Codecov.
|
| arn: { | ||
| oneOf: [{ $ref: '#/definitions/awsArn' }, { $ref: '#/definitions/awsCfFunction' }], | ||
| }, |
There was a problem hiding this comment.
What do you think about renaming awsArn definition (in current form) to awsArnString, and introduce awsArn definition, that allows form as accepted by AWS, which then we can use here directly?
There was a problem hiding this comment.
What I don't like too much is that, while the current awsArn ensure we're talking about an ARN, there are no guarantee that value returned by any of the awsCfFunction are actually resolved to ARN. I could use a GetAtt to get Lambda current version and this will pass the new awsArn definition.
More largely, any value within a CF template can be replaced by an AWS function resolution.
What we could envision here, is whenever one value used in serverless.yaml will be passed as is to a CF template, this value can either be the expected primitive type of the CF template (string, number, boolean) as well as any combination of CF functions.
We could then have :
awsString: {
oneOf: [{ type: 'string', minLength: 1}, { $ref: '#/definitions/awsCfFunction' }]
},
awsNumber: ...WDYT ?
There was a problem hiding this comment.
there are no guarantee that value returned by any of the awsCfFunction are actually resolved to ARN.
My idea for awsArn is that it's expected to be a valid ARN passed as is to AWS, which we validate just on surface, and it's AWS that eventually crashes with meaningful error, when nonsense instruction is passed.
I was thinking about keeping this as definition, as I believe we have many places where we propagate ARN down to a CF template supporting all instruction forms and without thoroughly validating it. Having that this definition could well serve all those place. (I think it's better to have reusable definition for that, instead being required to put oneOf combined instruction as above in all of them)
will be passed as is to a CF template, this value can either be the expected primitive type of the CF template
One problem of that approach is that we loose some sanity validation of string version, which could be nice to have
So I wonder whether it's not better to have dedicated definitions for types that are used frequently (like ARN) and in other cases use oneOf directly as you've proposed above.
There was a problem hiding this comment.
understood @medikoo , updated accordingly :)
medikoo
left a comment
medikoo
left a comment
There was a problem hiding this comment.
@fredericbarthelet thanks, it looks great in given form! I have just few minor remarks.
d1b0cab to
03c411f
Compare
|
@medikoo I updated my PR with your latest comments |
03c411f to
435766b
Compare
medikoo
left a comment
medikoo
left a comment
There was a problem hiding this comment.
It's outstanding! Thank you @fredericbarthelet
3 participants
Closes: #8035