fix(ecr): Repository.addToResourcePolicy returns incorrect result by watany-dev · Pull Request #21137 · aws/aws-cdk

watany-dev · 2022-07-14T11:21:09Z

fixes #12412

To fix Repository.addToResourcePolicy returning incorrect result (false-> true)

All Submissions:

Have you followed the guidelines in our Contributing guide?

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

gitpod-io · 2022-07-14T11:21:16Z

watany-dev · 2022-07-14T11:50:32Z

hmm… lets add test

TheRealAmazonKendra

Thank you for your contribution. We'll need tests for this change before we can provide a meaningful review. Please also ensure the title and body of the PR align to the guidelines in the Pull Request section of our contributing guide.

watany-dev · 2022-07-15T02:58:38Z

Thank you for your comment. As a starting point, I reviewed the title and text of the PR. I'm going to add a test

Pull request has been modified.

TheRealAmazonKendra · 2022-07-18T18:53:35Z

@Mergifyio update

mergify · 2022-07-18T18:54:38Z

update

✅ Branch has been successfully updated

TheRealAmazonKendra

I'd like to see a little bit more robust testing. Please see my comment below and let me know if you have any questions. Once you've made the change and/or if you have questions, please assign this review to me so I can follow up quickly.

TheRealAmazonKendra · 2022-07-18T19:25:34Z

packages/@aws-cdk/aws-ecr/test/repository.test.ts

      })).toThrow(/must follow the specified pattern/);
    });
+
+    test('return value addToResourcePolicy', () => {


Thanks for adding this test. Let's also add a test that looks at the actual output of calling this on a policy statement with concrete values. One test where the policy document is already defined and one where it is not.

ok. Update to a test that checks all return values, not just singular

TheRealAmazonKendra · 2022-07-18T19:55:06Z

Did this build locally? I'm seeing that the build hangs in the same spot each time I run it in CodeBuild.

TheRealAmazonKendra · 2022-07-18T21:07:57Z

@Mergifyio update

mergify · 2022-07-18T21:08:33Z

update

✅ Branch has been successfully updated

TheRealAmazonKendra · 2022-07-18T21:22:28Z

OK, so I pulled this down locally to run and see what the result was and I'm getting this

(node:60431) UnhandledPromiseRejectionWarning: TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'Object'
    --- property 'grantPrincipal' closes the circle
    at stringify (<anonymous>)
    at writeChannelMessage (internal/child_process/serialization.js:127:20)
    at process.target._send (internal/child_process.js:836:17)
    at process.target.send (internal/child_process.js:734:19)
    at reportSuccess (/Volumes/workplace/aws-cdk/node_modules/jest-worker/build/workers/processChild.js:59:11)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:60431) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 2)
(node:60431) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Pull request has been modified.

TheRealAmazonKendra

Looks good, just one more thing: Can you add one more test where there is already a policy document and test against the full output of that as well?

Pull request has been modified.

watany-dev · 2022-07-20T14:28:47Z

Looks good, just one more thing: Can you add one more test where there is already a policy document and test against the full output of that as well?

@TheRealAmazonKendra
I tried to fix the test. I hope there is a corresponding image.

TheRealAmazonKendra

Looks good, just one more thing: Can you add one more test where there is already a policy document and test against the full output of that as well?

@TheRealAmazonKendra I tried to fix the test. I hope there is a corresponding image.

That test is great. I'd like a second test, though, where there is an already existing policy statement within the policy document and that the entire policy document is as expected after performing the mutation on it with const artifact = new ecr.Repository(stack, 'Repo1').addToResourcePolicy(policyStmt); Your test shows that it adds the statement to an empty policy document properly, but let's make sure it also adds it correctly to one with content already in it.

Pull request has been modified.

mergify · 2022-07-22T16:44:12Z

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

aws-cdk-automation · 2022-07-22T17:19:37Z

AWS CodeBuild CI Report

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

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

mergify · 2022-07-22T17:20:01Z

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

…s#21137) fixes aws#12412 To fix Repository.addToResourcePolicy returning incorrect result (false-> true) ---- ### 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* ----

fix(＃12412) return value

3577f12

aws-cdk-automation requested a review from a team July 14, 2022 11:21

github-actions bot added p2 bug This issue is a bug. effort/medium Medium work item – several days of effort labels Jul 14, 2022

TheRealAmazonKendra previously requested changes Jul 14, 2022

View reviewed changes

watany-dev changed the title ~~fix(ecr) Repository.addToResourcePolicy returns incorrect result #12412~~ fix(ecr): Repository.addToResourcePolicy returns incorrect result Jul 15, 2022

Merge branch 'aws:main' into fix-ecr-result

873ef0a

TheRealAmazonKendra previously requested changes Jul 18, 2022

View reviewed changes

watany-dev force-pushed the fix-ecr-result branch from 9b11003 to e78f43e Compare July 19, 2022 12:16

watany-dev requested a review from TheRealAmazonKendra July 19, 2022 14:23

TheRealAmazonKendra previously requested changes Jul 19, 2022

View reviewed changes

watany-dev requested a review from TheRealAmazonKendra July 20, 2022 14:28

TheRealAmazonKendra previously requested changes Jul 20, 2022

View reviewed changes

watany-dev force-pushed the fix-ecr-result branch from 8937dd6 to 0483ae2 Compare July 22, 2022 10:42

watany-dev force-pushed the fix-ecr-result branch from 0483ae2 to d90fc04 Compare July 22, 2022 10:55

watany-dev added 2 commits July 22, 2022 11:05

add ecr method(.addToResourcePolicy) tests

e3191b0

Merge branch 'main' into fix-ecr-result

291351d

watany-dev force-pushed the fix-ecr-result branch from d90fc04 to 291351d Compare July 22, 2022 11:11

TheRealAmazonKendra approved these changes Jul 22, 2022

View reviewed changes

Merge branch 'main' into fix-ecr-result

12d28d8

mergify bot merged commit 5435215 into aws:main Jul 22, 2022

watany-dev deleted the fix-ecr-result branch July 26, 2022 05:19

Conversation

watany-dev commented Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Adding new Unconventional Dependencies:

New Features

Uh oh!

gitpod-io bot commented Jul 14, 2022

Uh oh!

watany-dev commented Jul 14, 2022

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

watany-dev commented Jul 15, 2022

Uh oh!

TheRealAmazonKendra commented Jul 18, 2022

Uh oh!

mergify bot commented Jul 18, 2022

✅ Branch has been successfully updated

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

watany-dev Jul 18, 2022

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra commented Jul 18, 2022

Uh oh!

TheRealAmazonKendra commented Jul 18, 2022

Uh oh!

mergify bot commented Jul 18, 2022

✅ Branch has been successfully updated

Uh oh!

TheRealAmazonKendra commented Jul 18, 2022

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

watany-dev commented Jul 20, 2022

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jul 22, 2022

Uh oh!

aws-cdk-automation commented Jul 22, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jul 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

watany-dev commented Jul 14, 2022 •

edited

Loading