cdk-ecr-deployment icon indicating copy to clipboard operation
cdk-ecr-deployment copied to clipboard

Published 20 hours ago • verified publisher icon cdklabs

feat: Allow to use secret with key-value pairs

Open Hi-Fi opened this issue 3 years ago • 6 comments
trafficstars

BREAKING CHANGE: API to define credentials changed

Fixes #179

Hi-Fi avatar Jun 18 '22 12:06 Hi-Fi

Thanks for your PR, Is it ready for review?

wchaws avatar Jun 20 '22 02:06 wchaws

I'll try to check this in actual environment still today that it works as expected. So trying to get it ready for review today.

Hi-Fi avatar Jun 20 '22 05:06 Hi-Fi

Some issues that needs to be checked still with Golang, so might take a while to find time to focus on those.

Hi-Fi avatar Jun 20 '22 18:06 Hi-Fi

@wchaws Now Lambda seemed to work as should, so I think this is ready to be reviewed. Also build works in Docker.

Hi-Fi avatar Jun 21 '22 18:06 Hi-Fi

@wchaws Do you have had time to check this?

Hi-Fi avatar Jun 27 '22 15:06 Hi-Fi

@wchaws how about this?

Hi-Fi avatar Aug 04 '22 09:08 Hi-Fi

@wchaws Do you have had time to check this?

@Hi-Fi Sorry for the late reply. I was so busy for the last few months and didn't have time to review your PR until now. Overall, your PR looks great except missing some unit test for main.go(I know it might be hard to test main.go, since main.go depends on SecretsManager).

wchaws avatar Sep 04 '22 09:09 wchaws

@wchaws Do you have had time to check this?

@Hi-Fi Sorry for the late reply. I was so busy for the last few months and didn't have time to review your PR until now. Overall, your PR looks great except missing some unit test for main.go(I know it might be hard to test main.go, since main.go depends on SecretsManager).

If remember correctly, I checked unit tests for that but it would require some refactoring to mock those secrets manager calls. But I can try to recheck those.

I think reflection is needed, and needs to be done also for S3 as currently main tests are skipped also due to that.

Hi-Fi avatar Sep 04 '22 15:09 Hi-Fi

@wchaws I added testing for secretsmanager now. I didn't add mocking to existing tests that were skipped, even same approach could be used for those to mock out e.g. s3.

Hi-Fi avatar Sep 06 '22 17:09 Hi-Fi

Hi @Hi-Fi, First, thanks for your contribution. But since this PR is blocking the sub-sequential release, I think I have to revert this commit. Besides that, I've been getting security alerts for quite some time now. Since our company mandates that these security vulnerabilities be solved, I think there are some issues with the current design. I will re-propose a new design to avoid receiving a lot of security vulnerabilities from go dependencies. The main idea is to download the skopeo cli tool directly instead of building go projects from scratch. I hope you can understand!

#222

wchaws avatar Feb 15 '23 06:02 wchaws