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 Notifications #1222

Closed
moofish32 opened this issue Nov 20, 2018 · 4 comments · Fixed by #1368
Closed

CodePipeline Manual Approval Notifications #1222

moofish32 opened this issue Nov 20, 2018 · 4 comments · Fixed by #1368
Assignees
Labels
@aws-cdk/aws-codepipeline Related to AWS CodePipeline feature-request A feature should be added or improved.

Comments

@moofish32
Copy link
Contributor

Right now we can't fully configure notifications for Manual approvals because we are missing the properties in:

export interface ManualApprovalActionProps extends actions.CommonActionProps,
    actions.CommonActionConstructProps {
}

The two extended Props objects do not enable us to use the configuration property of CodePipeline Actions.

The first recommendation via @skinny85 in gitter is that we should add a property to pass in TopicRef to ManualApprovalActionProps. During the discussion we agreed that it might also make sense for us to create a ManualApprovalWithEmailNotifcationAction (naming open for discussion). This action would instead take a list of emails and add subscriptions for each to an SNS topic that was created for the user. This seems to be a very common use case.

I'm not sure how we want to view support for a generic ARN vs LambdaRef etc at this point. For now the SNS Topic seems the most logical. Of course leaving the Slack discussion off the table for the moment, but we are all aware of its popularity.

@skinny85
Copy link
Contributor

How about this API @moofish32 ?

export interface ManualApprovalActionProps extends actions.CommonActionProps,
    actions.CommonActionConstructProps {
  /**
   * Optional SNS topic to send notifications to when an approval is pending.
   */
  notificationTopic?: sns.TopicRef;

  /**
   * A list of email addresses to subscribe to notifications when this Action is pending approval.
   * If this has been provided, then you must also provide the `notificationTopic` property.
   */
  notifyAddresses?: string[];
}

Thoughts?

@skinny85 skinny85 self-assigned this Nov 20, 2018
@rix0rrr rix0rrr added feature-request A feature should be added or improved. @aws-cdk/aws-codepipeline Related to AWS CodePipeline labels Nov 21, 2018
@eladb
Copy link
Contributor

eladb commented Nov 21, 2018

If this has been provided, then you must also provide the notificationTopic property.

Why not just create a topic if one is not provided?

@moofish32
Copy link
Contributor Author

I'm 50/50 on notifyAddresses vs notifyEmails. I agree with @eladb just make them both optional and create a topic if it's missing, but I think I want the topic available as a public readonly so that we can use this later in a resource policy, etc.

@skinny85
Copy link
Contributor

skinny85 commented Nov 21, 2018

Why not just create a topic if one is not provided?

Slick. Haven't thought about this possibility, but I like it.

@moofish32 sure, notifyEmails and having the topic as a public readonly topic?: TopicRef on the Action both sound good to me.

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 feature-request A feature should be added or improved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants