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(aws-sm): added keychain plugin for aws secret manager #964

Merged
merged 1 commit into from
Jul 28, 2021

Conversation

jagpreetsinghsasan
Copy link
Contributor

Commit to be reviewed


feat(aws-sm): added keychain playing for aws secret manager

    Primary Change
    ---
    1. Added new package cactus-plugin-keychain-aws-sm under packages/
    2. Added LocalStack under cactus-test-tooling/src/main/typescript for local aws secret manager deployment and testing

    Refactorings that were also necessary to incorporate 1) and 2)
    ---
    3. Updated public-api.ts under packages/cactus-test-tooling/src/main/typescript for exporting the Localstack class, its interface and constants

Resolves #912

Signed-off-by: Jagpreet Singh Sasan jagpreet.singh.sasan@accenture.com

@petermetz petermetz added the Keychain Tasks/bugs related to the Keychain plugin core interfaces or any of the implementations themselves. label May 13, 2021
@jagpreetsinghsasan jagpreetsinghsasan force-pushed the feature-912 branch 4 times, most recently from a48d04c to 4d77763 Compare May 25, 2021 08:03
@jagpreetsinghsasan jagpreetsinghsasan force-pushed the feature-912 branch 3 times, most recently from 1e1e090 to 129c0a5 Compare June 14, 2021 04:21
@jagpreetsinghsasan jagpreetsinghsasan force-pushed the feature-912 branch 2 times, most recently from 543dc6e to ff29cce Compare June 15, 2021 09:46
@petermetz
Copy link
Contributor

@jagpreetsinghsasan Just double checking, is this ready for review?

@jagpreetsinghsasan
Copy link
Contributor Author

Yes, this is ready for review

@petermetz
Copy link
Contributor

Yes, this is ready for review

@jagpreetsinghsasan Got it, thanks. To speed things up next time, please use this: https://github.blog/changelog/2019-02-21-re-request-review-on-a-pull-request/

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jagpreetsinghsasan I see some things resolved in the comments but the code looks the same as before. Please take a second pass on all the review comments and then hit the re-request review.

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jagpreetsinghsasan @elenaizaguirre Same request here regarding the openapi.json cross-references. (see my other comment on the other keychain plugin PRs for details)

@petermetz petermetz requested a review from takeutak July 7, 2021 23:27
@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2021

Codecov Report

Merging #964 (a0a08eb) into main (539a801) will decrease coverage by 0.07%.
The diff coverage is 69.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #964      +/-   ##
==========================================
- Coverage   73.30%   73.23%   -0.08%     
==========================================
  Files         248      252       +4     
  Lines        8782     8949     +167     
  Branches     1025     1044      +19     
==========================================
+ Hits         6438     6554     +116     
- Misses       1796     1835      +39     
- Partials      548      560      +12     
Impacted Files Coverage Δ
...-sm/src/main/typescript/plugin-factory-keychain.ts 66.66% <66.66%> (ø)
...main/typescript/localstack/localstack-container.ts 66.66% <66.66%> (ø)
...s-sm/src/main/typescript/plugin-keychain-aws-sm.ts 68.36% <68.36%> (ø)
...-keychain-aws-sm/src/main/typescript/public-api.ts 85.71% <85.71%> (ø)
...tus-test-tooling/src/main/typescript/public-api.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 539a801...a0a08eb. Read the comment docs.

@takeutak
Copy link
Contributor

Would you wait my review since I'm asking about this PR on #1017?

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jagpreetsinghsasan Please update to all Cactus package version occurrences to 0.6.0 from 0.5.0 (just issued the release) and then run the configure script to also update the lock files as well. Commit all of those changes and then we are ready to merge. Don't forget the squash the commits into a single one.

packages/cactus-plugin-keychain-aws-sm/package.json Outdated Show resolved Hide resolved
packages/cactus-plugin-keychain-aws-sm/package-lock.json Outdated Show resolved Hide resolved
packages/cactus-plugin-keychain-aws-sm/package-lock.json Outdated Show resolved Hide resolved
packages/cactus-plugin-keychain-aws-sm/package.json Outdated Show resolved Hide resolved
packages/cactus-plugin-keychain-aws-sm/package.json Outdated Show resolved Hide resolved
packages/cactus-plugin-keychain-aws-sm/package.json Outdated Show resolved Hide resolved
packages/cactus-plugin-keychain-aws-sm/package.json Outdated Show resolved Hide resolved
@petermetz petermetz removed the request for review from jonathan-m-hamilton July 20, 2021 19:05
@jagpreetsinghsasan jagpreetsinghsasan force-pushed the feature-912 branch 5 times, most recently from 4b55297 to 59852da Compare July 22, 2021 11:46
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jagpreetsinghsasan Almost forgot, please also

  1. migrate the tsconfig.json inside your package to match the new boilerplate (see any of the other packages for an example).
  2. Add an entry to the root tsconfig.json file under the references array that points to the new package that is being added in the PR
  3. Do this to all the pending PRs that are adding new packages

        Primary Change
        ---
        1. Added new package cactus-plugin-keychain-aws-sm under packages/
        2. Added Localstack under cactus-test-tooling/src/main/typescript for
		local aws secret manager deployment and testing

		Refactorings that were also necessary to incorporate 1) and 2)
        ---
        3. Updated public-api.ts under packages/cactus-test-tooling/src/main/typescript
		for exporting LocalStack class, its interfaces and constants

Resolves hyperledger-cacti#912

Signed-off-by: jagpreetsinghsaan <jagpreet.singh.sasan@accenture.com>
@petermetz petermetz merged commit ed6db9e into hyperledger-cacti:main Jul 28, 2021
@petermetz petermetz deleted the feature-912 branch July 28, 2021 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Keychain Tasks/bugs related to the Keychain plugin core interfaces or any of the implementations themselves.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(keychain-aws-secretmanager): create aws secret manager plugin
4 participants