[Carry #1136] Return real booleans in the output #1409
Conversation
Redshift's `ParameterValue` is a string, see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-property-redshift-clusterparametergroup-parameter.html CloudFront StreamingDistribution's `Enabled` is a boolean, see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudfront-streamingdistribution-streamingdistributionconfig.html and https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudfront-streamingdistribution-trustedsigners.html Amazon EMR's `Enable*Encryption` are booleans, see https://docs.aws.amazon.com/emr/latest/ManagementGuide/emr-create-security-configuration.html#emr-encryption-intransit-cli AWS DLM's `CopyTag` is a boolean, see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-dlm-lifecyclepolicy-schedule.html OpsWorks' `MysqlRootPasswordUbiquitous` seems to be a string, see https://github.com/awsdocs/aws-opsworks-user-guide/blob/master/doc_source/cli-examples-describe-layers.md Elasticsearch's `rest.action.multi.allow_explicit_index` is a boolean, see https://www.elastic.co/guide/en/elasticsearch/reference/current/url-access-control.html Closes cloudtools#1136 and cloudtools#1409
|
Commit 62fbe39 should not have passed the tests. I've tried that before: https://travis-ci.com/michael-k/troposphere/builds/113547741 |
|
Feel free to squash all the commits together, there's nothing ground-breaking in mine :) |
|
Waiting on review/approval from @phobologic |
|
This looks good to me from a code perspective - but I'm curious what we should do to make sure folks aren't freaked out if suddenly a bunch of their stacks start updating? The other question I have is whether or not json's booleans are actually case sensitive? This page makes me think they are: https://json-schema.org/understanding-json-schema/reference/boolean.html |
If they are neither reading the changelog nor using cfn-lint … Maybe it's a good thing if they freak out and they'll start using cfn-lint afterwards.
Yes, they are. Python: import json
json.loads("true") # True
json.loads("True") # JSONDecodeErrorJavascript: JSON.parse("true") # true
JSON.parse("True") # SyntaxError |
|
Ahh, sorry - my bad on the case sensitive question, I had flipped the cases between the python/resulting templates in my head somehow. That looks good. I waffle a little on punishing/scaring folks for not using cfn-lint. Maybe a better way to roll this out would be to have this be sort of "Feature Flagged" with an environment variable or something, and only turn it on if that variable is set - then start using warnings to let folks know that this change is coming. Then after a verison or two we can make it the default? What are you thoughts? |
|
Any suggestions regarding the naming of such a env var? |
|
I'm open to just about anything - Also, don't take my suggestion as what has to happen :) Just wanted to get some conversation started around it. It seems like a nice compromise though, letting folks who want to use real booleans start using it while not penalizing folks who aren't ready for it. Also curious around thoughts on throwing warnings to let folks know about the change so that we can encourage people to prepare. @markpeek @michael-k |
|
Likely prefix for uniqueness: |
Redshift's `ParameterValue` is a string, see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-property-redshift-clusterparametergroup-parameter.html CloudFront StreamingDistribution's `Enabled` is a boolean, see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudfront-streamingdistribution-streamingdistributionconfig.html and https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudfront-streamingdistribution-trustedsigners.html Amazon EMR's `Enable*Encryption` are booleans, see https://docs.aws.amazon.com/emr/latest/ManagementGuide/emr-create-security-configuration.html#emr-encryption-intransit-cli AWS DLM's `CopyTag` is a boolean, see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-dlm-lifecyclepolicy-schedule.html OpsWorks' `MysqlRootPasswordUbiquitous` seems to be a string, see https://github.com/awsdocs/aws-opsworks-user-guide/blob/master/doc_source/cli-examples-describe-layers.md Elasticsearch's `rest.action.multi.allow_explicit_index` is a boolean, see https://www.elastic.co/guide/en/elasticsearch/reference/current/url-access-control.html Closes cloudtools#1136 and cloudtools#1409
Redshift's `ParameterValue` is a string, see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-property-redshift-clusterparametergroup-parameter.html CloudFront StreamingDistribution's `Enabled` is a boolean, see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudfront-streamingdistribution-streamingdistributionconfig.html and https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudfront-streamingdistribution-trustedsigners.html Amazon EMR's `Enable*Encryption` are booleans, see https://docs.aws.amazon.com/emr/latest/ManagementGuide/emr-create-security-configuration.html#emr-encryption-intransit-cli AWS DLM's `CopyTag` is a boolean, see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-dlm-lifecyclepolicy-schedule.html OpsWorks' `MysqlRootPasswordUbiquitous` seems to be a string, see https://github.com/awsdocs/aws-opsworks-user-guide/blob/master/doc_source/cli-examples-describe-layers.md Elasticsearch's `rest.action.multi.allow_explicit_index` is a boolean, see https://www.elastic.co/guide/en/elasticsearch/reference/current/url-access-control.html EC2 ClientVpnEndpoint's ConnectionLogOptions's `Enabled` is a boolean, see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-clientvpnendpoint-connectionlogoptions.html#cfn-ec2-clientvpnendpoint-connectionlogoptions-enabled MSK Cluster's EncryptionInTransit's `InCluster` is a boolean, see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-msk-cluster-encryptionintransit.html#cfn-msk-cluster-encryptionintransit-incluster Closes cloudtools#1136 and cloudtools#1409
|
I've added support for the env var
Is this good enough or does this need tests for the old behavior as well? |
|
This looks great to me. Thanks @michael-k for seeing this through! |
|
Thank you @michael-k and @ikben! |
…s#1409) * Use real booleans in the output only if env var is set * Adjust booleans in tests and examples Redshift's `ParameterValue` is a string, see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-property-redshift-clusterparametergroup-parameter.html CloudFront StreamingDistribution's `Enabled` is a boolean, see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudfront-streamingdistribution-streamingdistributionconfig.html and https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cloudfront-streamingdistribution-trustedsigners.html Amazon EMR's `Enable*Encryption` are booleans, see https://docs.aws.amazon.com/emr/latest/ManagementGuide/emr-create-security-configuration.html#emr-encryption-intransit-cli AWS DLM's `CopyTag` is a boolean, see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-dlm-lifecyclepolicy-schedule.html OpsWorks' `MysqlRootPasswordUbiquitous` seems to be a string, see https://github.com/awsdocs/aws-opsworks-user-guide/blob/master/doc_source/cli-examples-describe-layers.md Elasticsearch's `rest.action.multi.allow_explicit_index` is a boolean, see https://www.elastic.co/guide/en/elasticsearch/reference/current/url-access-control.html EC2 ClientVpnEndpoint's ConnectionLogOptions's `Enabled` is a boolean, see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-clientvpnendpoint-connectionlogoptions.html#cfn-ec2-clientvpnendpoint-connectionlogoptions-enabled MSK Cluster's EncryptionInTransit's `InCluster` is a boolean, see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-msk-cluster-encryptionintransit.html#cfn-msk-cluster-encryptionintransit-incluster Closes cloudtools#1136 and cloudtools#1409
- reference: cloudtools#1409
* Fix autoscaling.Tags to use boolean instead of str - reference: #1409 * Use boolean() for propagation default value too * Fix test_tags
This picks up the work done by @ikben in PR #1136, addresses the merge conflict by rebasing and fixes the failing tests.