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

feat: idiomize cloudformation intrinsics and pseudo parameters #1428

Merged
merged 10 commits into from
Dec 27, 2018

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Dec 25, 2018

Refactor CloudFormation intrinsic functions so that they will have a more idiomatic shape. They have been converted from classes (e.g. new FnJoin(...)) to static methods (e.g. Fn.join(...)). Condition functions are mapped to Fn.conditionXxx and return an FnCondition object.

Furthermore, return type of these static methods are now tokens represented as the resolved type (i.e. string or string[]) instead of CloudFormationToken objects. This allows using these functions and pseudo parameters naturally (instead of requiring the use of .toString() or .toList()).

Fixes #202

BREAKING CHANGE: CloudFormation intrinsic functions are now represented as static methods under the Fn class (e.g. Fn.join(...) instead of new FnJoin(...).toString()).


Pull Request Checklist

  • Testing
    • Unit test added
    • CLI change?: manually run integration tests and paste output as a PR comment
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

Copy link
Contributor

@RomainMuller RomainMuller left a comment

Choose a reason for hiding this comment

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

Mostly goodness! A couple of comments though... Nothing too bad.

As discussed, it may be wise to revert the Aws changes as they'll unfortunately collide with #1436 in ways that might result in hardcore merge conflict pain.

@@ -7,7 +7,7 @@
"pkglint": "tools/pkglint/bin/pkglint -f ."
},
"devDependencies": {
"@types/node": "^8.10.38",
"@types/node": "8.10.38",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the pinning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason it seems like npm keeps moving us to 10.x when we build in CI and then we get these unsolicited breakage of master. Other ideas?

packages/@aws-cdk/aws-s3/lib/bucket.ts Outdated Show resolved Hide resolved
* @param value A string, such as "A", that you want to compare against a list of strings.
* @returns an FnCondition token
*/
public static containsCondition(listOfStrings: string[], value: string): FnCondition {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the Condition suffix everywhere. It looks mighty redundant. In the case of containsConditions here, the name even would imply I'm checking if some array contains a FnCondition reference, which is not what it's doing.

I would at the very least remove the Condition suffix, and maybe ideally move this into a Condition class (Condition.equals(val, val), Condition.contains(arr, val), ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some suffix/prefix is needed for two reasons:

  • Many of the condition functions use names that are reserved words (such as "if", "and", "or").
  • It should be possible for users to easily distinguish between condition functions and normal functions.

I agree about "containsCondition" implying the wrong thing.

Alternative options:

  • A prefix "condition": conditionContains, conditionAnd, etc
  • A postfix "cond": containsCond, andCond
  • A prefix "cond": condContains, condIf

Any other ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer going for a prefix (since we don't want to go full on namespace/class, that's what's closest), preferably condition (the short-hand doesn't improve much of anything).

@@ -90,6 +100,8 @@ export class Parameter extends Referenceable {
super(parent, name);
this.properties = props;
this.value = new Ref(this);
this.valueAsString = this.value.toString();
this.valueAsList = this.value.toList();
Copy link
Contributor

Choose a reason for hiding this comment

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

Am always somewhat annoyed that only one of those is valid for a given entity... And we allows both. But I guess we can't do better, can we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that we can also provide StringParameter and StringListParamater as sugar classes that will make this a bit more strongly-typed.

packages/@aws-cdk/cdk/test/cloudformation/test.fn.ts Outdated Show resolved Hide resolved
rule.addAssertion(new FnEquals('lhs', 'rhs'), 'lhs equals rhs');
rule.addAssertion(new FnNot(new FnAnd(new FnContains([ 'hello', 'world' ], "world"))), 'some assertion');
rule.addAssertion(Fn.equalsCondition('lhs', 'rhs'), 'lhs equals rhs');
rule.addAssertion(Fn.notCondition(Fn.andCondition(Fn.containsCondition([ 'hello', 'world' ], "world"))), 'some assertion');
Copy link
Contributor

Choose a reason for hiding this comment

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

This... is where the helpers I mentioned previously come in handy... This Polish notation nonsense is a recipe for disaster. Also, it's so verbose!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my note about not over indexing on the CFN conditions programming model. Especially not as part of this change.

@eladb
Copy link
Contributor Author

eladb commented Dec 27, 2018

Reverted the pseudo parameter change to avoid conflict with #1436

@eladb eladb merged commit 04217a5 into master Dec 27, 2018
@eladb eladb deleted the benisrae/intrinsics-and-pseudo-refactor branch December 27, 2018 14:36
eladb pushed a commit that referenced this pull request Dec 28, 2018
Fixes #742 
Built on top of #1428 

BREAKING CHANGE: AWS resource-related classes have been changed to conform to API guidelines:

  - `XxxRef` abstract classes are now `IXxx` interfaces
  - `XxxRefProps` are now `XxxImportProps`
  - `XxxRef.import(...)` are now `Xxx.import(...)` accept `XxxImportProps` and return `IXxx`
  - `export(): XxxImportProps` is now defined in `IXxx` and implemented by imported resources
  - Lambda's static "metric" methods moved from `lambda.FunctionRef` to `lambda.Function`.
  - Route53 record classes now require a `zone` when created (not assuming zone is the parent construct).
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 23, 2019
@mergify
Copy link
Contributor

mergify bot commented Sep 23, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Intrinsic functions and pseudo parameters hard to find, confusing to use
3 participants