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

RFC for Fn::Length #72

Merged
merged 4 commits into from
Jun 17, 2022
Merged

RFC for Fn::Length #72

merged 4 commits into from
Jun 17, 2022

Conversation

juegong2
Copy link
Contributor

Language Enhancement Request For Comment

This is a request for comments about new intrinsic function Fn::Size. See #70 for additional details.


Licensing

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

@juegong2 juegong2 requested a review from jlhood May 11, 2022 18:29
@juegong2 juegong2 self-assigned this May 11, 2022
@jlhood
Copy link
Contributor

jlhood commented May 12, 2022

@juegong2 Make sure to send a separate PR to update the README adding this RFC to the table.

Copy link
Contributor

@jlhood jlhood left a comment

Choose a reason for hiding this comment

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

Great first draft. No issues with syntax. Just want to make sure we include some clarifications on limitations.

RFCs/0070-Fn::Size.md Outdated Show resolved Hide resolved
RFCs/0070-Fn::Size.md Outdated Show resolved Hide resolved
RFCs/0070-Fn::Size.md Outdated Show resolved Hide resolved
RFCs/0070-Fn::Size.md Outdated Show resolved Hide resolved
RFCs/0070-Fn::Size.md Outdated Show resolved Hide resolved
RFCs/0070-Fn::Size.md Outdated Show resolved Hide resolved
RFCs/0070-Fn::Size.md Outdated Show resolved Hide resolved
RFCs/0070-Fn::Size.md Outdated Show resolved Hide resolved
RFCs/0070-Fn::Size.md Outdated Show resolved Hide resolved
RFCs/0070-Fn::Size.md Outdated Show resolved Hide resolved
@juegong2 juegong2 requested a review from jlhood May 12, 2022 20:49
Copy link
Contributor

@jlhood jlhood left a comment

Choose a reason for hiding this comment

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

2 minor comments, then it should be good to go!

RFCs/0070-Fn::Size.md Outdated Show resolved Hide resolved
RFCs/0070-Fn::Size.md Outdated Show resolved Hide resolved
@juegong2 juegong2 requested a review from jlhood May 12, 2022 22:02
jlhood
jlhood previously approved these changes May 12, 2022
Copy link
Contributor

@jlhood jlhood left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for iterating!

@lorengordon
Copy link

I'd suggest Fn::Length for greater similarity with most languages I've seen. I had no idea what was meant by Size when I first saw it here. I thought maybe it was related to something like the "size" of an EC2 instance type.

README.md Outdated
@@ -11,6 +11,7 @@ Go to: [What is an RFC](#what-is-an-rfc) | [RFC Process](#rfc-process)
[9](https://github.com/aws-cloudformation/cfn-language-discussion/issues/9)|[Adding looping functionality in CFN Template](https://github.com/aws-cloudformation/cfn-language-discussion/pull/19)|[@MalikAtalla-AWS](https://github.com/MalikAtalla-AWS)|✍️ review
[14](https://github.com/aws-cloudformation/cfn-language-discussion/issues/14)|[Convert Template Block to JSON String](https://github.com/aws-cloudformation/cfn-language-discussion/pull/15)|[@mluk-aws](https://github.com/mluk-aws)|✍️ review
[11](https://github.com/aws-cloudformation/cfn-language-discussion/issues/11)|[Allow Intrinsic Functions and Pseudo-Parameter References in DeletionPolicy and UpdateReplacePolicy](https://github.com/aws-cloudformation/cfn-language-discussion/pull/21)|[@MalikAtalla-AWS](https://github.com/MalikAtalla-AWS)|✍️ review
[70](https://github.com/aws-cloudformation/cfn-language-discussion/issues/70)|[Adding Fn::Size support in CFN Template](https://github.com/aws-cloudformation/cfn-language-discussion/pull/72)|[@juegong2](https://github.com/juegong2)|✍️ review
Copy link
Contributor

@jlhood jlhood May 13, 2022

Choose a reason for hiding this comment

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

Can you put this change in a separate PR? If you couple it with this one it won't get added to the README until after it's fully approved which doesn't make a lot of sense, since it's in review right now.

@josb
Copy link

josb commented May 13, 2022

@lorengordon I'm fine with the identifier Fn::Length if that is preferred.

Create RFC for Fn::Size
@jlhood
Copy link
Contributor

jlhood commented May 16, 2022

Some data to support that "length" tends to be more popular than "size":

  1. Java - Arrays have length property, Collections have size() method.
  2. Javascript - Arrays have length property
  3. Ruby - has both length and size methods (do the same thing, just aliases)
  4. Python - has built-in len() function
  5. Golang - also uses len() function
  6. HCL - length()
  7. CUE - len()

@juegong2 juegong2 changed the title RFC for Fn::Size RFC for Fn::Length May 17, 2022
@juegong2
Copy link
Contributor Author

Updated the naming of the new intrinsic function to Fn::Length

Copy link
Contributor

@jlhood jlhood left a comment

Choose a reason for hiding this comment

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

Can you rename the RFC file as well?

@juegong2
Copy link
Contributor Author

File name is updated as well

jlhood
jlhood previously approved these changes May 17, 2022
@benbridts
Copy link

I'm not sure if I understand the use case of this function:

  • For conditionals it's very clunky without support for integer comparisons.
  • For OutOfBound checking it has similar problems:
    • You can't use conditionals (see above)
    • CloudFormation will throw an error either way

```
because the ARN of MyBucket is not known until during provisioning.

**Note:** This is a short-term limitation due to underlying implementation constraints. In the fullness of time, this limitation should be removed.

Choose a reason for hiding this comment

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

I would remove this note, or be very careful about wording. Promising a short-term solution and not meeting this self-imposed deadline is a good way to lose trust.

Additionally, getting this to be useful during provisioning looks complex from the outside:

Having !Length work during provisioning does seem similar to how other functions work (so short term might be okay here)
However, most use cases will expect to be able to use an !If with !Length, and this would be a major change in how CloudFormation works. Currently in the If documentation, there is this remark.

You can only reference other conditions and values from the Parameters and Mappings sections of a template. For example, you can reference a value from an input parameter, but you can't reference the logical ID of a resource in a condition.

Repeating that in the limitations here might be a good idea too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback! I have updated the wording of this note.

Choose a reason for hiding this comment

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

👍🏻

Rename Fn::Size to Fn::Length

Co-Authored-By: Malik <[email protected]>
jlhood
jlhood previously approved these changes May 19, 2022
@juegong2 juegong2 dismissed stale reviews from jlhood and MalikAtalla-AWS via aa15c8c June 14, 2022 17:00
@juegong2 juegong2 merged commit dd14332 into aws-cloudformation:main Jun 17, 2022
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.

7 participants