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

feat(stepfunctions-tasks): additional IAM statements for AWS SDK service integration #22070

Merged
merged 2 commits into from
Sep 21, 2022

Conversation

jogold
Copy link
Contributor

@jogold jogold commented Sep 16, 2022

Add a additionalIamStatements prop to pass additional IAM statements. To be used when the call requires more than a single statement to be executed.

Closes #22006


All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

…ice integration

Add a `additionalIamStatements` prop to pass additional IAM statements.
To be used when the call requires more than a single statement to be
executed.

Closes aws#22006
@gitpod-io
Copy link

gitpod-io bot commented Sep 16, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team September 16, 2022 08:20
@github-actions github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 labels Sep 16, 2022
@jogold
Copy link
Contributor Author

jogold commented Sep 16, 2022

@kaizencc

@Naumel
Copy link
Contributor

Naumel commented Sep 16, 2022

Hello,

Thank you for taking the time and submitting a PR.

Pointing the obvious before starting to look at the change, from the linter:
Error: Features must contain a change to an integration test file

@jogold
Copy link
Contributor Author

jogold commented Sep 16, 2022

Hi @Naumel

Given the changes here I would say this PR could have a pr-linter/exempt-integ-test...?

@TheRealAmazonKendra
Copy link
Contributor

Hi @Naumel

Given the changes here I would say this PR could have a pr-linter/exempt-integ-test...?

Can you provide more information about why you think this should be exempt from integ testing? We have recently increased our testing requirements and I am hesitant to provide exemptions except in the case where a convincing argument has been made.

I know you're a long time contributor (and one of our top, for that matter), so this may seem annoying, but we are implementing some new standards across the board to drive down our bug count as much as possible.

@jogold
Copy link
Contributor Author

jogold commented Sep 21, 2022

Can you provide more information about why you think this should be exempt from integ testing? We have recently increased our testing requirements and I am hesitant to provide exemptions except in the case where a convincing argument has been made.

@TheRealAmazonKendra The change here adds statements to an IAM policy. This is shown in the unit test. Updating an integ test snapshot will show the same "diff" as the unit test. As for the "deployable" part we already know that an IAM policy can be deployed.

What are you trying to cover with an extra integ test? "Deployability" or hidden changes to the CF template? If it is the latter then "snapshoting" stacks (expect(Template.fromStack(stack).toJSON()).toMatchSnapshot()) in the unit tests would lead to the same results (but without having to rely on another testing framework, also integ tests are adding tons of files to the repo)? I think that an integ test should mainly be used to check "deployability". I would say that the boundary between unit test and integ test is not clear in the CDK context.

Anyway, if you want me to add or update an integ test here, I will do it 🙂.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 7ecd2bb
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@corymhall corymhall added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Sep 21, 2022
@mergify
Copy link
Contributor

mergify bot commented Sep 21, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit fbb941f into aws:main Sep 21, 2022
homakk pushed a commit to homakk/aws-cdk that referenced this pull request Dec 1, 2022
…ice integration (aws#22070)

Add a `additionalIamStatements` prop to pass additional IAM statements. To be used when the call requires more than a single statement to be executed.

Closes aws#22006


----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-stepfunctions-tasks): CallAwsService to allow custom iam policyStatement
5 participants