feat(aws): schema for `resources` when using AWS provider by glb · Pull Request #8139 · serverless/serverless

glb · 2020-08-26T19:07:52Z

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)

glb · 2020-08-26T19:30:10Z

Force pushed to fix broken unit test that was providing invalid resources for an AWS stack.

codecov-commenter · 2020-08-26T19:31:17Z

Codecov Report

Merging #8139 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #8139   +/-   ##
=======================================
  Coverage   88.21%   88.21%           
=======================================
  Files         248      248           
  Lines        9460     9460           
=======================================
  Hits         8345     8345           
  Misses       1115     1115

Impacted Files	Coverage Δ
lib/plugins/aws/provider/awsProvider.js	`93.11% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 149bf27...a9254a9. Read the comment docs.

medikoo

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

medikoo · 2020-08-27T10:15:54Z

      }
    }

+    // allow provider to override schema for `resources`.


Let's remove this comment (I think code is self explanatory on that)

medikoo · 2020-08-27T10:21:27Z


+    // allow provider to override schema for `resources`.
+    if (options.resources) {
+      this.schema.properties.resources = options.resources;


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:

serverless/lib/classes/ConfigSchemaHandler/index.js

Line 35 in 5f85543

Object.freeze(this.schema.properties.resources);

medikoo · 2020-08-27T10:24:00Z

+                  properties: {
+                    Type: { type: 'string' },
+                  },
+                  required: ['Type'],


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

medikoo · 2020-08-27T10:24:47Z

+                '^[a-zA-Z0-9]{1,255}$': {
+                  type: 'object',
+                  properties: {
+                    Type: { type: 'string' },


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:

serverless/lib/classes/ConfigSchemaHandler/index.js

Line 172 in 5f85543

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

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.

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?

@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.

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?

medikoo · 2020-08-27T10:27:07Z

+            //
+            // 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: {


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

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?

Yes, exactly, as all of them are supported by Framework

glb · 2020-08-27T15:08:22Z

+                    DeletionPolicy: { not: { type: 'null' } },
+                    DependsOn: { type: 'array', items: { not: { type: 'null' } } },
+                    Metadata: { type: 'object' },
+                    UpdatePolicy: { type: 'object' },
+                    UpdateReplacePolicy: { not: { type: 'null' } },


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?

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)

glb · 2020-08-27T21:17:20Z

@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.

medikoo

@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 https://github.com/serverless/enterprise-plugin/commit/79e55353251fdd08dc1067d27e685962a1c3432e 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

medikoo · 2020-08-28T10:08:29Z

+                    Properties: { type: 'object' },
+                    CreationPolicy: { type: 'object' },
+                    DeletionPolicy: { type: 'string' },
+                    DependsOn: { type: 'array', items: { not: { type: 'null' } } },


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

glb · 2020-08-28T15:54:06Z

@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?

medikoo

@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.

medikoo · 2020-08-31T08:28:10Z

+                    Properties: { type: 'object' },
+                    CreationPolicy: { type: 'object' },
+                    DeletionPolicy: { type: 'string' },
+                    DependsOn: { type: 'array', items: { not: { type: 'null' } } },


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.

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.

I think I've used $ref properly ... at least it seems to work.

medikoo · 2020-08-31T08:28:55Z

+              type: 'object',
+            },
+          },
+        },


We need additionalProperties: false here

medikoo

Thank you @glb ! 👍

glb force-pushed the 8014-resources-schema branch from 7d75f00 to dc0d2be Compare August 26, 2020 19:29

medikoo added cat/config-schema enhancement labels Aug 27, 2020

medikoo suggested changes Aug 27, 2020

View reviewed changes

medikoo assigned glb Aug 27, 2020

glb commented Aug 27, 2020

View reviewed changes

medikoo reviewed Aug 28, 2020

View reviewed changes

glb force-pushed the 8014-resources-schema branch 2 times, most recently from 6e8d22f to 868f7b5 Compare August 28, 2020 15:48

medikoo suggested changes Aug 31, 2020

View reviewed changes

glb force-pushed the 8014-resources-schema branch 2 times, most recently from 53d19a3 to a1ae7cd Compare September 4, 2020 18:53

feat(aws): schema for resources when using AWS provider

a9254a9

glb force-pushed the 8014-resources-schema branch from a1ae7cd to a9254a9 Compare September 4, 2020 18:58

glb requested a review from medikoo September 4, 2020 19:05

medikoo approved these changes Sep 7, 2020

View reviewed changes

medikoo merged commit 00d6f79 into serverless:master Sep 7, 2020

medikoo pushed a commit that referenced this pull request Sep 9, 2020

feat(Config Schema): Schema for AWS resources section (#8139)

f8265b9

glb mentioned this pull request Sep 9, 2020

Support Condition attribute on resources #8208

Closed

glb deleted the 8014-resources-schema branch September 11, 2020 17:26

	// TODO: Complete schema, see https://github.com/serverless/serverless/issues/8014
	resources: { type: 'object' },

Uh oh!

Conversation

glb commented Aug 26, 2020 • edited by medikoo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glb commented Aug 26, 2020

Uh oh!

codecov-commenter commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glb commented Aug 27, 2020

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glb commented Aug 28, 2020

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

glb commented Aug 26, 2020 •

edited by medikoo

Loading

codecov-commenter commented Aug 26, 2020 •

edited

Loading