chore(aws-cdk-readme): replace deprecated method used in aws-chatbot README.md #13521

BLasan · 2021-03-10T11:28:41Z

Currently addLambdaInvokeCommandPermissions method used to get the permissions,
which is a deprecated method now.
Use addToPolicy method to get necessary permissions

fix: #13444

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

gitpod-io · 2021-03-10T11:28:43Z

mergify · 2021-03-10T11:29:19Z

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

MrArnoldPalmer · 2021-03-11T18:47:04Z

packages/@aws-cdk/aws-chatbot/README.md

 slackChannel.addNotificationPermissions();
 slackChannel.addSupportCommandPermissions();
 slackChannel.addReadOnlyCommandPermissions();


We can remove these lines as the are also deprecated.

MrArnoldPalmer · 2021-03-11T18:50:07Z

packages/@aws-cdk/aws-chatbot/README.md

+const invokeCommandStatement = new iam.PolicyStatement({
+ effect: iam.Effect.ALLOW,
+ resources: ["RESOURCE_ARNS_ALLOWED"],
+ actions: ["INVOKING_ACTION_ALLOWED"],
+});


I think we can document this clearer and encourage a more idiomatic usage with the grantX methods on corresponding resources. IE, if we change this to.

const lambdaFn = new lambda.Function(this, 'MyChatbotHandler', { code: lambda.Code.fromAsset('./code'), runtime: lambda.Runtime.NODE_JS_14_X, }); lambdaFn.grantInvoke(slackChannel.role);

@BLasan do you agree?

Yuh, I think this would be ideal. But we need to add the permissions to the role first right?

so lambdaFn.grantInvoke(slackChannel.role); should handle all of that. This should be the only grant statement needed for the chatbot to invoke the function.

MrArnoldPalmer · 2021-03-11T18:50:50Z

packages/@aws-cdk/aws-chatbot/README.md

-slackChannel.addLambdaInvokeCommandPermissions();
+const invokeCommandStatement = new iam.PolicyStatement({
+ effect: iam.Effect.ALLOW,
+ resources: ["RESOURCE_ARNS_ALLOWED"],


NitPick: use single quotes to match the general repo style. (I prefer double but what can ya do 🤷🏻‍♂️ )

My bad. Will change this :)

Pull request has been modified.

BLasan · 2021-03-11T19:17:26Z

@MrArnoldPalmer Updated

MrArnoldPalmer · 2021-03-11T19:18:46Z

packages/@aws-cdk/aws-chatbot/README.md

+const invokeCommandStatement = new iam.PolicyStatement({
+ effect: iam.Effect.ALLOW,
+ resources: ['RESOURCE_ARNS_ALLOWED'],
+ actions: ['INVOKING_ACTION_ALLOWED'],
+});
+
+slackChannel.addToRolePolicy(invokeCommandStatement);


the lambda grant invoke removes the need for this.

@MrArnoldPalmer Updated. please review

Pull request has been modified.

MrArnoldPalmer · 2021-03-11T19:38:04Z

packages/@aws-cdk/aws-chatbot/README.md

+const lambdaFn = new lambda.Function(this, 'MyChatbotHandler', {
+ code: lambda.Code.fromAsset('./code'),
+ runtime: lambda.Runtime.NODE_JS_14_X,
+ handler: 'FILE_NAME.handler',
+});
+lambdaFn.grantInvoke(slackChannel.role)


Actually I'm not sure if this is needed here. I misinterpreted the point of the previous calls I believe. I'm not sure if addLambdaInvokeCommandPermissions is actually meant to grant access to a single specific lambda function. These aren't managed policies but just templates that the chatbot console will create a role with.

Perhaps we should just remove the usage of the methods that don't exist for now.

Yuh, that method is for granting permissions in order to access a particular lambda function. Should we remove this?

yeah lets remove this and the only changes from the original will be removing the calls to the deprecated methods. I do think users should be granting access to specific lambdas, cloudwatch metrics, etc and not using * resources. This however should be demonstrated in a more specific example.

Pull request has been modified.

BLasan · 2021-03-11T20:04:21Z

@MrArnoldPalmer Looks good?

MrArnoldPalmer · 2021-03-11T20:10:42Z

packages/@aws-cdk/aws-chatbot/README.md

+const invokeCommandStatement = new iam.PolicyStatement({
+ effect: iam.Effect.ALLOW,
+ resources: ['RESOURCE_ARNS_ALLOWED'],
+ actions: ['INVOKING_ACTION_ALLOWED'],
+});
+
+slackChannel.addToRolePolicy(invokeCommandStatement);


I think we should remove this as well. If you want to include examples about adding template policies, it should be done in a separate snippet with corresponding documentation in a different PR.

Ok, will remove this as well :)

@MrArnoldPalmer Made the requested changes. Please review

…README.md Currently addLambdaInvokeCommandPermissions method used to get the permissions, which is a deprecated method now. Use addToPolicy method to get necessary permissions.

Pull request has been modified.

aws-cdk-automation · 2021-03-12T10:44:21Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 3fddf76
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2021-03-12T10:44:43Z

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

BLasan changed the title ~~Replace deprecated method addLambdaInvokeCommandPermissions with addToPolicy~~ fix: deprecated method addLambdaInvokeCommandPermissions Mar 10, 2021

BLasan force-pushed the issue-13444 branch 3 times, most recently from 0f547f8 to baf1ee4 Compare March 10, 2021 11:38

BLasan changed the title ~~fix: deprecated method addLambdaInvokeCommandPermissions~~ chore(aws-cdk-readme): replace deprecated method used in aws-chatbot README.md Mar 10, 2021

github-actions bot added the @aws-cdk/aws-chatbot Related to AWS Chatbot label Mar 10, 2021

github-actions bot assigned MrArnoldPalmer Mar 10, 2021

MrArnoldPalmer previously requested changes Mar 11, 2021

View reviewed changes

BLasan force-pushed the issue-13444 branch from 46dd0e7 to d1aa705 Compare March 11, 2021 19:16

MrArnoldPalmer previously requested changes Mar 11, 2021

View reviewed changes

BLasan force-pushed the issue-13444 branch from d1aa705 to 0677592 Compare March 11, 2021 19:31

MrArnoldPalmer previously requested changes Mar 11, 2021

View reviewed changes

BLasan force-pushed the issue-13444 branch from 0677592 to 127718a Compare March 11, 2021 20:03

BLasan force-pushed the issue-13444 branch from 127718a to 8c74893 Compare March 11, 2021 20:04

MrArnoldPalmer previously requested changes Mar 11, 2021

View reviewed changes

chore(aws-cdk-readme): replace deprecated method used in aws-chatbot …

aa17b3c

…README.md Currently addLambdaInvokeCommandPermissions method used to get the permissions, which is a deprecated method now. Use addToPolicy method to get necessary permissions.

BLasan force-pushed the issue-13444 branch from 8c74893 to aa17b3c Compare March 11, 2021 20:13

MrArnoldPalmer previously approved these changes Mar 11, 2021

View reviewed changes

Merge branch 'master' into issue-13444

3fddf76

rix0rrr approved these changes Mar 12, 2021

View reviewed changes

mergify bot merged commit 4769b31 into aws:master Mar 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(aws-cdk-readme): replace deprecated method used in aws-chatbot README.md #13521

chore(aws-cdk-readme): replace deprecated method used in aws-chatbot README.md #13521

BLasan commented Mar 10, 2021 •

edited by MrArnoldPalmer

Loading

gitpod-io bot commented Mar 10, 2021 •

edited

Loading

mergify bot commented Mar 10, 2021

MrArnoldPalmer Mar 11, 2021

MrArnoldPalmer Mar 11, 2021

BLasan Mar 11, 2021 •

edited

Loading

MrArnoldPalmer Mar 11, 2021

BLasan Mar 11, 2021

MrArnoldPalmer Mar 11, 2021

BLasan Mar 11, 2021

BLasan commented Mar 11, 2021

MrArnoldPalmer Mar 11, 2021

BLasan Mar 11, 2021

MrArnoldPalmer Mar 11, 2021

BLasan Mar 11, 2021

MrArnoldPalmer Mar 11, 2021

BLasan commented Mar 11, 2021

MrArnoldPalmer Mar 11, 2021

BLasan Mar 11, 2021

BLasan Mar 11, 2021

aws-cdk-automation commented Mar 12, 2021

mergify bot commented Mar 12, 2021

chore(aws-cdk-readme): replace deprecated method used in aws-chatbot README.md #13521

chore(aws-cdk-readme): replace deprecated method used in aws-chatbot README.md #13521

Conversation

BLasan commented Mar 10, 2021 • edited by MrArnoldPalmer Loading

gitpod-io bot commented Mar 10, 2021 • edited Loading

mergify bot commented Mar 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BLasan Mar 11, 2021 • edited Loading

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

BLasan commented Mar 11, 2021

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

BLasan commented Mar 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Mar 12, 2021

AWS CodeBuild CI Report

mergify bot commented Mar 12, 2021

BLasan commented Mar 10, 2021 •

edited by MrArnoldPalmer

Loading

gitpod-io bot commented Mar 10, 2021 •

edited

Loading

BLasan Mar 11, 2021 •

edited

Loading