-
Notifications
You must be signed in to change notification settings - Fork 4.3k
chore: un-jsii and bundle @aws-cdk/yaml-cfn
#13933
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
Conversation
The `yaml-cfn` library is used as an implementation detail of `cloudformation-include`, `aws-cdk`, `decdk`, and the monopackages. Un-jsii it and bundle it into the framework packages that need it. Does not need to be accessed over jsii and does not need to be exposed in the monopackages.
The `yaml-cfn` library is used as an implementation detail of `cloudformation-include`, `aws-cdk`, `decdk`, and the monopackages. Un-jsii it and bundle it into the framework packages that need it. Does not need to be accessed over jsii and does not need to be exposed in the monopackages. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
eladb
left a comment
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.
Something feels a bit off here. yaml-cfn is 60 lines of code. Does it really make sense to add all these hacks just to be able to reuse these 60 lines in two places in our code?
Did you consider the following options:
- Get rid of this module and copy & paste (rarely the right choice, but perhaps that's the rare case where this is a reasonable solution).
- Publish this module as a separate library from outside our mono-repo and use it like a normal module so we don't need all this hackery.
|
What's feeling off is the wavering tower of cards that is the infrastructure we need to build upon. I would prefer to shore up said tower of cards, rather than accept anything can just fall over at any time and just living with that. Could we copy/paste? Yes we could. As you said, I'd rather not. Same for new repos. Yes we could, but it feels like an unscalable solution. Seems like being able to bundle monorepo packages would be a good capability to have in our back pocket going forward, in any case. |
eladb
left a comment
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 feel this is the wrong direction. The fact that we need to massage the tarball after its created doesn't smell right, and the sheer amount of code required to do that feels like it's introducing unneeded risk to our packaging process (which is in itself fragile).
In this particular case, I think simply copy & pasting this single function is probably a safer and more pragmatic solution.
I'll leave it to you to decide if this is still the approach you think we should take.
Approving with "do-not-merge".
|
I want to have the ability to bundle monorepo packages going forward. |
|
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). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
This reverts commit 1cd175d. Due to build break.
`aws-cdk-lib` needs to have no dependencies (except for `constructs`), and we don't want to support this package as part of its public API. Therefore, it needs to be bundled into all packages that consume it. NPM has a bug that makes it impossible to `bundledDependencies` a package that is linked from the monorepo. We tried to work around that bug but it blocked our build because the `.tgz` is sometimes not available during the `postpack` phase (#13933). Simplest way around this now is to just copy/paste the implementation of `yaml-cfn` 3 times. Should we ever need to bundle monorepo dependencies again in the future, we can try to revive the `postpack` solution of #13933.
`aws-cdk-lib` needs to have no dependencies (except for `constructs`), and we don't want to support this package as part of its public API. Therefore, it needs to be bundled into all packages that consume it. NPM has a bug that makes it impossible to `bundledDependencies` a package that is linked from the monorepo. We tried to work around that bug but it blocked our build because the `.tgz` is sometimes not available during the `postpack` phase (#13933). Simplest way around this now is to just copy/paste the implementation of `yaml-cfn` 3 times. Should we ever need to bundle monorepo dependencies again in the future, we can try to revive the `postpack` solution of #13933. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
The `yaml-cfn` library is used as an implementation detail of `cloudformation-include`, `aws-cdk`, `decdk`, and the monopackages. Un-jsii it and bundle it into the framework packages that need it. Does not need to be accessed over jsii and does not need to be exposed in the monopackages. ---- The previous version of this PR (aws#13699) had to be reverted (aws@b1ffd33), because of a bug in NPM that causes `.npmignore` files in symlinked, bundled dependencies to be ignored. This in turn caused undesired `.ts` files to be bundled, which caused `ts-node` to exit with an error. Introduce a new `cdk-postpack` tool which will strip files that NPM *should* have ignored from the generated tarball, and add some verification to the `pack.sh` script to make sure we don't accidentally bundle `.ts` files. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
`aws-cdk-lib` needs to have no dependencies (except for `constructs`), and we don't want to support this package as part of its public API. Therefore, it needs to be bundled into all packages that consume it. NPM has a bug that makes it impossible to `bundledDependencies` a package that is linked from the monorepo. We tried to work around that bug but it blocked our build because the `.tgz` is sometimes not available during the `postpack` phase (aws#13933). Simplest way around this now is to just copy/paste the implementation of `yaml-cfn` 3 times. Should we ever need to bundle monorepo dependencies again in the future, we can try to revive the `postpack` solution of aws#13933. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
The
yaml-cfnlibrary is used as an implementation detail ofcloudformation-include,aws-cdk,decdk, and the monopackages.Un-jsii it and bundle it into the framework packages that need it.
Does not need to be accessed over jsii and does not need to be exposed
in the monopackages.
The previous version of this PR (#13699) had to be reverted (b1ffd33), because of a bug in NPM
that causes
.npmignorefiles in symlinked, bundled dependencies to be ignored.This in turn caused undesired
.tsfiles to be bundled, which causedts-nodeto exit with an error.
Introduce a new
cdk-postpacktool which will strip files that NPM should haveignored from the generated tarball, and add some verification to the
pack.shscriptto make sure we don't accidentally bundle
.tsfiles.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license