feat(stepfunctions-tasks): add optional role property to EvaluateExpressionProps #29288

lagroujl · 2024-02-28T00:08:06Z

Issue # (if applicable)

Reason for this change

This change enables customers who need more control over IAM roles and whose environments do not allow them to use auto-generated roles to use EvaluateExpression.

Description of changes

Added a role property to EvaluateExpressionProps that allows customers to bring their own role. This would replace the role that is auto-generated by SingletonFunction when creating the Lambda Function used inside EvaluateExpression.

Description of how you validated changes

Added new unit test that validates created Function uses role created externally

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

lagroujl · 2024-02-28T15:15:53Z

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/evaluate-expression.ts

+  }
+
+  if (props.role) {
+    uuid += props.role.node.addr;


I am not totally sure if this is the right way to go about this. But the point is, I want the role to be part of the signature of the function created, so if I want to use different roles in one part of the stack or don't set a role at all, it will create different functions, instead of the first occurrence blocking all of the others.

Is there a risk of this uuid being too long?

I could not find one documented. Also, addr should have a fixed length of 42. SingletonFunction eventually passes it to tryFindChild. So that is where a length constraint could potentially come up, but otherwise, I think the risk is very low/none.

More likely, what I'm worried about is this addr value changing between deployments and deleting/recreating the function. I'm not sure I understand how the value of addr is calculated well enough to guarantee that won't happen

Are you able to test that? Does it change?

What is the value of props.role.node.addr? Could we use the props.role.roleName/roleArn instead?

What is the value of props.role.node.addr?

https://docs.aws.amazon.com/cdk/api/v2/docs/constructs.Node.html#addr

tl:dr; SHA-1 of the components of the construct path.

Could we use the props.role.roleName/roleArn instead?

No. this could be a token and change between deployments.

Were you able to test if the value of props.role.node.addr changes between deployments? From the description, Addresses are calculated using a SHA-1 of the components of the construct path., I'm not confident that it would be the same between deployments.

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/evaluate-expression.ts

paulhcsun · 2024-04-26T00:17:06Z

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/evaluate-expression.ts

+  }
+
+  if (props.role) {
+    uuid += props.role.node.addr;


What is the value of props.role.node.addr? Could we use the props.role.roleName/roleArn instead?

paulhcsun

Just one question and docstring change.

aws-cdk-automation · 2024-05-24T00:09:31Z

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

lagroujl · 2024-05-29T18:34:06Z

What is the value of props.role.node.addr?

https://docs.aws.amazon.com/cdk/api/v2/docs/constructs.Node.html#addr

tl:dr; SHA-1 of the components of the construct path.

Could we use the props.role.roleName/roleArn instead?

No. this could be a token and change between deployments.

aws-cdk-automation · 2024-06-12T00:09:53Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

aws-cdk-automation · 2024-06-17T16:48:01Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 04d2a2d
Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2024-07-06T00:08:40Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2024-07-07T00:09:57Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2024-07-08T00:09:40Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2024-07-14T00:10:09Z

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2024-07-21T00:10:19Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

github-actions · 2024-07-21T00:10:38Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please 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.

aws-cdk-automation requested a review from a team February 28, 2024 00:08

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Feb 28, 2024

lagroujl changed the title ~~feat(stepfunctions-tasks): add optional role property to allow customers to bring their own role~~ feat(stepfunctions-tasks): add optional role property to EvaluateExpressionProps allow customers to bring their own role Feb 28, 2024

lagroujl changed the title ~~feat(stepfunctions-tasks): add optional role property to EvaluateExpressionProps allow customers to bring their own role~~ feat(stepfunctions-tasks): add optional role property to EvaluateExpressionProps to allow customers to bring their own role Feb 28, 2024

lagroujl changed the title ~~feat(stepfunctions-tasks): add optional role property to EvaluateExpressionProps to allow customers to bring their own role~~ feat(stepfunctions-tasks): add optional role property to EvaluateExpressionProps Feb 28, 2024

aws-cdk-automation previously requested changes Feb 28, 2024

View reviewed changes

lagroujl commented Feb 28, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 28, 2024

github-actions bot mentioned this pull request Mar 1, 2024

Monthly PR metrics report - Feb 2023 #29328

Closed

github-actions bot added the feature-request A feature should be added or improved. label Mar 4, 2024

lagroujl mentioned this pull request Mar 12, 2024

(stepfunctions-tasks): support bring your own role for Lambda created by EvaluteExpression #29350

Open

2 tasks

msambol reviewed Mar 20, 2024

View reviewed changes

packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/evaluate-expression.ts Show resolved Hide resolved

paulhcsun reviewed Apr 26, 2024

View reviewed changes

lagroujl force-pushed the stepfunctions-tasks-evaluate-expression-role branch 2 times, most recently from da1d06c to 6d82f21 Compare June 17, 2024 13:15

Reset to main branch and add back in chagnes

695a608

lagroujl force-pushed the stepfunctions-tasks-evaluate-expression-role branch from 6d82f21 to 695a608 Compare June 17, 2024 13:16

aws-cdk-automation previously requested changes Jun 17, 2024

View reviewed changes

Commit integration tests

abe1df2

lagroujl and others added 2 commits June 17, 2024 10:56

Fix unit tests that specified runtime explicitly

9199105

Merge branch 'main' into stepfunctions-tasks-evaluate-expression-role

04d2a2d

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jul 21, 2024

aws-cdk-automation closed this Jul 21, 2024

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stepfunctions-tasks): add optional role property to EvaluateExpressionProps #29288

feat(stepfunctions-tasks): add optional role property to EvaluateExpressionProps #29288

lagroujl commented Feb 28, 2024 •

edited

Loading

aws-cdk-automation left a comment

lagroujl Feb 28, 2024

msambol Mar 20, 2024

lagroujl Mar 20, 2024 •

edited

Loading

lagroujl Mar 20, 2024

msambol Mar 20, 2024

paulhcsun Apr 26, 2024 •

edited

Loading

lagroujl May 29, 2024

paulhcsun Jul 2, 2024

paulhcsun Apr 26, 2024 •

edited

Loading

paulhcsun left a comment

aws-cdk-automation commented May 24, 2024

lagroujl commented May 29, 2024

aws-cdk-automation commented Jun 12, 2024

aws-cdk-automation left a comment

aws-cdk-automation commented Jun 17, 2024

aws-cdk-automation commented Jul 6, 2024

aws-cdk-automation commented Jul 7, 2024

aws-cdk-automation commented Jul 8, 2024

aws-cdk-automation commented Jul 14, 2024

aws-cdk-automation commented Jul 21, 2024

github-actions bot commented Jul 21, 2024

feat(stepfunctions-tasks): add optional role property to EvaluateExpressionProps #29288

feat(stepfunctions-tasks): add optional role property to EvaluateExpressionProps #29288

Conversation

lagroujl commented Feb 28, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

lagroujl Feb 28, 2024

Choose a reason for hiding this comment

msambol Mar 20, 2024

Choose a reason for hiding this comment

lagroujl Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

lagroujl Mar 20, 2024

Choose a reason for hiding this comment

msambol Mar 20, 2024

Choose a reason for hiding this comment

paulhcsun Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

lagroujl May 29, 2024

Choose a reason for hiding this comment

paulhcsun Jul 2, 2024

Choose a reason for hiding this comment

paulhcsun Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

paulhcsun left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented May 24, 2024

lagroujl commented May 29, 2024

aws-cdk-automation commented Jun 12, 2024

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jun 17, 2024

AWS CodeBuild CI Report

aws-cdk-automation commented Jul 6, 2024

aws-cdk-automation commented Jul 7, 2024

aws-cdk-automation commented Jul 8, 2024

aws-cdk-automation commented Jul 14, 2024

aws-cdk-automation commented Jul 21, 2024

github-actions bot commented Jul 21, 2024

lagroujl commented Feb 28, 2024 •

edited

Loading

lagroujl Mar 20, 2024 •

edited

Loading

paulhcsun Apr 26, 2024 •

edited

Loading

paulhcsun Apr 26, 2024 •

edited

Loading