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

update ttl to be long #1417

Merged
merged 4 commits into from
Mar 16, 2020
Merged

update ttl to be long #1417

merged 4 commits into from
Mar 16, 2020

Conversation

kddejong
Copy link
Contributor

Issue #, if available:
fix #1414
Description of changes:

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

@kddejong kddejong merged commit ea9c6e8 into aws-cloudformation:master Mar 16, 2020
@kddejong kddejong deleted the Fix/1414 branch March 16, 2020 12:23
@jason-idk
Copy link

Issue #, if available:
fix #1414
Description of changes:

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

I have a couple of issues here that I would like to surface:

  1. There are insufficient documentation updates that go with this change.
  2. I am now receiving errors while linting my previously working CF templates with records set in them.

If this is a required change, can you update the documentation where it states this needs to be a string and nothing else? (https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-route53-recordset.html#cfn-route53-recordset-ttl) Also can examples be added? I would have to imagine I am not the only one with this issue.

Thanks,
Jason

@anilkumarmyla
Copy link

We have several of our cfn templates fail because of this, is this change correct since the official documentation claims TTL is a string?

@deleugpn
Copy link

deleugpn commented Mar 19, 2020

Is it possible to allow both options? I reported this issue and I would not like to change all of my templates to String since TTL is obviously a number.
Validating it as String is also bad if the value is a non-numeric string. See #1425 (comment)

@kddejong
Copy link
Contributor Author

kddejong commented Mar 19, 2020

@deleugpn this is a tricky one but the answer to your question is yes. You can disable strict checking by configuring the rule E3012 to be strict false. This will do some conversion for you if needed. Example. If the type is number and you specify a string we will make sure the string is a number.

The hard part with this one is that it should be long in the docs and the spec.

Still talking with a few people about the best way to handle this.

@jarreds
Copy link

jarreds commented Mar 21, 2020

We use statically typed code generated off the spec, so this was a breaking change for us. Could this tool simply check if the string value is parseable as a long if it's not a legit long?

It seems like introducing this non-backward compatible change to the spec is unlikely, but that's pure speculation.

@maulik887
Copy link

maulik887 added a commit to maulik887/aws-cloudformation-user-guide that referenced this pull request Mar 25, 2020
As per this change updating Documentation aws-cloudformation/cfn-lint#1417
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TTL should be of type String
6 participants