chore(assertions): release as an alpha module in CDKv2 by skinny85 · Pull Request #15614 · aws/aws-cdk

skinny85 · 2021-07-16T20:35:51Z

The @aws-cdk/assertions library was previously excluded from the V2 build of the experimental modules,
because the vendor.sh script it used for vendoring in a few modules
(cfnspec, cloudformation-diff, and assert-internal)
used paths relative to the V1 module inside the repository.

Change the vendor.sh script to short-circuit and exit when running in transform phase for
the alpha packages. That's required because the transform phase re-writes imports when copying
the TS files (because it needs to know which modules are also experimental to do the rewriting correctly),
but the vendor.sh script runs only after the build script for the package has been invoked,
which means any TS files copied by it successfully would not have their imports re-written.
Since the build for alpha packages runs after the build for @aws-cdk/assertions
(we add an explicit dependency to make sure that happens),
those files are already "vendored in" correctly, and will be copied when creating the separate package.

Also adjust the module copying logic to account for bundled dependencies,
which @aws-cdk/assertion uses.

Closes #15589

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2021-07-16T20:35:53Z

nija-at

All of this 'feels' super hacky. It feels too easy to introduce defects, unintended side effects, and worse of all, debugging is going to be a nightmare.

How about a different approach?

Move 'cloudformation-diff' into its own repo outside of the monorepo. It depends on a small part of 'cfnspec'. Without diving too deep into it, I think it should be possible to refactor this part of cfnspec into cloudformation-diff as well.

Then, copy over the files from 'assert-internal' into 'assertions' as part of the source tree instead of copying them in.

All that will remain to be copied over would the cloudformation spec from cfnspec which can then be passed as input to the 'cloudformation-diff' module.

added benefit: we'll get 'nozem' to work again.

cc @eladb @rix0rrr

packages/@aws-cdk/assertions/vendor-in.sh

skinny85 · 2021-07-19T15:55:22Z

@nija-at before I reply, help me understand something.

Your proposal is to actually change the way @aws-cdk/assertions is built completely, right? This is not only a change in V2, it would also apply to the V1 version of @aws-cdk/assertions - am I understanding you correctly?

nija-at · 2021-07-19T16:04:27Z

This is not only a change in V2, it would also apply to the V1 version of @aws-cdk/assertions - am I understanding you correctly?

Yes. Remove further v2 special casing.

skinny85 · 2021-07-19T16:52:02Z

This is not only a change in V2, it would also apply to the V1 version of @aws-cdk/assertions - am I understanding you correctly?

Yes. Remove further v2 special casing.

OK. Can we get this PR merged then? I'm fine if you want to change how @aws-cdk/assertions is built, but I'd rather not block V2 experimental modules on it.

nija-at · 2021-07-19T17:36:27Z

I'm not comfortable approving this change. We should not be looking at these independently.

This change is increasing the complexity of our build system, which is already overloaded and complicated.
All solutions must strive to simplify, otherwise we're headed towards it being unmanageable.

Given that this is a v2 branch only change, this moves the burden of debugging and resolution to the oncall or somewhere later from the PR that introduces any defect here.
For the reasons stated in my previous comment, the debugging of this is going to be hard, with all the special cases peppered around.

We need to find a solution that makes the overall system simpler.

nija-at · 2021-07-19T17:39:29Z

Two options that come to mind are - the approach I had mentioned above, or supporting dependency bundling within the monorepo so we don't have to vendor in.

Maybe there are others?

skinny85 · 2021-07-19T18:25:19Z

Not sure why we can't enable this for V2 experimental builds while working on improving the mess in @aws-cdk/assertions concurrently? Whatever change you want to do to the build of @aws-cdk/assertions, it will be a completely separate PR from this one, targeting the master branch. If you want to get rid of vendor.sh script, this means the workaround that I added here will be removed anyway, so I don't think introducing it temporarily is a big deal. This entire change in @aws-cdk/assertions is 4 lines of Bash.

I don't think making this PR wait for the PR with the build changes for @aws-cdk/assertions is a good idea. We are coupling things together that don't have to be coupled, and so we are slowing down the overall development speed of the project. In fact, you're basically saying that this PR will be blocked until a different PR, which has not yet even been opened, is merged. That means you're introducing a new dependency to the critical path of releasing the experimental modules project. That can't be a good thing.

Given that this is a v2 branch only change, this moves the burden of debugging and resolution to the oncall or somewhere later from the PR that introduces any defect here.
For the reasons stated in my previous comment, the debugging of this is going to be hard, with all the special cases peppered around.

This is already the case, whether we merge this PR or not. I take the responsibility for fixing these issues, not the oncall, like I already did in #15648.

rix0rrr · 2021-07-20T13:34:07Z

Since the build for @aws-cdk-lib runs after the build for @aws-cdk/assertions
(we add an explicit dependency to make sure that happens),
those files are already "vendored in" correctly,
and will be copied when creating the separate package.

This seems to suggest that it's fine to NOT run vendor.sh because everything will be okay already.

If so, then why is the fix not to do the following in the monorepo preparation script?

ln -sf /bin/true vendor.sh

?

packages/@aws-cdk/assertions/vendor-in.sh

skinny85 · 2021-07-20T15:39:12Z

Since the build for @aws-cdk-lib runs after the build for @aws-cdk/assertions
(we add an explicit dependency to make sure that happens),
those files are already "vendored in" correctly,
and will be copied when creating the separate package.

This seems to suggest that it's fine to NOT run vendor.sh because everything will be okay already.

If so, then why is the fix not to do the following in the monorepo preparation script?
ln -sf /bin/true vendor.sh 
?

It's a possible solution, but I'd rather keep these hacks locally to the packages, instead of putting them in the experimental packages generation. For example, if Niranjan's plan goes through, and the build of @aws-cdk/assertions is changed to no longer use the vendor.sh script, this hack will be naturally removed, while it would stay in indefinitely if it was part of the experimental packages build.

packages/@aws-cdk/assertions/vendor-in.sh

skinny85 · 2021-07-29T22:53:01Z

@nija-at I have rebased this PR on top of #15722. After #15722 has been merged, if you agree with the changes here, if you could change the base of this PR to be v2-main, and then approve it, it should apply cleanly. Thanks!

The `@aws-cdk/assertions` library was previously excluded from the V2 build of the experimental modules, because the `vendor.sh` script it used for vendoring in a few modules (`cfnspec`, `cloudformation-diff`, and `assert-internal`) used relative paths inside the repository. Change the script to use relative paths instead, and also adjust the module copying logic to account for bundled dependencies, which `@aws-cdk/assertion` uses.

nija-at · 2021-08-04T11:08:19Z

tools/pkglint/lib/rules.ts

+    // don't do this check for the separately-vended modules
+    if (pkg.packageName.startsWith('@aws-cdk-lib-alpha/')) {
+      return;
+    }


Why is this being skipped?

Huh, it's not clear. I removed this check and re-ran pkglint, and didn't see any differences in any of these packages. Perhaps a hold-out from an older iteration? I'll remove for now until the reason becomes clearer.

Huh, I must not have built and/or linted the right thing. Here's the build failure.

@aws-cdk-lib-alpha/assertions: In package package.json @aws-cdk-lib-alpha/assertions: - [yarn/nohoist-bundled-dependencies] Repository-level 'workspaces.nohoist' directive is missing: @aws-cdk-lib-alpha/assertions/colors, @aws-cdk-lib-alpha/assertions/colors/**, @aws-cdk-lib-alpha/assertions/diff, @aws-cdk-lib-alpha/assertions/diff/**, @aws-cdk-lib-alpha/assertions/fast-deep-equal, @aws-cdk-lib-alpha/assertions/fast-deep-equal/**, @aws-cdk-lib-alpha/assertions/string-width, @aws-cdk-lib-alpha/assertions/string-width/**, @aws-cdk-lib-alpha/assertions/table, @aws-cdk-lib-alpha/assertions/table/** (fixable)

I don't know enough to understand this. What is this rule meant to prevent and why we should not apply this to v2?

For example, an alternate solution could be to add this to the 'nohoist' flag here - https://github.com/aws/aws-cdk/blob/v2-main/packages/individual-packages/lerna.json.
I'm not saying that's the right solution, just that I don't understand enough of it.

Can you provide a little more context?

It's really not clear. I don't think I understand why Adam wanted to exclude the alpha packages from this rule.

The build is complaining that a bundled dependency (e.g., colors) is not added to the top-level nohoist config, which keeps those dependencies at the local package level rather than hoisting them up to the top-level workspace. We have colors as a nohoist listing three other times already (@aws-cdk/assertions/colors, aws-cdk-lib/colors, and monocdk/colors). Not sure why we wouldn't add the alpha packages in the same way.

I ran pkglint -f in the alpha assertions package, which added the relevant entries to the top-level package.json. This should solve the build error, and I think is the right thing to do.

Thanks for diving in. Sounds like the right approach, until we have info that says otherwise.

tools/individual-pkg-gen/copy-files-removing-deps.ts

…table-build # Conflicts: # tools/individual-pkg-gen/copy-files-removing-deps.ts

nija-at

Thanks for taking care of this @njlynch

I've updated the title to be cleaner. It looks the description is slightly stale (e.g., uses the term @aws-cdk-lib while we've changed to @aws-cdk-lib-alpha).

Would be good to clean up the description before merging.

aws-cdk-automation · 2021-08-06T16:12:34Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: baaa200
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2021-08-06T16:13:02Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

skinny85 requested a review from nija-at July 16, 2021 20:35

skinny85 self-assigned this Jul 16, 2021

mergify bot added the contribution/core This is a PR that came from AWS. label Jul 16, 2021

skinny85 linked an issue Jul 16, 2021 that may be closed by this pull request

Handle the @aws-cdk/assertions library in the V2 build of unstable modules #15589

Closed

skinny85 force-pushed the fix/assertions-unstable-build branch from 6057a72 to 7986f2e Compare July 16, 2021 23:10

nija-at suggested changes Jul 19, 2021

View reviewed changes

packages/@aws-cdk/assertions/vendor-in.sh Outdated Show resolved Hide resolved

skinny85 force-pushed the fix/assertions-unstable-build branch from 7986f2e to 38e2be0 Compare July 19, 2021 16:51

skinny85 requested a review from nija-at July 19, 2021 16:52

rix0rrr reviewed Jul 20, 2021

View reviewed changes

packages/@aws-cdk/assertions/vendor-in.sh Outdated Show resolved Hide resolved

skinny85 force-pushed the fix/assertions-unstable-build branch from 38e2be0 to a59f2d9 Compare July 20, 2021 15:34

skinny85 requested a review from rix0rrr July 20, 2021 15:40

skinny85 force-pushed the fix/assertions-unstable-build branch from a59f2d9 to 7eb25db Compare July 20, 2021 15:46

nija-at reviewed Jul 22, 2021

View reviewed changes

packages/@aws-cdk/assertions/vendor-in.sh Outdated Show resolved Hide resolved

skinny85 force-pushed the fix/assertions-unstable-build branch from 7eb25db to 5820f85 Compare July 29, 2021 22:49

skinny85 changed the base branch from v2-main to adamruka/move-v2-experiments-build-to-script July 29, 2021 22:50

skinny85 requested a review from nija-at July 29, 2021 23:11

skinny85 force-pushed the fix/assertions-unstable-build branch from 5820f85 to a67ebe0 Compare July 30, 2021 18:13

skinny85 changed the base branch from adamruka/move-v2-experiments-build-to-script to v2-main July 30, 2021 18:13

njlynch self-assigned this Aug 4, 2021

nija-at reviewed Aug 4, 2021

View reviewed changes

njlynch added 3 commits August 6, 2021 12:40

removing rule exclusion

5de9434

Merge remote-tracking branch 'origin/v2-main' into fix/assertions-uns…

19f5620

…table-build # Conflicts: # tools/individual-pkg-gen/copy-files-removing-deps.ts

ACTUALLY remove pkglint exclusion

5c84139

njlynch requested a review from nija-at August 6, 2021 11:47

add missing packages to nohoist config

baaa200

nija-at changed the title ~~chore: V2 version of the @aws-cdk/assertions library~~ chore(assertions): release as an alpha module in CDKv2 Aug 6, 2021

nija-at added the pr/do-not-merge This PR should not be merged at this time. label Aug 6, 2021

nija-at approved these changes Aug 6, 2021

View reviewed changes

njlynch removed the pr/do-not-merge This PR should not be merged at this time. label Aug 6, 2021

mergify bot merged commit dd2d286 into aws:v2-main Aug 6, 2021

skinny85 deleted the fix/assertions-unstable-build branch August 24, 2021 22:20

Conversation

skinny85 commented Jul 16, 2021 • edited by njlynch Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitpod-io bot commented Jul 16, 2021

Uh oh!

nija-at left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skinny85 commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nija-at commented Jul 19, 2021

Uh oh!

skinny85 commented Jul 19, 2021

Uh oh!

nija-at commented Jul 19, 2021

Uh oh!

nija-at commented Jul 19, 2021

Uh oh!

skinny85 commented Jul 19, 2021

Uh oh!

rix0rrr commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

skinny85 commented Jul 20, 2021

Uh oh!

Uh oh!

skinny85 commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nija-at Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

njlynch Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

njlynch Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

nija-at Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

njlynch Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

nija-at Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nija-at left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Aug 6, 2021

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

skinny85 commented Jul 16, 2021 •

edited by njlynch

Loading

nija-at left a comment •

edited

Loading

skinny85 commented Jul 19, 2021 •

edited

Loading

rix0rrr commented Jul 20, 2021 •

edited

Loading

skinny85 commented Jul 29, 2021 •

edited

Loading

nija-at left a comment •

edited

Loading