feat(servicecatalog): add CloudFormation Parameter constraint by arcrank · Pull Request #15770 · aws/aws-cdk

arcrank · 2021-07-26T17:54:27Z

Add ability to configure parameter options for when launching a product.

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

Co-authored-by: Dillon Ponzo dponzo18@gmail.com

Add ability to configure parameter options for when launching a product. Co-authored-by: Dillon Ponzo <dponzo18@gmail.com>

gitpod-io · 2021-07-26T17:54:29Z

arcrank · 2021-07-26T17:57:08Z

Want to specifically call out how we call the formatRules in association-manager.ts to make sure that that approach is cdk friendly. Seemed like a clean way to do it in typescript but please let us know if there is another avenue that you think is better.

Also scoping down the types for the Cfn Conditions proved more cumbersome than just using the condition interface and adding documentation about what intrinsic functions you can use.

skinny85

Want to specifically call out how we call the formatRules in association-manager.ts to make sure that that approach is cdk friendly. Seemed like a clean way to do it in typescript but please let us know if there is another avenue that you think is better.

I think it's fine... what specifically were you worried about?

Also scoping down the types for the Cfn Conditions proved more cumbersome than just using the condition interface and adding documentation about what intrinsic functions you can use.

Can you expand why was it cumbersome? I still would like to see this change implemented, so that specifying the wrong function will be a compile-time error.

packages/@aws-cdk/aws-servicecatalog/lib/constraints.ts

packages/@aws-cdk/aws-servicecatalog/lib/portfolio.ts

packages/@aws-cdk/aws-servicecatalog/lib/private/association-manager.ts

skinny85 · 2021-07-26T21:37:13Z

packages/@aws-cdk/aws-servicecatalog/lib/private/association-manager.ts

+      // Add dependsOn to force proper order in deployment.
+      constraint.addDependsOn(association.cfnPortfolioProductAssociation);
+    } else {
+      throw new Error(`Provisioning rule ${assertion.ruleName} already configured on association ${this.prettyPrintAssociation(portfolio, product)}`);


There's some thought to making this override the previous Rule, instead of erroring out.

Maybe an option in constrainProvisioningParameters?

This is here because in CFN templates you would have all the provisioning rules listed in the same construct, so you can't have overlapping names which are the keys in the map. I broke it apart here like the event notifications, since one would probably only ever add 1-2 rules per product, but I think we still would rather keep unique names in case we need to shift back to that mode of adding all rules in one construct, which would be less of a breaking change if we enforce unique names now. But I don't think someone would want to override rules, usually they would do that with the condition.

packages/@aws-cdk/aws-servicecatalog/lib/portfolio.ts

arcrank · 2021-07-27T16:41:23Z

Can you expand why was it cumbersome? I still would like to see this change implemented, so that specifying the wrong function will be a compile-time error.

I meant it was going to be cumbersome to fit in this PR. We have been looking at how to modify the interface in core for these to extend a ICfnRuleConditionExpression that can extend the current ICfnConditionExpression but will take some time.

Pull request has been modified.

skinny85 · 2021-07-27T17:02:38Z

Can you expand why was it cumbersome? I still would like to see this change implemented, so that specifying the wrong function will be a compile-time error.

I meant it was going to be cumbersome to fit in this PR. We have been looking at how to modify the interface in core for these to extend a ICfnRuleConditionExpression that can extend the current ICfnConditionExpression but will take some time.

Fortunately, you made this PR small enough that I don't think adding the changes in core to it will make it too big 😉.

Please include these in this PR.

skinny85

Looks good overall, but please include the changes mentioned in #15770 (comment) to make providing the functions to the assertions compile-time safe.

packages/@aws-cdk/aws-servicecatalog/README.md

packages/@aws-cdk/aws-servicecatalog/lib/constraints.ts

packages/@aws-cdk/aws-servicecatalog/lib/private/association-manager.ts

skinny85 · 2021-07-27T17:50:45Z

packages/@aws-cdk/aws-servicecatalog/lib/private/util.ts

  ids.forEach(val => sha256.update(val));
  return sha256.digest('hex').slice(0, 12);
-}
+}


not sure what/how this snuck in.

Still here 😉.

didn't see what change was but I think this is removed now

packages/@aws-cdk/aws-servicecatalog/test/portfolio.test.ts

Pull request has been modified.

arcrank · 2021-07-28T00:20:19Z

I am still not sure if this is correct way for implementing the stronger typed assertions, I am still not sure if this is correct, it almost looks like the already existing ICfnConditionExpression is used where we need it.... still digging.

Edit: Actually I am not sure what exactly the issue was, I think ICfnConditionExpression is the rule conditions we want. Was there a different example of this working where it shouldn't?

packages/@aws-cdk/core/lib/cfn-condition.ts

arcrank · 2021-07-29T19:52:27Z

packages/@aws-cdk/core/lib/cfn-condition.ts

+ * Interface to specify certain functions as rule-specifc.
+ * These functions can only be used in ``Rules`` section of template.
+ */
+ export interface ICfnRuleConditionExpression extends ICfnConditionExpression {


packages/@aws-cdk/core/lib/cfn-fn.ts

arcrank · 2021-07-29T21:53:16Z

I might've requested review too early, I thought build failed after making CfnRule change but that might have been from master branch being out of date, rebuilding.

skinny85

Looks great! A few minor notes.

packages/@aws-cdk/aws-servicecatalog/README.md

packages/@aws-cdk/aws-servicecatalog/lib/constraints.ts

skinny85 · 2021-07-29T23:03:52Z

packages/@aws-cdk/aws-servicecatalog/lib/private/util.ts

  ids.forEach(val => sha256.update(val));
  return sha256.digest('hex').slice(0, 12);
-}
+}


Still here 😉.

packages/@aws-cdk/aws-servicecatalog/test/integ.portfolio.ts

packages/@aws-cdk/aws-servicecatalog/test/portfolio.test.ts

packages/@aws-cdk/core/lib/cfn-condition.ts

packages/@aws-cdk/core/lib/cfn-fn.ts

Pull request has been modified.

arcrank · 2021-07-30T04:12:02Z

Okay, I tried Codebuild with and without the changes in cfn-rule.ts since I kept getting build errors. I am pretty sure I've had updates from master, is there a better way to do it? I also fetched upstream in my local branch and did build and test in both servicecatalog and core.

skinny85 · 2021-07-30T16:53:21Z

Okay, I tried Codebuild with and without the changes in cfn-rule.ts since I kept getting build errors. I am pretty sure I've had updates from master, is there a better way to do it? I also fetched upstream in my local branch and did build and test in both servicecatalog and core.

Like I suspected, you need to revert the changes to CfnRule:

@aws-cdk/core... CHANGES.
Original assembly: @aws-cdk/core@1.116.0
Updated assembly:  @aws-cdk/core@0.0.0
API elements with incompatible changes:
err  - METHOD @aws-cdk/core.CfnRule.addAssertion: argument condition, takes @aws-cdk/core.ICfnRuleConditionExpression (formerly @aws-cdk/core.ICfnConditionExpression): @aws-cdk/core.ICfnConditionExpression does not extend @aws-cdk/core.ICfnRuleConditionExpression [incompatible-argument:@aws-cdk/core.CfnRule.addAssertion]
err  - IFACE @aws-cdk/core.CfnRuleAssertion: property assert, @aws-cdk/core.ICfnConditionExpression does not extend @aws-cdk/core.ICfnRuleConditionExpression: type of property assertions, input to @aws-cdk/core.CfnRule.<initializer> [strengthened:@aws-cdk/core.CfnRuleAssertion]
err  - IFACE @aws-cdk/core.CfnRuleProps: property ruleCondition, @aws-cdk/core.ICfnConditionExpression does not extend @aws-cdk/core.ICfnRuleConditionExpression: input to @aws-cdk/core.CfnRule.<initializer> [strengthened:@aws-cdk/core.CfnRuleProps]

skinny85

Looks great!

skinny85 · 2021-07-30T19:11:57Z

packages/@aws-cdk/aws-servicecatalog/README.md


+### CloudFormation parameters constraint
+
+CloudFormation parameters constraints allow you to configure the that are available to end users when they launch a product via defined rules.


Missing word?

Suggested change

CloudFormation parameters constraints allow you to configure the that are available to end users when they launch a product via defined rules.

CloudFormation parameters constraints allow you to configure the values that are available to end users when they launch a product via defined rules.

skinny85 · 2021-07-30T19:16:28Z

packages/@aws-cdk/core/lib/cfn-fn.ts


-class FnConditionBase extends Intrinsic implements ICfnConditionExpression {
+class FnConditionBase extends Intrinsic implements ICfnRuleConditionExpression {
+  readonly disambiguator = true;


Maybe an empty line after this line?

will track for next PR.

mergify · 2021-07-30T19:17:21Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2021-07-30T19:57:10Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 8edf43a
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2021-07-30T19:57:38Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

) Add ability to configure parameter options for when launching a product. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* Co-authored-by: Dillon Ponzo <dponzo18@gmail.com>

feat(servicecatalog): Add provisioning parameters constraint

e3699e2

Add ability to configure parameter options for when launching a product. Co-authored-by: Dillon Ponzo <dponzo18@gmail.com>

skinny85 previously requested changes Jul 26, 2021

View reviewed changes

Aidan Crank added 2 commits July 27, 2021 12:42

addressing PR comments

9249fd3

fixing typo

cd721bd

arcrank requested a review from skinny85 July 27, 2021 16:47

skinny85 previously requested changes Jul 27, 2021

View reviewed changes

addressing PR comments

c927a18

arcrank force-pushed the add_provisioning_rules branch from bffb25f to c927a18 Compare July 28, 2021 00:12

peterwoodworth assigned skinny85 Jul 28, 2021

peterwoodworth added the @aws-cdk/aws-servicecatalog Related to AWS Service Catalog label Jul 28, 2021

arcrank commented Jul 29, 2021

View reviewed changes

packages/@aws-cdk/core/lib/cfn-condition.ts Outdated Show resolved Hide resolved

arcrank commented Jul 29, 2021

View reviewed changes

packages/@aws-cdk/core/lib/cfn-fn.ts Show resolved Hide resolved

Aidan Crank added 5 commits July 29, 2021 17:00

addresing more PR comments, added a field to to enable structural typing

5d4057b

fixing a description field

4335c87

fixing integ test expected with updated description

ddbd6e1

fixing camel case

20df32a

changing name of field in rule expression back to disambiguator

28c6369

arcrank force-pushed the add_provisioning_rules branch from 71d4a3f to 28c6369 Compare July 29, 2021 21:26

arcrank requested a review from skinny85 July 29, 2021 21:51

integ expected file not uploaded as expected

a586aa8

arcrank force-pushed the add_provisioning_rules branch from 091631a to a586aa8 Compare July 29, 2021 22:19

skinny85 previously requested changes Jul 29, 2021

View reviewed changes

fixing last commit that deleted file instead of unstaging

32a7200

arcrank force-pushed the add_provisioning_rules branch from f64a78a to 32a7200 Compare July 30, 2021 01:06

Aidan Crank added 2 commits July 29, 2021 21:13

trying to remove changes to util file

c1d190e

updating language

8cc8d28

arcrank force-pushed the add_provisioning_rules branch from 8e73ae6 to 8cc8d28 Compare July 30, 2021 01:41

checking if build failure is related to rule condition change

f3887ba

arcrank force-pushed the add_provisioning_rules branch from b9d6d15 to f3887ba Compare July 30, 2021 03:09

Merge branch 'master' into add_provisioning_rules

935c6eb

one final test of cfn-rule change

a905e09

Aidan Crank and others added 2 commits July 30, 2021 13:34

reverting prev commit but keeping one formatting change in other file

6453116

Merge branch 'master' into add_provisioning_rules

00b1d0b

arcrank requested a review from skinny85 July 30, 2021 19:04

skinny85 approved these changes Jul 30, 2021

View reviewed changes

Merge branch 'master' into add_provisioning_rules

8edf43a

skinny85 changed the title ~~feat(servicecatalog): Add provisioning parameters constraint~~ feat(servicecatalog): add CloudFormation Parameter constraint Jul 30, 2021

mergify bot merged commit 58fda91 into aws:master Jul 30, 2021

arcrank deleted the add_provisioning_rules branch October 25, 2021 16:40


		### CloudFormation parameters constraint

		CloudFormation parameters constraints allow you to configure the that are available to end users when they launch a product via defined rules.

Conversation

arcrank commented Jul 26, 2021

Uh oh!

gitpod-io bot commented Jul 26, 2021

Uh oh!

arcrank commented Jul 26, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arcrank Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arcrank commented Jul 27, 2021

Uh oh!

skinny85 commented Jul 27, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

arcrank commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arcrank commented Jul 29, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arcrank commented Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skinny85 commented Jul 30, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

arcrank Jul 27, 2021 •

edited

Loading

arcrank commented Jul 28, 2021 •

edited

Loading

arcrank commented Jul 30, 2021 •

edited

Loading