layers schema #8299
layers schema #8299
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8299 +/- ##
==========================================
- Coverage 88.02% 88.01% -0.01%
==========================================
Files 249 249
Lines 9308 9305 -3
==========================================
- Hits 8193 8190 -3
Misses 1115 1115
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thank you @thewizarodofoz ! It'll be great to have that in, and it looks very promising, please see my remarks
lib/configSchema.js
Outdated
| individually: { type: 'boolean' }, | ||
| path: { type: 'string' }, | ||
| artifact: { type: 'string' }, | ||
| exclude: { type: 'array', items: { type: 'string' } }, | ||
| include: { type: 'array', items: { type: 'string' } }, | ||
| excludeDevDependencies: { type: 'boolean' }, |
There was a problem hiding this comment.
As I inspect the code we only support artifact, include and exclude property on layer.package, so schema should be different from top level package property, and I think best to treat both as different.
lib/configSchema.js
Outdated
| package: { | ||
| package: packageSchema, | ||
|
|
||
| layers: { |
There was a problem hiding this comment.
This is an AWS specific property, so I think we should define it in context o AWS provider, in same way as resources (which also initially were defined here, but we moved it with: 6d7e967)
There was a problem hiding this comment.
So should I remove layers from lib/configSchema.js altogether? or keep the simple {type: 'object'} schema there?
There was a problem hiding this comment.
I believe it should be moved completely, exactly in a same way as resources where moved with this commit: 6d7e967
lib/configSchema.js
Outdated
| ], | ||
| }, | ||
| }, | ||
| compatibleRuntimes: { type: 'array', items: { type: 'string' }, maxItems: 5 }, |
There was a problem hiding this comment.
It'll be good to rely on awsLambdaRuntime definition
lib/configSchema.js
Outdated
| items: { | ||
| oneOf: [ | ||
| { type: 'integer', minimum: 100000000000, maximum: 999999999999 }, | ||
| { type: 'string', pattern: '^\\d{12}$' }, |
There was a problem hiding this comment.
lib/configSchema.js
Outdated
| oneOf: [ | ||
| { type: 'integer', minimum: 100000000000, maximum: 999999999999 }, | ||
| { type: 'string', pattern: '^\\d{12}$' }, | ||
| { const: '*' }, |
There was a problem hiding this comment.
I would remove it and rely on pattern as in AWS docs
There was a problem hiding this comment.
@thewizarodofoz thanks for update! It looks promising, still if I see correctly, in current form this schema is totally ignored.
To have it working, we need to introduce exactly same changes as we did here 6d7e967 for resources
(note changes in lib/classes/ConfigSchemaHandler/index.js and I think it'll be nice to add documentation line at docs/providers/aws/guide/plugins.md)
| { type: 'integer', minimum: 100000000000, maximum: 999999999999 }, | ||
| { | ||
| type: 'string', | ||
| pattern: '\\d{12}|\\*|arn:(aws[a-zA-Z-]*):iam::\\d{12}:root', |
There was a problem hiding this comment.
I think we need to enclose it with ^ and $
@medikoo Not sure that I follow. If I understand correctly, |
There was a problem hiding this comment.
@medikoo Not sure that I follow. If I understand correctly, resources is mentioned there in the context of "Definition for eventual top level "resources" section", but in this PR we said that layers is not a top level section (it's aws specific) so it will not be relevant to a new custom provider.
layers actually remain top-level, we didn't change that. We just moved it's definition to AWS context.
In this PR we move it's definition, and a capability to define layers through defineProvider schema extension utility, and to have that fully complete, it'll be great to also expose in docs that layers can be defined via defineProvider. See full docs for defineProvider: https://github.com/serverless/serverless/blob/6d7e96722721c8a5c614bea2802af7142011d35f/docs/providers/aws/guide/plugins.md#defineprovider-helper
There was a problem hiding this comment.
That looks great @thewizarodofoz !
Sorry for still not yet taking it in, but today one improvement landed which I believe will vastly improve service configuration handling, and which allows us to simply schema as proposed here. Please see my comment
| type: 'array', | ||
| items: { | ||
| oneOf: [ | ||
| { type: 'integer', minimum: 100000000000, maximum: 999999999999 }, |
There was a problem hiding this comment.
Today I've turned on automatic coercion with #8319, and having that I believe we no longer need that number definition.
We can also remove the logic which tries to handle eventual numeric input
Closes: #8015