Add RFC for Fn::NumberComparison

[mingxingong]

Language Enhancement Request For Comment

This is a request for comments about a new intrinsic function Fn::NumberComparison. See #80 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.

[benkehoe]

Two questions:

• Should this be Fn::Compare for simplicity?
• Are there going to be any YAML problems with the comparison operators? In particular, > is used for line-folded multi-line strings. What's the likelihood users end up with confusing syntax errors if they do something wrong, and is there anything that can be done about it?

[josb]

Could the comparison function simply be expressed using words, e. g. Equals LessThan, etc.?

[benkehoe]

If we're talking about Fn::LessThan, maybe. If we're talking about

NotMeetRetentionRequirement: !NumberComparison
    - !Ref 'retentionInDays'
    - LessThanOrEquals
    - 365

I think that's probably worse.

[josb]

Why is it worse? Compare does seem like a more generic name. I like that the comparator function could potentially be configurable and without meta-character quoting issues.

[benkehoe]

it wouldn't help reduce CloudFormation's reputation for being verbose, for one thing

[josb]

I'd prefer verbosity over less functionality though.

[benbridts]

Would a !Sub like syntax be helpful?

NotMeetRetentionRequirement: !Maths "${retentionInDays} <= 365"
MeetsRetentionRequirement: !Maths [ "${x} <= 365", {x: !Ref retentionInDays}

(to restrict this to boolean results !IfMaths might be better)

[benbridts]

Here are a few fun yaml edge cases to think about (see also https://github.com/aws-cloudformation/cfn-language-discussion/issues/31#issuecomment-1133106727 to resolve at least some of these):

Parameters
  retentionInDays:
    Description: Log retention in days
    Type: Number
Parameters
  retentionInDaysString:
    Description: Log retention in days
    Type: String
Conditions:
  CastString: !NumberComparison
    - !Ref 'retentionInDays'
    - ==
    - "365"  # should this be cast to a number or fail?
  CastParameter: !NumberComparison
    - !Ref 'retentionInDaysString'  # should this be cast to a number or fail? 
    - ==
    - 365
  Octal1: !NumberComparison
    - !Ref 'retentionInDays'
    - ==
    - "0365"  # If we cast, should this be 365 or 254 (= octal 365)
  Octal2: !NumberComparison
    - 0365
    - ==
    - "0365"  # does the previous decision make this confusing?

and do previous decisions influence the json behaviour?

{"NotOctal": {"Fn::NumberComparison": [365, "==", "0365"]}

I think for the current implementation it would make sense to never cast (so it's only the current 0365 == 0o365 == 254 yaml <1.2 problem). However, there are Resources that will return numbers in a String type, so that would block future expansion to use this with resources (Fn::Cast could solve this, or if using the !Sub like syntax, you could have !IfMaths "number(${Resource.Attribute}) <= 123")

[josb]

How about !Expr instead of !IfMaths?

Opens the door to !Expr 'lowercase', !Ref MyParam, etc.

[benbridts]

How about !Expr instead of !IfMaths?

Opens the door to !Expr 'lowercase', !Ref MyParam, etc.

I'm not that worried about the actual name (although I think it's fun to imagine someone yelling "Maths!").

I would be careful with opening this up too much. Using the same function to edit data and to compare data seems like it would lead to problems later. I'd keep If or Compare in the name, so users know this should return a boolean.

Eg: !CompareExpression "lowercase(${MyParam}) == 'something'" would be closer to the original request

[josb]

I can live with that.

[rhbecker]

Given that it's been over a year since last movement on this issue, I figured it was worth a quick check-in ...

1. Are there outstanding concerns preventing the assigned reviewer from approving this?
2. Is there another person that could be assigned that is both eligible and available to serve as a 2nd reviewer?