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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lib/configSchema.js
Outdated
oneOf: [ | ||
{ type: 'integer', minimum: 100000000000, maximum: 999999999999 }, | ||
{ type: 'string', pattern: '^\\d{12}$' }, | ||
{ const: '*' }, |
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.
I would remove it and rely on pattern as in AWS docs
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Thank you @thewizarodofoz ! That looks great
Closes: #8015