test(all): move e2e tests to cdk v2 #676

flochaz · 2022-03-21T21:14:08Z

Description of your changes

Move all end to end test to cdk v2.

Details

bump cdk deps to 2.17.0
replace all @aws-cdk deps to aws-cdk-lib
Use @Tanemahuta implementation suggested here feat(cli): bundle dependencies aws/aws-cdk#18667 (comment) which simply rebuild the stackArtifact with the proper type.

[OUTDATED] Details

Thanks to @3p3r work and especially this line https://github.com/3p3r/cdk-web/blob/main/cdk-web.webpack.config.js#L132 I've been able to isolate the needed work around to aws/aws-cdk#18211 issue by duplicating CloudFormationDeployments class and rewrite isAssetManifestArtifact function to

function isAssetManifestArtifact(art: cxapi.CloudArtifact): art is cxapi.AssetManifestArtifact {
  return Object.keys(art).includes('file');
}

How to verify this change

https://github.com/awslabs/aws-lambda-powertools-typescript/actions/runs/2026670475

Related issues, RFCs

aws/aws-cdk#18211

PR status

Is this ready for review?: NO
Is it a breaking change?: NO

Checklist

Breaking change checklist

I have documented the migration process
I have added, implemented necessary warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

dreamorosi

This is great @flochaz, super excited about this PR. It has been a long time coming :)

saragerion

Thanks Florian for the PR!

saragerion · 2022-03-22T08:13:26Z

packages/commons/tests/utils/cloudformation-deployments.ts

+/* eslint-disable @typescript-eslint/ban-ts-comment */
+/* eslint-disable @typescript-eslint/explicit-function-return-type */
+/* eslint-disable @typescript-eslint/member-ordering */
+/* eslint-disable @typescript-eslint/explicit-member-accessibility */
+/* eslint-disable @typescript-eslint/member-delimiter-style */
+/* eslint-disable newline-before-return */
+/* eslint-disable func-style */


Suggestion: rather than adding this list of linting exceptions what about addressing opportunities for improving our linting rules in a separated space/issue?

Interesting article:
https://stackoverflow.blog/2020/07/20/linters-arent-in-your-way-theyre-on-your-side/

I went through these rules and I think they are fine as it is for global level.

This file is only exceptional since it's duplicated from an other project. I prefer to just disable lint in this file and keep it exactly as it is for ease of maintenance.

saragerion · 2022-03-22T08:14:53Z

packages/commons/tests/utils/cloudformation-deployments.ts

@@ -0,0 +1,223 @@
+// This file is duplicating CloudFormationDeployments define in https://github.com/aws/aws-cdk/blob/master/packages/aws-cdk/lib/api/cloudformation-deployments.ts


Minor: since a branch version's may mutate, referring to a commit rather than a branch will make sure the link will work in the future

Link:
https://github.com/aws/aws-cdk/blob/2ea9da118794809265d215e3d2f554bbcb91b271/packages/aws-cdk/lib/api/cloudformation-deployments.ts

I assume that there is no change here? Do we do this to expose a private class?

saragerion · 2022-03-22T08:17:24Z

packages/commons/tests/utils/cloudformation-deployments.ts

@@ -0,0 +1,223 @@
+// This file is duplicating CloudFormationDeployments define in https://github.com/aws/aws-cdk/blob/master/packages/aws-cdk/lib/api/cloudformation-deployments.ts


Nit:

Suggested change

// This file is duplicating CloudFormationDeployments define in https://github.com/aws/aws-cdk/blob/master/packages/aws-cdk/lib/api/cloudformation-deployments.ts

// This file is duplicating CloudFormationDeployments defined in https://github.com/aws/aws-cdk/blob/master/packages/aws-cdk/lib/api/cloudformation-deployments.ts

saragerion · 2022-03-22T08:23:02Z

packages/commons/tsconfig.json

@@ -12,6 +12,7 @@
        "inlineSourceMap": true,
        "moduleResolution": "node",
        "resolveJsonModule": true,
+        "useUnknownInCatchVariables": false,


According to the documentation:

This pattern ensures that error handling code becomes more comprehensive because you cannot guarantee that the object being thrown is a Error subclass ahead of time.

What is the benefit of this approach instead of explicitly checking the types of the Errors? Example:

try { // something } catch (e: unknown) { if (err instanceof Error) { // something } }

ijemmy

Apart from Sara's comment, I'm ok with this change.

Duplicating a class to make it public concerns me on the maintenance effort.

But I think that this is a better option than calling subprocess.run('cdk'). We need to keep an eye on this and see how often it breaks. (vs. the effort to implement a robust call to cdk)

ijemmy · 2022-03-22T16:12:03Z

packages/commons/tests/utils/cloudformation-deployments.ts

@@ -0,0 +1,223 @@
+// This file is duplicating CloudFormationDeployments define in https://github.com/aws/aws-cdk/blob/master/packages/aws-cdk/lib/api/cloudformation-deployments.ts


I assume that there is no change here? Do we do this to expose a private class?

ijemmy · 2022-03-22T16:19:12Z

packages/commons/tests/utils/cloudformation-deployments.ts

+/* eslint-disable @typescript-eslint/ban-ts-comment */
+/* eslint-disable @typescript-eslint/explicit-function-return-type */
+/* eslint-disable @typescript-eslint/member-ordering */
+/* eslint-disable @typescript-eslint/explicit-member-accessibility */
+/* eslint-disable @typescript-eslint/member-delimiter-style */
+/* eslint-disable newline-before-return */
+/* eslint-disable func-style */


I went through these rules and I think they are fine as it is for global level.

This file is only exceptional since it's duplicated from an other project. I prefer to just disable lint in this file and keep it exactly as it is for ease of maintenance.

flochaz · 2022-03-22T20:09:33Z

Moving to @Tanemahuta implementation suggested here aws/aws-cdk#18667 (comment) ... so much easier

ijemmy

This is much nicer than the other version. The abstraction is very beautiful.

From what I see, the only risk is that the cx-api is changed (for synthesizing the stack artifact).
I doubt if they’ll change CloudFormationDeployments , SdkProvider , and DeployStackResult soon.

dreamorosi · 2022-03-23T09:15:14Z

package.json

@@ -59,9 +59,9 @@
    "@typescript-eslint/eslint-plugin": "^5.12.1",
    "@typescript-eslint/parser": "^5.12.1",
    "archiver": "^5.3.0",


Is this still needed? I think it's used by cdk to create the asset archives but just checking.

indeed might not be needed. will merge that one to let @ijemmy work and give it a try through another PR

github-actions bot added the internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) label Mar 21, 2022

flochaz force-pushed the chazalf/e2eTestCDKV2 branch 2 times, most recently from 9ad3353 to 8e71980 Compare March 22, 2022 06:21

flochaz changed the title ~~chore(all): move e2e tests to cdk v2~~ test(all): move e2e tests to cdk v2 Mar 22, 2022

dreamorosi assigned flochaz Mar 22, 2022

dreamorosi self-requested a review March 22, 2022 14:04

dreamorosi added this to Pull Requests - Work in progress in Pull Requests via automation Mar 22, 2022

dreamorosi added this to the production-ready-release milestone Mar 22, 2022

dreamorosi requested a review from saragerion March 22, 2022 14:04

dreamorosi previously approved these changes Mar 22, 2022

View reviewed changes

saragerion reviewed Mar 22, 2022

View reviewed changes

ijemmy reviewed Mar 22, 2022

View reviewed changes

flochaz dismissed dreamorosi’s stale review via 17847e1 March 22, 2022 20:17

flochaz force-pushed the chazalf/e2eTestCDKV2 branch from 8e71980 to 17847e1 Compare March 22, 2022 20:17

Pull Requests automation moved this from Pull Requests - Work in progress to Pull Requests - Review needed Mar 22, 2022

test: Move e2e test to cdk V2

1e1bcbc

flochaz force-pushed the chazalf/e2eTestCDKV2 branch from 17847e1 to 1e1bcbc Compare March 22, 2022 20:38

fix missing await

90790bb

dreamorosi self-requested a review March 23, 2022 09:12

ijemmy approved these changes Mar 23, 2022

View reviewed changes

dreamorosi approved these changes Mar 23, 2022

View reviewed changes

Pull Requests automation moved this from Pull Requests - Review needed to Pull Requests - Approved and ready to be merged Mar 23, 2022

ijemmy mentioned this pull request Mar 23, 2022

test(common,logger,metrics): run e2e tests for metrics in both Node runtimes #678

Merged

13 tasks

flochaz merged commit e2a9132 into main Mar 23, 2022

flochaz deleted the chazalf/e2eTestCDKV2 branch March 23, 2022 09:41

Pull Requests automation moved this from Pull Requests - Approved and ready to be merged to Pull Requests - Merged or Closed Mar 23, 2022

dreamorosi mentioned this pull request Mar 23, 2022

feat: added captureHTTPsRequest feature #677

Merged

11 tasks

dreamorosi mentioned this pull request Apr 14, 2022

Maintenance: ignore available updates for CDK v2 and reduce frequency of new PRs #672

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(all): move e2e tests to cdk v2 #676

test(all): move e2e tests to cdk v2 #676

flochaz commented Mar 21, 2022 •

edited

dreamorosi left a comment

saragerion left a comment

saragerion Mar 22, 2022

ijemmy Mar 22, 2022

saragerion Mar 22, 2022

dreamorosi Mar 22, 2022

ijemmy Mar 22, 2022

saragerion Mar 22, 2022

saragerion Mar 22, 2022

ijemmy left a comment

ijemmy Mar 22, 2022

ijemmy Mar 22, 2022

flochaz commented Mar 22, 2022 •

edited

ijemmy left a comment

dreamorosi Mar 23, 2022

flochaz Mar 23, 2022

		@@ -0,0 +1,223 @@
		// This file is duplicating CloudFormationDeployments define in https://github.com/aws/aws-cdk/blob/master/packages/aws-cdk/lib/api/cloudformation-deployments.ts

test(all): move e2e tests to cdk v2 #676

test(all): move e2e tests to cdk v2 #676

Conversation

flochaz commented Mar 21, 2022 • edited

Description of your changes

Details

[OUTDATED] Details

How to verify this change

Related issues, RFCs

PR status

Checklist

Breaking change checklist

dreamorosi left a comment

Choose a reason for hiding this comment

saragerion 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

ijemmy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flochaz commented Mar 22, 2022 • edited

ijemmy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flochaz commented Mar 21, 2022 •

edited

flochaz commented Mar 22, 2022 •

edited