Skip to content
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

chore(cdk cli): Disable Pretty Printing CloudFormation JSON Templates #18886

Closed
wants to merge 2 commits into from

Conversation

mckalexee
Copy link

Currently the CDK pretty printing JSON files whenever it creates the CloudFormation templates. Since these files are only read by machines, the extra formatting only increases the file size.

This is a practical concern, since CloudFormation only allows template files to be 1mb in size. On a test run, I was able to reduce an output template size from 886kb to 495kb.

Since this change is small, ideally it could be backported to v1.

fixes #18694


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

The JSON file that is created by the CDK for deployment
is only read by machines, but pretty printing
the file uses up a lot of space. This is a practical
concern due to the fact that CloudFormation
has a file size limit for templates
@gitpod-io
Copy link

gitpod-io bot commented Feb 9, 2022

@github-actions github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Feb 9, 2022
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this change is safe.

Since these files are only read by machines, the extra formatting only increases the file size.

You make this assertion, but we don't actually know that. For one counterexample I know of: we send the template to CloudFormation. In the console, CloudFormation will show the template in the exact same format as it was sent to it.

So, if today someone relies on reading the CDK templates in the CloudFormation console, they would be in for a rude awakening if all their templates become unreadable all of a sudden.

And there are probably more cases that we don't even know about.

This change would need to be more selective.

@mckalexee
Copy link
Author

Thanks @rix0rrr, those are good points.

I do think the difference in resulting size of the templates is significant enough that this is worth tackling. CDK already generates fairly long names to prevent collision, and those take up a bit of space on their own. Getting the file size down makes CDK much more usable.

I like @PatMyron's idea to change the indentation to 1. We still get the formatting/readability benefits with the smaller file sizes, and the change is small.

Alternatively, would it be reasonable to be able to configure the compaction of the JSON file with a cli option or an environment variable? That way nothing breaks for anyone, but teams can enable this option as they see fit.

The CloudFormation console will display 
the original template file that was 
uploaded by the CDK. To balance the 
concerns of readability and file size
the indentation has been changed from
two spaces to one space.

Co-authored-by: Pat Myron <PatMyron@users.noreply.github.com>
@mergify mergify bot dismissed rix0rrr’s stale review March 2, 2022 15:12

Pull request has been modified.

@mckalexee mckalexee requested a review from rix0rrr March 2, 2022 15:13
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@rix0rrr rix0rrr added feature-request A feature should be added or improved. p2 and removed p2 feature-request A feature should be added or improved. package/tools Related to AWS CDK Tools or CLI labels Mar 4, 2022
@rigsbyt
Copy link

rigsbyt commented Mar 31, 2022

@rix0rrr can you provide any more detail on what you think could make this workable? A cli option, ...?

As someone with stacks that are running into the template size limit, I will gladly sacrifice readability in the console for an easy ~50% reduction in my stack sizes...

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line you changed will not be hit in practice anymore (with any recent version of the CDK framework).

The location to change is this one: https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/core/lib/stack.ts#L796

Add property to StackProps like readonly minimizeTemplate?: boolean;, explaining the behavior and the trade-off (non-human readable template in CloudFormation console).

Don't forget to add a unit test.

@rigsbyt
Copy link

rigsbyt commented Mar 31, 2022

Thanks @rix0rrr

@mckalexee are you still interested in pushing this through? I can probably support if you're not interested or able.

@PatMyron
Copy link
Contributor

Most won't opt into optional features, so improving defaults marginally has larger impact than optionally optimizing further, especially since it's already possible to remove all whitespace with something like jq for those seeking workarounds, so I think it's worth starting with improving the default slightly:
#19656

@rigsbyt
Copy link

rigsbyt commented Mar 31, 2022

@PatMyron can you provide an example of how to leverage jq with the CDK tools? Or are you suggesting not using the CDK tools for deployment if you go this route?

@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 1, 2022

I shipped Pat's change, but I'm still willing to ship this one if it gets updated.

One more thing: if we determine that the rendered template will be too big for CloudFormation, we add a warning (not an error!) that suggest use of the new property to minimize the template.

This addresses Pat's concern of "most won't opt into optional features": they will if we tell them to.

mergify bot pushed a commit that referenced this pull request Apr 1, 2022
also: aws/serverless-application-model#2368, cloudtools/troposphere#2028
#18694
#18886
[CloudFormation templates can currently only be 1MB](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/cloudformation-limits.html)

Simply reducing indentation from 2 to 1 should remove ~1/4 of the template file size for everyone by default while still preserving indentation formatting. Beyond improving the default, those wishing to reduce readability for further reduced file size could opt into using something like [`jq`](https://stedolan.github.io/jq/) on their own for now

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

StevePotter pushed a commit to StevePotter/aws-cdk that referenced this pull request Apr 27, 2022
also: aws/serverless-application-model#2368, cloudtools/troposphere#2028
aws#18694
aws#18886
[CloudFormation templates can currently only be 1MB](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/cloudformation-limits.html)

Simply reducing indentation from 2 to 1 should remove ~1/4 of the template file size for everyone by default while still preserving indentation formatting. Beyond improving the default, those wishing to reduce readability for further reduced file size could opt into using something like [`jq`](https://stedolan.github.io/jq/) on their own for now

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@github-actions github-actions bot added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Apr 30, 2022
@github-actions github-actions bot closed this Apr 30, 2022
@Dan-Wuensch
Copy link

Please re-open this. The 1 MB stack template limit is a huge issue for scaling CDK projects, especially as it pertains to API Gateway. Minification wouldn't solve the problem but is a nice bandaid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(cdk cli): Create Smaller CloudFormation Templates
6 participants