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

aws-chatbot: ISlackChannelConfiguration should include addNotificationTopic() method #21171

Closed
2 tasks done
polastre opened this issue Jul 15, 2022 · 25 comments
Closed
2 tasks done
Labels
@aws-cdk/aws-chatbot Related to AWS Chatbot effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 wontfix We have determined that we will not resolve the issue.

Comments

@polastre
Copy link

polastre commented Jul 15, 2022

Describe the feature

#15588 and #16643 added the method addNotificationTopic() to the SlackChannelConfiguration construct. This allows the SNS topic to be added to an existing configuration.

For an existing slack channel, it can be imported with SlackChannelConfiguration. fromSlackChannelConfigurationArn. This returns SlackChannelConfiguration.

If you create a new SlackChannelConfiguration construct, then you can use addNotificationTopic since it returns that construct object. If you use fromSlackChannelConfigurationArn, then you get ISlackChannelConfiguration which does not have addNotificationTopic.

The request to is add addNotificationTopic to ISlackChannelConfiguration so that it can used with existing slack channel configurations.

Use Case

A stack where the slack channel configuration is already defined, and the stack seeks to add notification topics to that existing slack channel configuration.

Proposed Solution

Add addNotificationTopic to the interface in https://github.com/xykkong/aws-cdk/blob/master/packages/@aws-cdk/aws-chatbot/lib/slack-channel-configuration.ts#L109

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.32.0

Environment details (OS name and version, etc.)

typescript

@polastre polastre added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jul 15, 2022
@github-actions github-actions bot added the @aws-cdk/aws-chatbot Related to AWS Chatbot label Jul 15, 2022
@kaizencc kaizencc added p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Jul 20, 2022
@kaizencc kaizencc removed their assignment Jul 20, 2022
@kaizencc
Copy link
Contributor

This sounds reasonable @polastre! Are you interested in contributing this?

@daschaa
Copy link
Contributor

daschaa commented Jul 27, 2022

@kaizencc I tried working on this but I'm not sure how to properly test this. Do you have an idea/hint how to do it?

@debnon
Copy link

debnon commented Mar 28, 2023

I implemented this change, but when unit testing realized that adding an SNS topic to a Slack Config retrieved by ARN from another stack doesn't actually seem to add it to the existing Slack Config.

Does it go against best practices to modify one stack from another like this, and if not, is there an established way to trigger the changes to Cloudformation after calling the method to add SNS topics? Thanks.

@sercantor
Copy link

I also had the same thing @debnon, when I added the method and tried to use it, it created the sns topics, but they weren't in slack channel topics. can anyone show an example similar to this so we can just add it?

@zentavr
Copy link

zentavr commented Apr 20, 2023

Is there any workaround to use shared stack which creates slack notifications and attach the new SNS topics inside?

@ben-buitendijk
Copy link

@larskinder
That is Python, right? I'm using Typescript and it seems like the construct doesn't have that method...

image

Also it doesn't show in the documentation:
https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_chatbot.ISlackChannelConfiguration.html

@larskinder
Copy link

Sry, re-added my previous post, that you were referencing to.

This is how you can add SNS topics to an existing Slack config.

slack_bot = chatbot.SlackChannelConfiguration(
    chatbot.SlackChannelConfiguration.from_slack_channel_configuration_arn(
        self,
        "SlackNotifications",
        slack_channel_configuration_arn=(
            f"arn:aws:chatbot::{account}:chat-configuration/slack-channel/test-channel-config"
        ),
    )
)

slack_bot.add_notification_topic(sns_topic)
slack_bot.add_notification_topic(another_sns_topic)

@larskinder
Copy link

larskinder commented Apr 21, 2023

@ben-buitendijk what version of cdk are you using?
In general, this method (fromSlackChannelConfigurationArn(scope, id, slackChannelConfigurationArn)) is available.

https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_chatbot.SlackChannelConfiguration.html#static-fromwbrslackwbrchannelwbrconfigurationwbrarnscope-id-slackchannelconfigurationarn

@ben-buitendijk
Copy link

That is the page when you create a new SlackChannelConfiguration, but the construct when you import it from an ARN doesn't have that method

@ollesumonto
Copy link

Need this for the interface.

@sercantor
Copy link

no response? this shouldn't be that hard I feel like 😁

@github-actions github-actions bot added p1 and removed p2 labels May 14, 2023
@github-actions
Copy link

This issue has received a significant amount of attention so we are automatically upgrading its priority. A member of the community will see the re-prioritization and provide an update on the issue.

@mayconfrr
Copy link

Bump

@luiscastillocr
Copy link

luiscastillocr commented Aug 21, 2023

I think this is already implemented by now

https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_chatbot.SlackChannelConfiguration.html#addwbrnotificationwbrtopicnotificationtopic

but now i am wondering if there is a way to obtain the SlackChannelConfiguration topics? so somehow get the list of subscribed topics for a given SlackChannelConfiguration using CDK or CLI!

EDIT: typo

@sercantor
Copy link

@luiscastillocr I don't think it's been added, look at the last commits. https://github.com/aws/aws-cdk/commits/26dcc1e11a07d93681145049aa06d80a7d2114b9/packages/aws-cdk-lib/aws-chatbot/lib/slack-channel-configuration.ts

not sure even if it's possible, the resource is created, we'd be wanting to manipulate the resource we created in one stack in another stack.

an alternative to this would be to do Alarms -> SNS -> Lambda (provide your webhook url & stuff, write your custom message) -> Slack

@luiscastillocr
Copy link

hey @sercantor you are right, i was looking at the SlackChannelConfiguration construct when you actually were talking about the ISlackChannelConfiguration contructor, my apologies, in my case as a workaround i decided to create my own SlackConfiguration from CDK along with a custom Topic so I can have the ARN to send custom notifications from codebuild using the aws CLI

const events = [
            // actions
            'codepipeline-pipeline-action-execution-failed',
            'codepipeline-pipeline-action-execution-canceled',

            // stages
            'codepipeline-pipeline-stage-execution-resumed',
            'codepipeline-pipeline-stage-execution-canceled',
            'codepipeline-pipeline-stage-execution-failed',

            // pipeline
            'codepipeline-pipeline-pipeline-execution-failed',
            'codepipeline-pipeline-pipeline-execution-canceled',
            'codepipeline-pipeline-pipeline-execution-started',
            'codepipeline-pipeline-pipeline-execution-resumed',
            'codepipeline-pipeline-pipeline-execution-succeeded'
        ];

const slackChannelConfigurationTopic = new sns.Topic(this, slackChannelTopicName);

const slackChannelConfiguration = new chatbot.SlackChannelConfiguration(this, slackChannelConfigurationName, {
            slackChannelConfigurationName: slackChannelConfigurationName,
            slackWorkspaceId: slackChatbotWorkspaceId,
            slackChannelId: slackChannelId,
            notificationTopics: [snsTopic]
        });

const notificationRule = new code_star_notifications.NotificationRule(this,
            codestarNotificationRuleName,
            {
                events: events,
                source: pipeline.pipeline,
                targets: [slackChannelConfiguration]
            }
        );

notificationRule.node.addDependency(pipeline);

then i can do something like this in a CodeBuildStep command

aws sns publish --topic-arn "${SLACK_TOPIC_ARN}" --message file://cdk/lib/slack-msg.json

😉

@luiscastillocr
Copy link

well looks like I am going to need this method after all, happen that there are co-workers that have their slack channel pre-configured in their sanboxes so if someone have it pointing to the same slack channel ID, the SlackChannelConfiguration will throw an error saying the channel is already configured.

Sry, re-added my previous post, that you were referencing to.

This is how you can add SNS topics to an existing Slack config.

slack_bot = chatbot.SlackChannelConfiguration(
    chatbot.SlackChannelConfiguration.from_slack_channel_configuration_arn(
        self,
        "SlackNotifications",
        slack_channel_configuration_arn=(
            f"arn:aws:chatbot::{account}:chat-configuration/slack-channel/test-channel-config"
        ),
    )
)

slack_bot.add_notification_topic(sns_topic)
slack_bot.add_notification_topic(another_sns_topic)

@larskinder I don't think this is possible in NodeJS since you have to mandatory pass the SlackChannelConfigurationProps along with a name, so i think i am back to the waiting room for this feature!

@larskinder
Copy link

Hey, sorry for not getting back to you sooner. About your last question, I only tried it back in the day with Python, sadly never with TS. :(
I believe other ppl in this channel, when they write that it is not possible in TS. Out of curiosity and because I have to work with TS soon again, I might still take another look at it. ^^

@vaughan-rich
Copy link

+1

@YotillaAntoni
Copy link

Same issue here.

@GavinZZ
Copy link
Contributor

GavinZZ commented Jan 25, 2024

After team discussion, we are not able to support this feature.

fromSlackChannelConfigurationArn will import an existing slack channel configuration that was created using any means (i.e. from another stack, or manual creation, or etc.). If we now add addNotificationTopic method, it is essentially trying to modify this resource.

If this resource is created/managed using CloudFormation, this will cause CloudFormation stack drift, which should be avoided. If this resource is not managed by CloudFormation, it's impossible to modify the property.

In CDK, resources referenced via fromAbc method are read-only. The interfaces can only have methods that are read-only or that are creating a different resource.

@YotillaAntoni
Copy link

I guess as a workaround one could instead import a topic connected to the slack channel configuration with Topic.fromArn into CDK

@GavinZZ GavinZZ closed this as completed Jan 31, 2024
@GavinZZ GavinZZ closed this as not planned Won't fix, can't repro, duplicate, stale Jan 31, 2024
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

1 similar comment
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@GavinZZ GavinZZ added the wontfix We have determined that we will not resolve the issue. label Jan 31, 2024
@tallpaul-embark
Copy link

I understand not wanting to support the addNotificationTopic in some ways, but if that is the case I feel like we should not be locking down the new slack channel configuration to a channel only being allowed to be used once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-chatbot Related to AWS Chatbot effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 wontfix We have determined that we will not resolve the issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.