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

CodePipeline Manual Approval Configuration should not require an SNS Topic. #6100

Closed
brettswift opened this issue Feb 4, 2020 · 1 comment · Fixed by #6106
Closed

CodePipeline Manual Approval Configuration should not require an SNS Topic. #6100

brettswift opened this issue Feb 4, 2020 · 1 comment · Fixed by #6106
Assignees
Labels
@aws-cdk/aws-codepipeline Related to AWS CodePipeline in-progress This issue is being actively worked on.

Comments

@brettswift
Copy link

Description

Something I noticed, which is low priority - the simple work around is to supply an SNS topic even if you don't want to use it.

Code pipeline manual approval configuration does not appear in the pipeline if an SNS topic is not provided.

The console should show "additional information" and the "external link" if there isn't an SNS topic, so approvers in the console can read the message. Cloudformation allows this, CDK does not.

The code that causes this is the conditional here:

configuration: this._notificationTopic
? {
NotificationArn: this._notificationTopic.topicArn,
CustomData: this.props.additionalInformation,
ExternalEntityLink: this.props.externalEntityLink,
}
: undefined,

Reproduction Steps

Run the following stack and look at the configuration output:

To isolate the part of the template this convenience command is provided
npm run build ; npx cdk synth | grep -i 'Category: Approval' -A 10 -B 10

import * as cdk from '@aws-cdk/core';
import codepipeline = require('@aws-cdk/aws-codepipeline');
import codepipelineActions = require('@aws-cdk/aws-codepipeline-actions');
import s3 = require('@aws-cdk/aws-s3');
import sns = require('@aws-cdk/aws-sns');

export class CodepipelineBugReproStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const sourceOutput = new codepipeline.Artifact("SourceArtifact");

    const sourceBucket = new s3.Bucket(this, `SourceBucket`, {
      versioned: true,
      removalPolicy: cdk.RemovalPolicy.DESTROY,
      bucketName: `bswift-bugreproduce-codepipeline`
    });

    const sourceAction = new codepipelineActions.S3SourceAction({
      actionName: 'S3Source',
      bucket: sourceBucket,
      bucketKey: 'artifact.zip',
      output: sourceOutput,
    } as codepipelineActions.S3SourceActionProps);

    const approval = new codepipelineActions.ManualApprovalAction({
      actionName: `Manual-Approval`,
// Comment this line out and deploy to reproduce ////////////////////////
      notificationTopic: new sns.Topic(this, 'dummyTopic'), 
/////////////////////////////////////////////////////////////////////////////////////////////////
      additionalInformation: "This deploy is scary, will you approve it?!",
      externalEntityLink: "https://bitbucket.com/amazingrepo",
      runOrder: 1,
    } as codepipelineActions.ManualApprovalActionProps)

    new codepipeline.Pipeline(this, 'TestPipeline', {
      pipelineName: 'BugReproPipeline',
      stages: [
        {
          stageName: 'source',
          actions: [sourceAction],
        },
        {
          stageName: 'deploy',
          actions: [approval]
        }
      ]
    });

  }
}

Environment

  • **CLI Version :1.22.0
  • **Framework Version:1.22.0
  • **OS :OSX
  • **Language :typescript

To Fix

I'm not sure what combination of those config properties is valid - if they are all valid independently the code should reflect that and allow it.

I can't seem to find this in the documentation for cloudformation - the actions show examples but don't seem to declare any other rules. I have successfully deployed an approval without SNS and with the other 2 configs.

This is 🐛 Bug Report

@brettswift brettswift added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 4, 2020
@skinny85 skinny85 self-assigned this Feb 4, 2020
@skinny85 skinny85 added @aws-cdk/aws-codepipeline Related to AWS CodePipeline and removed bug This issue is a bug. labels Feb 4, 2020
@skinny85
Copy link
Contributor

skinny85 commented Feb 4, 2020

Thanks for reporting @brettswift . I'll work on a fix.

skinny85 added a commit to skinny85/aws-cdk that referenced this issue Feb 4, 2020
…without a topic

We were only rendering the CustomData and ExternalEntityLink configuration properties for the manual approval action if a notification SNS topic was either created or passed when instantiating the action.
This is a mistake, as those properties make sense even when the action doesn't publish anything to a topic.

Fixes aws#6100
@SomayaB SomayaB added in-progress This issue is being actively worked on. chore and removed needs-triage This issue or PR still needs to be triaged. labels Feb 4, 2020
@mergify mergify bot closed this as completed in #6106 Feb 5, 2020
mergify bot added a commit that referenced this issue Feb 5, 2020
…without a topic (#6106)

We were only rendering the CustomData and ExternalEntityLink configuration properties for the manual approval action if a notification SNS topic was either created or passed when instantiating the action.
This is a mistake, as those properties make sense even when the action doesn't publish anything to a topic.

Fixes #6100

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-codepipeline Related to AWS CodePipeline in-progress This issue is being actively worked on.
Projects
None yet
3 participants