[BREAKING] Move `@aws-cdk/resources` classes into L2 packages #264

RomainMuller · 2018-07-09T15:36:11Z

The create_missing_libraries.sh script was used to bootstrap L2
packages for all the namespaces that didn't have one already.

Existing L2 packages were manually edited to:

Invoke cfn2ts to generate the L1 class library for the service/scope
Depend on @aws-cdk/runtime (which used to be blended in
@aws-cdk/resources, and was extracted out)
Export the L1 library as expected

The @aws-cdk/assert behavior was changed to rely on
@aws-cdk/cloudformation-resource-spec to determine the UpdateType of
attributes, rendering the registry mechanism that was introduced to
@aws-cdk/resources a while ago redundant (hence it got dropped).

Bonus: Renamed the prepare command in all package.json to build (Fixes #197).

The `create_missing_libraries.sh` script was used to bootstrap L2 packages for all the namespaces that didn't have one already. Existing L2 packages were manually edited to: - Invoke `cfn2ts` to generate the L1 class library for the service/scope - Depend on `@aws-cdk/runtime` (which used to be blended in `@aws-cdk/resources`, and was extracted out) - Export the L1 library as expected The `@aws-cdk/assert` behavior was changed to rely on `@aws-cdk/cloudformation-resource-spec` to determine the `UpdateType` of attributes, rendering the `registry` mechanism that was introduced to `@aws-cdk/resources` a while ago redundant (hence it got dropped).

rix0rrr · 2018-07-09T15:43:04Z

Can we merge @aws-cdk/runtime back into each package? I'd prefer minimizing dependencies, and it's not huge. (We do need to check that it doesn't use instanceof anywhere because that's definitely going to break)

rix0rrr · 2018-07-09T15:44:46Z

Also, if you're asking me, this PR is trying to do too many things at once. But I understand it's going to be a bitch trying to factor those out, so never mind.

rix0rrr · 2018-07-09T15:48:13Z

I thought the plan was to put CFN resources into a cloudformation namespace? Is that off the table?

Although I understand that might complicate export of the property types.

RomainMuller · 2018-07-09T15:50:15Z

I agree with there being lots in this PR, but yeas, isolating each individual change is an order of magnitude more work to just build & un-build stuff so it si buildable & all... I know it sucks.

Regarding @aws-cdk/runtime, I don't like the idea of duplicating the classes everywhere. Parts of what's interesting in isolating in own package is that said package comes with it's own test suite, which previously was blended with @aws-cdk/resources (when the code being tested was defined in cfn2ts in the form of a template fragment).

Regarding the cloudformation namespace, it can be done... Would have to create a namespace specifically for the Resource classes, which may make the code generation logic more complex (but nothing impossible).

rix0rrr · 2018-07-09T15:43:53Z

build.sh

-echo "boostrapping..."
-lerna bootstrap --reject-cycles
+echo "building..."
+lerna exec --stream "npm run build"


Will this build symlinks?

Nope, lerna bootstrap will have done that at the install.sh step. I can duplicate the bootstrap command, though if you feel it's better... It's incredibly cheap to run.

We should automatically run ./install.sh is node_modules doesn't exist.

We already do (L4-6 in build.sh).

eladb

🥇

Please generate all resource types into the cloudformation namespace.

eladb · 2018-07-10T07:54:20Z

build.sh

-echo "boostrapping..."
-lerna bootstrap --reject-cycles
+echo "building..."
+lerna exec --stream "npm run build"


We should automatically run ./install.sh is node_modules doesn't exist.

eladb · 2018-07-10T07:54:49Z

create_missing_libraries.sh

@@ -0,0 +1,97 @@
+#!/bin/bash


Use hyphens in file names create-missing-libraries.sh

eladb · 2018-07-10T07:59:38Z

create_missing_libraries.sh

+        echo "⏳ Creating package ${P} for scope ${S}..."
+        mkdir -p packages/${P}/test
+        mkdir -p packages/${P}/lib
+        echo '*.js' >> packages/${P}/.gitignore


Can you please use HEREDOCs instead? Will be wayyyy more readable...

(I find the fact that HEREDOCs require you to break indentation very disturbing, BTW).

I know. Me too, but they are still way better than this 😜

eladb · 2018-07-10T08:02:23Z

packages/@aws-cdk/acm/lib/index.ts

@@ -1,2 +1,5 @@
 export * from './certificate';
 export * from './certificate-ref';
+
+// The L1 library for AWS::CertificateManager:


Let's use the terminology AWS::CertificateManager CloudFormation resources

eladb · 2018-07-10T08:04:00Z

packages/@aws-cdk/apigateway/lib/index.ts

@@ -0,0 +1,2 @@
+// The L1 Library for AWS::ApiGateway:
+export * from '../cfn/apigateway';


I think the generated code should go under lib/cloudformation/

Also, make sure cfn2ts creates a README file under the generated directory that says that it is generated and from which spec date

That information is already included in the generated file itself... Do we really need it repeated?

No, you are right.

eladb · 2018-07-10T08:09:08Z

tools/cfn2ts/lib/codegen.ts

        this.code.openFile(this.outputFile);

+        const meta = {
+            generatedOn: new Date(),


so java.... I would go with generated and fingerprint

eladb · 2018-07-10T08:11:01Z

tools/cfn2ts/lib/codegen.ts

+                const prop = spec.Properties[pname];
+                if (prop.Required) {
+                    const propName = pname.toLocaleLowerCase() === 'name' ? resourceNameProperty(resourceName.specName!) : pname;
+                    this.code.line(`this.required(properties, '${genspec.cloudFormationToScriptName(propName)}');`);


I was just about to deprecate construct.required. Can you just add this to runtime?

eladb · 2018-07-10T08:13:00Z

tools/cfn2ts/lib/index.ts


-        generator.addSpec(spec);
+    if (force || !await generator.upToDate(outPath)) {


style: prefer early bailout: if (!force && await upToDate) { return; }

eladb · 2018-07-10T08:14:32Z

tools/cfn2ts/package.json

@@ -25,6 +25,7 @@
  },
  "license": "LicenseRef-LICENSE",
  "dependencies": {
+    "@aws-cdk/cloudformation-resource-spec": "^0.7.3-beta",


I'd call this package @aws-cdk/cloudformation-spec or even @aws-cdk/cfnspec. Keep 'em short!

I wish this was said on the two occasions I asked for naming suggestions 🤨

Renaming to @aws-cdk/cdk-cfnspec.

Cool. Are we doing the cdk-xxx as part of this change, or just this one for now?

Just this one. This changeset is already bloated enough I think :)

👍 I was worried that you are crazier than I thought

eladb · 2018-07-10T08:16:04Z

packages/@aws-cdk/certificatemanager/package.json

+  },
+  "dependencies": {
+    "@aws-cdk/core": "^0.7.3-beta",
+    "@aws-cdk/runtime": "^0.7.3-beta"


Can we just merge runtime into core? I agree with @rix0rrr - doesn't seem like a useful module. And also, the name is way too presumptuous for what it's doing 😉

✅ Much prefer this than duplicating it into every generated package.

- Namespace resource and property classes under 'cloudformation' - Merge @aws-cdk/runtime into @aws-cdk/core - Rename @aws-cdk/clouformation-resource-spec to @aws-cdk/cdk-cfnspec - Generate code into lib/<ns>.generated.ts

Disabling use of '--reject-cycles' on 'lerna bootstrap' as it would blow up on the cycle created by (e.g:) how aws-cdk uses @aws-cdk/s3, which itself uses @aws-cdk/assert.

…or generated code.

RomainMuller force-pushed the rmuller/split-resources branch from 86a4a83 to dcd89bc Compare July 9, 2018 15:38

rix0rrr approved these changes Jul 9, 2018

View reviewed changes

eladb reviewed Jul 10, 2018

View reviewed changes

RomainMuller added 3 commits July 10, 2018 15:19

PR Feedback incorporation:

bb7c332

- Namespace resource and property classes under 'cloudformation' - Merge @aws-cdk/runtime into @aws-cdk/core - Rename @aws-cdk/clouformation-resource-spec to @aws-cdk/cdk-cfnspec - Generate code into lib/<ns>.generated.ts

Modelling dependency from @aws-cdk/assert on aws-cdk

1ba5f7f

Disabling use of '--reject-cycles' on 'lerna bootstrap' as it would blow up on the cycle created by (e.g:) how aws-cdk uses @aws-cdk/s3, which itself uses @aws-cdk/assert.

Add comment

27dad9a

RomainMuller force-pushed the rmuller/split-resources branch from 744394f to 27dad9a Compare July 10, 2018 13:43

RomainMuller added 2 commits July 10, 2018 15:51

Make @aws-cdk/assert resolve the cdk command correctly

e46f5f8

Change import patterns to use per-class import instead of wild-card f…

826dc09

…or generated code.

RomainMuller merged commit aa404a0 into master Jul 10, 2018

RomainMuller deleted the rmuller/split-resources branch July 10, 2018 15:06

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

pahud mentioned this pull request Nov 19, 2019

[aws-eks] Managed NodeGroups #5086

Closed

2 tasks

		@@ -0,0 +1,2 @@
		// The L1 Library for AWS::ApiGateway:
		export * from '../cfn/apigateway';


		generator.addSpec(spec);
		if (force \|\| !await generator.upToDate(outPath)) {

[BREAKING] Move @aws-cdk/resources classes into L2 packages #264

[BREAKING] Move @aws-cdk/resources classes into L2 packages #264

Conversation

RomainMuller commented Jul 9, 2018

rix0rrr commented Jul 9, 2018

rix0rrr commented Jul 9, 2018

rix0rrr commented Jul 9, 2018 • edited Loading

RomainMuller commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[BREAKING] Move `@aws-cdk/resources` classes into L2 packages #264

[BREAKING] Move `@aws-cdk/resources` classes into L2 packages #264

rix0rrr commented Jul 9, 2018 •

edited

Loading