-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
feat(aws): schema for resources
when using AWS provider
#8139
Conversation
7d75f00
to
dc0d2be
Compare
Force pushed to fix broken unit test that was providing invalid resources for an AWS stack. |
Codecov Report
@@ Coverage Diff @@
## master #8139 +/- ##
=======================================
Coverage 88.21% 88.21%
=======================================
Files 248 248
Lines 9460 9460
=======================================
Hits 8345 8345
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 @glb it looks really good. It was a good call to extend defineProvider
to setup this definition.
I've proposed some other improvements, see my comments
@@ -170,6 +170,11 @@ class ConfigSchemaHandler { | |||
} | |||
} | |||
|
|||
// allow provider to override schema for `resources`. |
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.
Let's remove this comment (I think code is self explanatory on that)
@@ -170,6 +170,11 @@ class ConfigSchemaHandler { | |||
} | |||
} | |||
|
|||
// allow provider to override schema for `resources`. | |||
if (options.resources) { | |||
this.schema.properties.resources = options.resources; |
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 we should also remove it's definition from core configSchema.js
here:
serverless/lib/configSchema.js
Lines 36 to 37 in 5f85543
// TODO: Complete schema, see https://github.com/serverless/serverless/issues/8014 | |
resources: { type: 'object' }, |
Idea is that what's there is can be subject to extension and not to replacement, and here we're replacing the schema, which may lead to errors (e.g. if at some point something is added to resources
at configSchema.js
, it'll be ineffective.
We also should remove this line:
Object.freeze(this.schema.properties.resources); |
properties: { | ||
Type: { type: 'string' }, | ||
}, | ||
required: ['Type'], |
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.
Currently resources.Resources
serve as an extension object, through which resources as created by the Framework can be extended, in such cases users do not provide Type
, and we should not require it
'^[a-zA-Z0-9]{1,255}$': { | ||
type: 'object', | ||
properties: { | ||
Type: { type: 'string' }, |
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 probably be good to add definition for Properties
(just `type: 'object') and all properties listed here: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-product-attribute-reference.html (just sanity checks).
Ideally if we define all those properties (aside of Type
) as reusable definition. For that I'd add support for provider to define its own definitions here:
So they're added to this object:
serverless/lib/configSchema.js
Line 116 in 5f85543
definitions: { |
With this move I would also define awsArn
and awsCfImport
in scope of defineProvider
call (move out from core schema).
Having that I would reuse this definition here, in form:
{
properties: { $ref: '#/definitions/awsResource' },
additionalProperties: {
Type: { type: "string: }
}
and reuse it also in context of resources.extensions
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've put in a first attempt at a smoke-check definition for Properties
, CreationPolicy
, DeletionPolicy
, DependsOn
, Metadata
, UpdatePolicy
, and UpdateReplacePolicy
. I'm not sure I understand why you want these to be reusable definitions?
Note that the supported properties for resources.extensions
are different from the supported properties for resources
, so we can't re-use the type definition for resources
and resources.extensions
. Type
is explicitly not supported for resources.extensions
(see #7352 and specifically this switch statement).
I can look into adding support for extending definitions
and moving awsArn
and awsCfImport
.
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.
Note that the supported properties for resources.extensions are different from the supported properties for resources,
Both resources.Resources
and resource.extensions
extend CloudFormation resources, why do you assume that supported properties are different?
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 resources.Resources
allows you to add new resources or extend existing resources; resource.extensions
explicitly only allows you to extend existing resources. When I wrote #7352 at your request, the implementation intentionally disallowed specifying Type
in resources.extensions
for this reason.
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.
Yes, afaik the only difference between them is that Type
is not supported in context of resources.extensions
.
That's also the reason why I suggested to exclude Type
in generalization. Afaik generalization as I proposed can work well for both, or do I miss something?
// | ||
// The only required attribute is `Type`; `Properties` and other common attributes are optional. | ||
// See also https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/resources-section-structure.html | ||
Resources: { |
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.
Let's provide definitions for all possible CF template properties (listed here: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/template-anatomy.html ) but ofc without going into deep details, it should serve more as sanity check
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.
To clarify: you're just looking for a top-level check on AWSTemplateFormatVersion
, Description
, Metadata
, Parameters
, Mappings
, Conditions
, Transform
, and Outputs
in addition to the existing schema for Resources
here?
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.
Yes, exactly, as all of them are supported by Framework
DeletionPolicy: { not: { type: 'null' } }, | ||
DependsOn: { type: 'array', items: { not: { type: 'null' } } }, | ||
Metadata: { type: 'object' }, | ||
UpdatePolicy: { type: 'object' }, | ||
UpdateReplacePolicy: { not: { type: 'null' } }, |
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.
DeletionPolicy
and UpdateReplacePolicy
are defined as string
, but I'm not sure if you're allowed to reference parameters for them, so I'm being really permissive here. null
is definitely not a correct value, so will flag that.
I suppose another option would be { oneOf: [{type: 'object'},{type: 'string'}] }
. Thoughts?
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 assume only string
type is supported in them. It's very meta property (as e.g. Type
, afaik, you cannot define it by referencing some other resource).
At least documentation doesn't list any other configuration options: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-attribute-deletionpolicy.html
I'd stick to string
(unless we have some proof it can work with other type of value)
@medikoo I've pushed changes that I think address almost all of your suggestions except for the shared definition for resources (I haven't wrapped my head around it yet). There are some failing unit tests that I don't yet understand; will see if I can dig into them tomorrow. Any pointers would be appreciated. |
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.
@glb Great thanks, I see we're getting close! 👍
There are some failing unit tests that I don't yet understand
I've looked into it, and apparently there was a bug in @serverless/enterprise-plugin
which unconditionally provided definitions that apply only to AWS provider, and moving AWS specific definitions to AWS provider scope, made it crash when we dealt with non-AWS provider service.
It's fixed with serverless/dashboard-plugin@79e5535 and published with v3.8.1
When going through that I've also approached other quirks, therefore I've prepared a PR on my own where I move definitions
and resources
context, in a way, where all exposed issues are addressed.
PR (#8144) is merged now, so simply rebase against master
. Sorry for conflicts it'll cause! Still now in this PR we can focus purely on resources
schema with no side cleanup tasks
Properties: { type: 'object' }, | ||
CreationPolicy: { type: 'object' }, | ||
DeletionPolicy: { type: 'string' }, | ||
DependsOn: { type: 'array', items: { not: { type: 'null' } } }, |
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 all items here should be typeof string
, it seems clearly indicated in AWS docs: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-attribute-dependson.html
6e8d22f
to
868f7b5
Compare
@medikoo I've rebased and squashed everything down to the schema change and the unit test fix, and I've re-run a manual test to confirm it catches the issue reported in #8118 (but the message is still not super-helpful). Serverless: Configuration warning at 'resources.Resources': should be object I think I've addressed all of your comments, does it look OK now? |
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.
@glb that looks great! We're near there (see my two comments)
it catches the issue reported in #8118 (but the message is still not super-helpful).
Yes, still having a signal that config is invalid is already a great improvement. Concerning the message I believe that what I proposed here: #8118 (comment) will solve it in very clean way (and you'll end with a clear message indicating where exactly is the error).
Note that what we deal with in #8118 is (1) an error in variables resolution that (2) results with an invalid config. We've solved here (2) reporting of fact that resolved config is invalid. While (1) an error of variables resolution is other matter that I'd coin separately.
Properties: { type: 'object' }, | ||
CreationPolicy: { type: 'object' }, | ||
DeletionPolicy: { type: 'string' }, | ||
DependsOn: { type: 'array', items: { not: { type: 'null' } } }, |
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.
We should allow just string in DependsOn
Having this secluded to definition (as proposed here) will ensure that definition in both cases are matched, and eventual errors are fixed for both locations.
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've fixed the DependsOn
so that it is an array of strings. Unfortunately I'm not as adept with JSON Schema as I'd like to be so I'm not 100% sure what the definition should look like for this -- your proposal showed what the uses might look like but not the definition itself.
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 I've used $ref
properly ... at least it seems to work.
type: 'object', | ||
}, | ||
}, | ||
}, |
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.
We need additionalProperties: false
here
53d19a3
to
a1ae7cd
Compare
a1ae7cd
to
a9254a9
Compare
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 @glb ! 👍
Adds schema for the
resources
top-level element when using the AWS provider.Does not add tests as directed in #8051. I've run a small manual test.
Addresses: #8118 (flags input issue but error could be better if validation was done before
mergeArrays
, see comment)Closes: #8014 (does not cover non-AWS providers)