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

Framework: remove property renames for CloudFormation constructs #852

Closed
elexisvenator opened this issue Oct 5, 2018 · 3 comments · Fixed by #1021 or #973
Closed

Framework: remove property renames for CloudFormation constructs #852

elexisvenator opened this issue Oct 5, 2018 · 3 comments · Fixed by #1021 or #973
Assignees
Labels
bug This issue is a bug.

Comments

@elexisvenator
Copy link

in AssociationResourceProps: documentName is documentName
in the CF template, this becomes DocumentName

This fails when deploying the stack. According to the documentation, the parameter should be Name. Manually modifying the CF template generated by CDK to use the correct name works.

My workaround
Because AssociationResource is a L1 model, I have built a wrapper around it to do what I need. I have change my wrapper to instead create an instance of OverloadedAssociation - a class I created that extends AssociationResource.

  /**
   * ``AssociationResource`` does not generate the correct CF file, one of the parameters needs to be renamed.
   */
  class OverloadedAssociation extends Ssm.AssociationResource {
    /**
     * Creates a new ``AWS::SSM::Association``.
     *
     * @param parent   the ``cdk.Construct`` this ``AssociationResource`` is a part of
     * @param name     the name of the resource in the ``cdk.Construct`` tree
     * @param properties the properties of this ``AssociationResource``
     */
    constructor(parent: Construct, name: string, properties: Ssm.AssociationResourceProps) {
      super(parent, name, properties);
    }

    protected renderProperties(properties: any): { [key: string]: any } {
      const baseResult = super.renderProperties(properties);

      return {
        // this is the line that changed
        Name: baseResult.DocumentName,
        AssociationName: baseResult.AssociationName,
        DocumentVersion: baseResult.DocumentVersion,
        InstanceId: baseResult.InstanceId,
        OutputLocation: baseResult.OutputLocation,
        Parameters: baseResult.Parameters,
        ScheduleExpression: baseResult.ScheduleExpression,
        Targets: baseResult.Targets,
      };
    }
  }

Is this the right way to workaround this problem? Should I be modifying .ToCloudFormation() instead?

@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 5, 2018

You shouldn't have to do anything yourself. This translation happens in the code generator. We have an issue there. Thanks for reporting.

@rix0rrr rix0rrr self-assigned this Oct 5, 2018
@rix0rrr rix0rrr added the bug This issue is a bug. label Oct 5, 2018
rix0rrr pushed a commit that referenced this issue Oct 5, 2018
The rename used to happen in the cfnspec, which helped the model
generate correct names, but the code generator would lose the ability to
access the original name, and so it could not be emitted correctly.

Add a property renaming feature to the 'cfn2ts' code generator
(configured via a special entry in package.json).

Fixes #852.
rix0rrr pushed a commit that referenced this issue Oct 5, 2018
The rename used to happen in the cfnspec, which helped the model
generate correct names, but the code generator would lose the ability to
access the original name, and so it could not be emitted correctly.

Add a property renaming feature to the 'cfn2ts' code generator
(configured via a special entry in package.json).

Fixes #852.
@ChintanRaval
Copy link
Contributor

here's what I've found as a workaround while this bug gets fixed:

import { cloudformation as ssmCloudFormation } from '@aws-cdk/aws-ssm';
import { App, Stack, StackProps } from '@aws-cdk/cdk';

export class TestStack extends Stack {
    constructor(parent: App, name: string, props: StackProps) {
      super(parent, name, props);

      const nameOfDocument = 'something';
      const associationResource = new ssmCloudFormation.AssociationResource(this, "bob", {
        documentName: nameOfDocument
      })
      associationResource.addOverride('Properties.Name', nameOfDocument);
      associationResource.addOverride('Properties.DocumentName', undefined);
    }
}

and it gives the below output:

Resources:
    bob:
        Type: 'AWS::SSM::Association'
        Properties:
            Name: something
    CDKMetadata:
        Type: 'AWS::CDK::Metadata'
        Properties:
            Modules: '@aws-cdk/aws-directoryservice=0.10.0,@aws-cdk/aws-ec2=0.10.0,@aws-cdk/aws-iam=0.10.0,@aws-cdk/aws-ssm=0.10.0,@aws-cdk/cdk=0.10.0,@aws-cdk/cx-api=0.10.0,esprima=4.0.1,js-base64=2.4.9,js-yaml=3.12.0,monolith-sql-servers=0.0.1'

Found help in release notes here and documentation here.

@eladb
Copy link
Contributor

eladb commented Oct 8, 2018

@ChintanRaval glad you were able to work around this issue. Stay tuned for #854

@rix0rrr rix0rrr changed the title aws-ssm AssociationResource uses incorrect name in CF for "document name" Framework: remove property renames for CloudFormation constructs Oct 9, 2018
rix0rrr pushed a commit that referenced this issue Oct 19, 2018
BREAKING CHANGE: the 'Name' -> 'ResourceName' renames of  L1
CloudFormation resources have been removed, to match CloudFormation
behavior.

Fixes #852.
rix0rrr added a commit that referenced this issue Oct 22, 2018
The construct id parameter used to be a property called `name` in the 
props object, and so properties called `name` were renamed to `xxxName`
(where `xxx` is the name of the resource). Since we now pass names as
a constructor argument, we can get rid of the properly rename mechanism.

BREAKING CHANGE: if you use L1s, you may need to change some
`XxxName` properties back into `Name`. These will match the CloudFormation
property names.

Fixes #852.
rix0rrr pushed a commit that referenced this issue Oct 26, 2018
__IMPORTANT NOTE__: when upgrading to this version of the CDK framework, you must also upgrade
your installation the CDK Toolkit to the matching version:

```shell
$ npm i -g aws-cdk
$ cdk --version
0.14.0 (build ...)
```

Bug Fixes
=========

* remove CloudFormation property renames ([#973](#973)) ([3f86603](3f86603)), closes [#852](#852)
* **aws-ec2:** fix retention of all egress traffic rule ([#998](#998)) ([b9d5b43](b9d5b43)), closes [#987](#987)
* **aws-s3-deployment:** avoid deletion during update using physical ids ([#1006](#1006)) ([bca99c6](bca99c6)), closes [#981](#981) [#981](#981)
* **cloudformation-diff:** ignore changes to DependsOn ([#1005](#1005)) ([3605f9c](3605f9c)), closes [#274](#274)
* **cloudformation-diff:** track replacements ([#1003](#1003)) ([a83ac5f](a83ac5f)), closes [#1001](#1001)
* **docs:** fix EC2 readme for "natgatway" configuration ([#994](#994)) ([0b1e7cc](0b1e7cc))
* **docs:** updates to contribution guide ([#997](#997)) ([b42e742](b42e742))
* **iam:** Merge multiple principals correctly ([#983](#983)) ([3fc5c8c](3fc5c8c)), closes [#924](#924) [#916](#916) [#958](#958)

Features
=========

* add construct library for Application AutoScaling ([#933](#933)) ([7861c6f](7861c6f)), closes [#856](#856) [#861](#861) [#640](#640) [#644](#644)
* add HostedZone context provider ([#823](#823)) ([1626c37](1626c37))
* **assert:** haveResource lists failing properties ([#1016](#1016)) ([7f6f3fd](7f6f3fd))
* **aws-cdk:** add CDK app version negotiation ([#988](#988)) ([db4e718](db4e718)), closes [#891](#891)
* **aws-codebuild:** Introduce a CodePipeline test Action. ([#873](#873)) ([770f9aa](770f9aa))
* **aws-sqs:** Add grantXxx() methods ([#1004](#1004)) ([8c90350](8c90350))
* **core:** Pre-concatenate Fn::Join ([#967](#967)) ([33c32a8](33c32a8)), closes [#916](#916) [#958](#958)

BREAKING CHANGES
=========

* DynamoDB AutoScaling: Instead of `addReadAutoScaling()`, call `autoScaleReadCapacity()`, and similar for write scaling.
* CloudFormation resource usage: If you use L1s, you may need to change some `XxxName` properties back into `Name`. These will match the CloudFormation property names.
* You must use the matching `aws-cdk` toolkit when upgrading to this version, or context providers will cease to work. All existing cached context values in `cdk.json` will be invalidated and refreshed.
@rix0rrr rix0rrr mentioned this issue Oct 26, 2018
rix0rrr added a commit that referenced this issue Oct 26, 2018
__IMPORTANT NOTE__: when upgrading to this version of the CDK framework, you must also upgrade
your installation the CDK Toolkit to the matching version:

```shell
$ npm i -g aws-cdk
$ cdk --version
0.14.0 (build ...)
```

Bug Fixes
=========

* remove CloudFormation property renames ([#973](#973)) ([3f86603](3f86603)), closes [#852](#852)
* **aws-ec2:** fix retention of all egress traffic rule ([#998](#998)) ([b9d5b43](b9d5b43)), closes [#987](#987)
* **aws-s3-deployment:** avoid deletion during update using physical ids ([#1006](#1006)) ([bca99c6](bca99c6)), closes [#981](#981) [#981](#981)
* **cloudformation-diff:** ignore changes to DependsOn ([#1005](#1005)) ([3605f9c](3605f9c)), closes [#274](#274)
* **cloudformation-diff:** track replacements ([#1003](#1003)) ([a83ac5f](a83ac5f)), closes [#1001](#1001)
* **docs:** fix EC2 readme for "natgatway" configuration ([#994](#994)) ([0b1e7cc](0b1e7cc))
* **docs:** updates to contribution guide ([#997](#997)) ([b42e742](b42e742))
* **iam:** Merge multiple principals correctly ([#983](#983)) ([3fc5c8c](3fc5c8c)), closes [#924](#924) [#916](#916) [#958](#958)

Features
=========

* add construct library for Application AutoScaling ([#933](#933)) ([7861c6f](7861c6f)), closes [#856](#856) [#861](#861) [#640](#640) [#644](#644)
* add HostedZone context provider ([#823](#823)) ([1626c37](1626c37))
* **assert:** haveResource lists failing properties ([#1016](#1016)) ([7f6f3fd](7f6f3fd))
* **aws-cdk:** add CDK app version negotiation ([#988](#988)) ([db4e718](db4e718)), closes [#891](#891)
* **aws-codebuild:** Introduce a CodePipeline test Action. ([#873](#873)) ([770f9aa](770f9aa))
* **aws-sqs:** Add grantXxx() methods ([#1004](#1004)) ([8c90350](8c90350))
* **core:** Pre-concatenate Fn::Join ([#967](#967)) ([33c32a8](33c32a8)), closes [#916](#916) [#958](#958)

BREAKING CHANGES
=========

* DynamoDB AutoScaling: Instead of `addReadAutoScaling()`, call `autoScaleReadCapacity()`, and similar for write scaling.
* CloudFormation resource usage: If you use L1s, you may need to change some `XxxName` properties back into `Name`. These will match the CloudFormation property names.
* You must use the matching `aws-cdk` toolkit when upgrading to this version, or context providers will cease to work. All existing cached context values in `cdk.json` will be invalidated and refreshed.
jonparker pushed a commit to jonparker/aws-cdk that referenced this issue Oct 29, 2018
__IMPORTANT NOTE__: when upgrading to this version of the CDK framework, you must also upgrade
your installation the CDK Toolkit to the matching version:

```shell
$ npm i -g aws-cdk
$ cdk --version
0.14.0 (build ...)
```

Bug Fixes
=========

* remove CloudFormation property renames ([aws#973](aws#973)) ([3f86603](aws@3f86603)), closes [aws#852](aws#852)
* **aws-ec2:** fix retention of all egress traffic rule ([aws#998](aws#998)) ([b9d5b43](aws@b9d5b43)), closes [aws#987](aws#987)
* **aws-s3-deployment:** avoid deletion during update using physical ids ([aws#1006](aws#1006)) ([bca99c6](aws@bca99c6)), closes [aws#981](aws#981) [aws#981](aws#981)
* **cloudformation-diff:** ignore changes to DependsOn ([aws#1005](aws#1005)) ([3605f9c](aws@3605f9c)), closes [aws#274](aws#274)
* **cloudformation-diff:** track replacements ([aws#1003](aws#1003)) ([a83ac5f](aws@a83ac5f)), closes [aws#1001](aws#1001)
* **docs:** fix EC2 readme for "natgatway" configuration ([aws#994](aws#994)) ([0b1e7cc](aws@0b1e7cc))
* **docs:** updates to contribution guide ([aws#997](aws#997)) ([b42e742](aws@b42e742))
* **iam:** Merge multiple principals correctly ([aws#983](aws#983)) ([3fc5c8c](aws@3fc5c8c)), closes [aws#924](aws#924) [aws#916](aws#916) [aws#958](aws#958)

Features
=========

* add construct library for Application AutoScaling ([aws#933](aws#933)) ([7861c6f](aws@7861c6f)), closes [aws#856](aws#856) [aws#861](aws#861) [aws#640](aws#640) [aws#644](aws#644)
* add HostedZone context provider ([aws#823](aws#823)) ([1626c37](aws@1626c37))
* **assert:** haveResource lists failing properties ([aws#1016](aws#1016)) ([7f6f3fd](aws@7f6f3fd))
* **aws-cdk:** add CDK app version negotiation ([aws#988](aws#988)) ([db4e718](aws@db4e718)), closes [aws#891](aws#891)
* **aws-codebuild:** Introduce a CodePipeline test Action. ([aws#873](aws#873)) ([770f9aa](aws@770f9aa))
* **aws-sqs:** Add grantXxx() methods ([aws#1004](aws#1004)) ([8c90350](aws@8c90350))
* **core:** Pre-concatenate Fn::Join ([aws#967](aws#967)) ([33c32a8](aws@33c32a8)), closes [aws#916](aws#916) [aws#958](aws#958)

BREAKING CHANGES
=========

* DynamoDB AutoScaling: Instead of `addReadAutoScaling()`, call `autoScaleReadCapacity()`, and similar for write scaling.
* CloudFormation resource usage: If you use L1s, you may need to change some `XxxName` properties back into `Name`. These will match the CloudFormation property names.
* You must use the matching `aws-cdk` toolkit when upgrading to this version, or context providers will cease to work. All existing cached context values in `cdk.json` will be invalidated and refreshed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug.
Projects
None yet
4 participants