feat(aws-chatbot): Support L2 construct for SlackChannelConfiguration of chatbot. #9702

luckily · 2020-08-14T14:45:36Z

I am ready for the first run.

Support L2 construct for SlackChannelConfiguration of chatbot.

add L2 construct
add unit tests
add integration test
update package.json

Resolves: #9679

cc @skinny85

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

1. add L2 construct 2. add unit tests 3. add integration test 4. update package.json

mergify · 2020-08-14T14:46:01Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

packages/@aws-cdk/aws-chatbot/lib/slack-channel-configuration.ts

packages/@aws-cdk/aws-chatbot/test/slack-channel-configuration.test.ts

skinny85

Looks awesome @luckily , thanks so much for the contribution!

I have some comments, mainly around simplifying and removing some things from this PR, but in general looks great!

Thanks,
Adam

packages/@aws-cdk/aws-chatbot/lib/slack-channel-configuration.ts

skinny85 · 2020-08-14T22:30:44Z

packages/@aws-cdk/aws-chatbot/lib/slack-channel-configuration.ts

+/**
+ * Represents a Slack channel configuration
+ */
+export interface ISlackChannelConfiguration extends cdk.IResource {


I have to say, I don't love the name SlackChannelConfiguration. Yes, I understand that the L1 is called like that, but I feel like we can do better.

How does everyone feel about SlackIntegration? @luckily @humanzz @Stacy-D ?

Agree, the shorter is better.

Shorter is nicer... but wouldn't that break consistency in naming?

Shorter is nicer... but wouldn't that break consistency in naming?

Do you mean consistency with CloudFormation?

packages/@aws-cdk/aws-chatbot/lib/slack-channel-configuration.ts

packages/@aws-cdk/aws-chatbot/test/slack-channel-configuration.test.ts

Pull request has been modified.

1. change default `loggingLevel` from `LoggingLevel.NONE` to `undefined`. 2. extract `new cdk.Stack()` to a `beforeEach()`. 3. change from `toMatch` to `haveResourceLike` for unit test asserting method. 4. add a test case to increase code coverage.

1. remove all prefix of @aws-cdk/assert 2. test case `import` move to import-slack-channel-configuration.test

1. rename configurationName to slackChannelConfigurationName for keep it consistent with slackChannelConfigurationArn. 2. adjust method chain for better way. 3. use `props.notificationTopics?.map(topic => topic.topicArn)` is better. 4. add a test case.

luckily · 2020-08-15T18:38:12Z

packages/@aws-cdk/aws-chatbot/lib/slack-channel-configuration.ts

+       * The ArnComponents API will return `slack-channel/my-slack`
+       * So i need to handle that to gets a correct name.`my-slack`
+       */
+      readonly slackChannelConfigurationName = (() => {


@skinny85 how about it?

packages/@aws-cdk/aws-chatbot/lib/slack-channel-configuration.ts

1. change addToPrincipalPolicy of method name to addToRolePolicy for consistency 2. addXPermissions() change to use this.addToRolePolicy() for avoiding throw error. 3. ISlackChannelConfiguration extend iam.IGrantable and implementing. 4. add test case for addXPermissions() methods of imported slack channel configuration.

luckily · 2020-08-18T13:29:12Z

Hi @skinny85, @humanzz and @Stacy-D

Thanks for your feedbacks.

I already resolved some problems by your feedbacks, but we still have issue about the naming, my opinion is consistency is important in large project, SlackIntegration is also great, but probably SlackIntegration is more suitable for L3 construct or patterns.

It decided by aws-cdk team.

skinny85

Nice! This is a great start for this module @luckily . Thanks for the contribution!

mergify · 2020-08-19T21:33:21Z

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

skinny85 · 2020-08-19T22:46:54Z

@luckily I think some additional linter rules were added that made the build fail. Can you update from origin/master, and fix the problems?

Pull request has been modified.

luckily · 2020-08-20T11:07:12Z

@skinny85 The problem already fixed.

bryan-hunter

💯

jogold · 2020-08-28T11:36:02Z

packages/@aws-cdk/aws-chatbot/lib/slack-channel-configuration.ts

+       * The ArnComponents API will return `slack-channel/my-slack`
+       * So i need to handle that to gets a correct name.`my-slack`
+       */
+      readonly slackChannelConfigurationName = (() => {


readonly get slackChannelConfigurationName() instead of an IIFE here?

@jogold
This way will not throw error when fromSlackChannelConfigurationArn static method called.
Would you want to throw error in slackChannelConfigurationName(assessor getter) of SlackChannelConfiguration?

I'm pretty sure @jogold meant this:

public get slackChannelConfigurationName(): string { const re = /^slack-channel\//; const resourceName = cdk.Stack.of(scope).parseArn(slackChannelConfigurationArn).resourceName as string; if (!re.test(resourceName)) { throw new Error('The ARN of a Slack integration must be in the form: arn:aws:chatbot:accountId:chat-configuration/slack-channel/slackChannelName'); } return resourceName.substring('slack-channel/'.length); }

Actually, something like this:

public static fromSlackChannelConfigurationArn(scope: cdk.Construct, id: string, slackChannelConfigurationArn: string): ISlackChannelConfiguration { const resourceName = cdk.Stack.of(scope).parseArn(slackChannelConfigurationArn).resourceName as string; if (!resourceName) { throw new Error('The ARN of a Slack integration must be in the form: arn:aws:chatbot:accountId:chat-configuration/slack-channel/slackChannelName'); } class Import extends SlackChannelConfigurationBase { readonly slackChannelConfigurationArn = slackChannelConfigurationArn; readonly role?: iam.IRole = undefined; readonly grantPrincipal: iam.IPrincipal; /* * For example: arn:aws:chatbot::1234567890:chat-configuration/slack-channel/my-slack * The ArnComponents API will return `slack-channel/my-slack` * So i need to handle that to gets a correct name.`my-slack` */ readonly slackChannelConfigurationName = resourceName.substring('slack-channel/'.length);; // ... } return new Import(scope, id); }

would be even better.

@jogold @skinny85
I adjusted.

1. adjust error description. 2. remove the IIFE way.

Merge branch 'master' into feat/aws-chatbot

skinny85

Thanks a lot for the great contribution @luckily !

mergify · 2020-09-01T21:41:38Z

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 · 2020-09-01T22:20:21Z

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 · 2020-09-01T22:20:22Z

AWS CodeBuild CI Report

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

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

L2 constructs were introduced last month in #9702 for slack channel configuration. marking the module as experimental to reflect that the module is no longer cfn-only.

…n-only (#10880) L2 constructs were introduced last month in #9702 for slack channel configuration. marking the module as experimental to reflect that the module is no longer cfn-only. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Support L2 construct for SlackChannelConfiguration of chatbot.

71888fe

1. add L2 construct 2. add unit tests 3. add integration test 4. update package.json

update README

a42112a

luckily changed the title ~~feat: Support L2 construct for SlackChannelConfiguration of chatbot.~~ feat(aws-chatbot): Support L2 construct for SlackChannelConfiguration of chatbot. Aug 14, 2020

skinny85 mentioned this pull request Aug 14, 2020

[aws-chatbot] L2 support for SlackChannelConfiguration #9679

Closed

2 tasks

humanzz reviewed Aug 14, 2020

View reviewed changes

packages/@aws-cdk/aws-chatbot/lib/slack-channel-configuration.ts Outdated Show resolved Hide resolved

humanzz reviewed Aug 14, 2020

View reviewed changes

packages/@aws-cdk/aws-chatbot/lib/slack-channel-configuration.ts Outdated Show resolved Hide resolved

humanzz reviewed Aug 14, 2020

View reviewed changes

packages/@aws-cdk/aws-chatbot/lib/slack-channel-configuration.ts Show resolved Hide resolved

Stacy-D reviewed Aug 14, 2020

View reviewed changes

packages/@aws-cdk/aws-chatbot/lib/slack-channel-configuration.ts Outdated Show resolved Hide resolved

Stacy-D reviewed Aug 14, 2020

View reviewed changes

packages/@aws-cdk/aws-chatbot/test/slack-channel-configuration.test.ts Show resolved Hide resolved

SomayaB assigned MrArnoldPalmer Aug 14, 2020

skinny85 previously requested changes Aug 14, 2020

View reviewed changes

rename configurationRole to role

c60d82d

luckily added 5 commits August 15, 2020 22:58

remove the double !

d94a1c8

rename configurationRole to role

1af27b5

Adjust unit tests

0786017

1. remove all prefix of @aws-cdk/assert 2. test case `import` move to import-slack-channel-configuration.test

Enhanced to better

e7a2d16

1. rename configurationName to slackChannelConfigurationName for keep it consistent with slackChannelConfigurationArn. 2. adjust method chain for better way. 3. use `props.notificationTopics?.map(topic => topic.topicArn)` is better. 4. add a test case.

luckily commented Aug 15, 2020

View reviewed changes

luckily added 2 commits August 17, 2020 22:56

remove addXPermissions() methods

733e6ae

skinny85 previously approved these changes Aug 19, 2020

View reviewed changes

Merge branch 'master' into feat/aws-chatbot

8a88e33

luckily added 2 commits August 20, 2020 12:47

Merge branch 'master' into feat/aws-chatbot

136921e

fix linter problem

26c7a27

bryan-hunter approved these changes Aug 27, 2020

View reviewed changes

jogold reviewed Aug 28, 2020

View reviewed changes

luckily and others added 4 commits September 1, 2020 12:52

Enhanced fromSlackChannelConfigurationArn() static method

cc1c6e2

1. adjust error description. 2. remove the IIFE way.

update from origin/master

93f5b49

Merge branch 'master' into feat/aws-chatbot

upgrade constructs to ^3.0.4

b752640

Merge branch 'master' into feat/aws-chatbot

4ac2a7e

skinny85 approved these changes Sep 1, 2020

View reviewed changes

Merge branch 'master' into feat/aws-chatbot

d2cac48

mergify bot merged commit 05f5e62 into aws:master Sep 1, 2020

shivlaks mentioned this pull request Oct 15, 2020

chore(chatbot): update module stability to experimental instead of cfn-only #10880

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-chatbot): Support L2 construct for SlackChannelConfiguration of chatbot. #9702

feat(aws-chatbot): Support L2 construct for SlackChannelConfiguration of chatbot. #9702

luckily commented Aug 14, 2020

mergify bot commented Aug 14, 2020

skinny85 left a comment

skinny85 Aug 14, 2020

luckily Aug 15, 2020

humanzz Aug 17, 2020

skinny85 Aug 17, 2020

luckily Aug 15, 2020

luckily commented Aug 18, 2020

skinny85 left a comment

mergify bot commented Aug 19, 2020

skinny85 commented Aug 19, 2020

luckily commented Aug 20, 2020

bryan-hunter left a comment

jogold Aug 28, 2020

luckily Aug 31, 2020

skinny85 Sep 1, 2020 •

edited

skinny85 Sep 1, 2020 •

edited

luckily Sep 1, 2020

skinny85 left a comment •

edited

mergify bot commented Sep 1, 2020

mergify bot commented Sep 1, 2020

aws-cdk-automation commented Sep 1, 2020

feat(aws-chatbot): Support L2 construct for SlackChannelConfiguration of chatbot. #9702

feat(aws-chatbot): Support L2 construct for SlackChannelConfiguration of chatbot. #9702

Conversation

luckily commented Aug 14, 2020

mergify bot commented Aug 14, 2020

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luckily commented Aug 18, 2020

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 19, 2020

skinny85 commented Aug 19, 2020

luckily commented Aug 20, 2020

bryan-hunter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 Sep 1, 2020 • edited

Choose a reason for hiding this comment

skinny85 Sep 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 left a comment • edited

Choose a reason for hiding this comment

mergify bot commented Sep 1, 2020

mergify bot commented Sep 1, 2020

aws-cdk-automation commented Sep 1, 2020

AWS CodeBuild CI Report

skinny85 Sep 1, 2020 •

edited

skinny85 Sep 1, 2020 •

edited

skinny85 left a comment •

edited