Skip to content

Conversation

@iliapolo
Copy link
Contributor

@iliapolo iliapolo commented May 7, 2020

Commit Message

chore(cli): fix nested stack integ test collision (#7846)

The StackWithNestedStackUsingParameters in our integ test was for some reason passing a hardcoded parameter value to the nested stack instead of using the parameter value from the parent stack. In addition to not actually testing that parameters are passable between the parent and the child, it caused a collision with parallel tests because the topic name is provided was not unique.

This PR simply passes the parent stack parameter to the nested stack, the value of the parent stack param is already unique because it prepends the stack prefix.

In addition, many tests were using parameters like so:

new sns.Topic(this, 'TopicParameter', {
  topicName: new cdk.CfnParameter(this, 'TopicNameParam')
});

instead of:

new sns.Topic(this, 'TopicParameter', {
  topicName: new cdk.CfnParameter(this, 'TopicNameParam').valueAsString
});

The topicName property expects a string, not a CfnParameter. Since our tests are in javascript, it "compiled". But migrating this to typescript would actually result in a proper compilation error.

Strangely enough, both of these synthesize to the same thing:

Parameters:
  MyTopicParam:
    Type: String
Resources:
  MyTopic86869434:
    Type: AWS::SNS::Topic
    Properties:
      TopicName:
        Ref: MyTopicParam

This is because CfnParameter resolves to its value getter:

public resolve(_context: IResolveContext): any {
  return this.value;
}

End Commit Message

Regarding the valueAsString quirk, I changed it since i think the original commit was just an oversight that happened to work. Our docs all refer to using valueAs methods when defining parameters.


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

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label May 7, 2020
@mergify
Copy link
Contributor

mergify bot commented May 7, 2020

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
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: e1c6239
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 06b8a54
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 2e0b433
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented May 7, 2020

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).

@mergify mergify bot merged commit 5b34ccb into master May 7, 2020
@mergify mergify bot deleted the epolon/fix-nested-stack-params-integ-test branch May 7, 2020 16:33
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.

4 participants