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

Merged
merged 1 commit into from
Jul 28, 2021

Conversation

jagpreetsinghsasan
Copy link
Contributor

Commit to be reviewed

feat(google-sm): added keychain plugin for google secret manager

    Primary Change
    ---
    1. Added new package cactus-plugin-keychain-google-sm under packages/
    2. Added PluginKeychainGoogleSmMock class to mock the functions of SecretManagerServiceClient under packages/cactus-plugin-keychain-google-sm/src/test/typescript/mock/plugin-keychain-google-sm-mock.ts

Resolves #983

Signed-off-by: Jagpreet Singh Sasan [email protected]

@jagpreetsinghsasan jagpreetsinghsasan force-pushed the feature-983 branch 2 times, most recently from acae0de to f18535c Compare June 10, 2021 10:05
@jagpreetsinghsasan jagpreetsinghsasan marked this pull request as ready for review June 10, 2021 10:07
@jagpreetsinghsasan jagpreetsinghsasan force-pushed the feature-983 branch 2 times, most recently from 5cf5885 to c98d998 Compare June 11, 2021 05:29
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 apply the same changes to this one as for the Azure and the AWS plugins (see other 2 PRs for more info)

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.

@elenaizaguirre @jagpreetsinghsasan Same request here as I mentioned in the other PR, please coordinate the merging of your relevant PRs regarding the cross-spec references.

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

codecov-commenter commented Jul 7, 2021

Codecov Report

Merging #1017 (8f397d7) into main (43be168) will decrease coverage by 0.74%.
The diff coverage is 42.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1017      +/-   ##
==========================================
- Coverage   73.23%   72.49%   -0.75%     
==========================================
  Files         252      261       +9     
  Lines        8949     9172     +223     
  Branches     1044     1080      +36     
==========================================
+ Hits         6554     6649      +95     
- Misses       1835     1953     +118     
- Partials      560      570      +10     
Impacted Files Coverage Δ
...enerated/openapi/typescript-axios/configuration.ts 9.09% <9.09%> (ø)
...pescript/generated/openapi/typescript-axios/api.ts 14.00% <14.00%> (ø)
...cript/generated/openapi/typescript-axios/common.ts 23.91% <23.91%> (ø)
...escript/generated/openapi/typescript-axios/base.ts 41.66% <41.66%> (ø)
...m/src/main/typescript/plugin-keychain-google-sm.ts 60.78% <60.78%> (ø)
...-sm/src/main/typescript/plugin-factory-keychain.ts 66.66% <66.66%> (ø)
.../typescript/mock/plugin-keychain-google-sm-mock.ts 73.68% <73.68%> (ø)
...ychain-google-sm/src/main/typescript/public-api.ts 85.71% <85.71%> (ø)
...script/generated/openapi/typescript-axios/index.ts 100.00% <100.00%> (ø)
... and 8 more

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 43be168...8f397d7. Read the comment docs.

@petermetz petermetz added Keychain Tasks/bugs related to the Keychain plugin core interfaces or any of the implementations themselves. enhancement New feature or request labels Jul 8, 2021
@takeutak
Copy link
Contributor

takeutak commented Jul 10, 2021

@jagpreetsinghsasan Would you tell me how to position the codes included in this PR for review? (as the same question to #964, #991) cc: @petermetz

  • Is this package required features or optional ones on Cactus?
  • How do you intend to use the package on Cactus use-case?

@jagpreetsinghsasan
Copy link
Contributor Author

Hi @takeutak

Would you tell me how to position the codes included in this PR for review?

I didnt understand this statement, can you explain it?

Is this package required features or optional ones on Cactus?

This plugin is another keychain plugin, but for cloud vault (like Google secret manager in this case). As keychain plugins are optional to choose from depending upon client requirements, this plugin is also optional.

How do you intend to use the package on Cactus use-case?

The usage of this plugin is demonstrated in the test case (packages/cactus-plugin-keychain-google-sm/src/test/typescript/integration/plugin-keychain-google-sm.test.ts) (exactly like the vault keychain plugin, just with its own set of input requirements).

@takeutak
Copy link
Contributor

takeutak commented Jul 19, 2021

@jagpreetsinghsasan Thanks for your kind explanation. I understood.

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.

@jagpreetsinghsasan jagpreetsinghsasan force-pushed the feature-983 branch 2 times, most recently from bbce5d6 to 2a049a4 Compare July 22, 2021 04:07
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 Found a few more things last minute, please also apply these changes to the other two keychain plugin PRs in the queue. Sorry for not noticing these little tweaks earlier.

.cspell.json Outdated Show resolved Hide resolved
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

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 LGTM, please resolve conflict(s) by rebasing and then we are good to go.

@jagpreetsinghsasan
Copy link
Contributor Author

Sure, if this is merged after azure-kv, then it will not have merge conflicts, or else, there might be conflict again (I have added azure-kv to the tsconfig.json here as well)

	Primary Change
	---
	1. Added new package cactus-plugin-keychain-google-sm under packages/
	2. Added PluginKeychainGoogleSmMock class to mock the functions of SecretManagerServiceClient under
	packages/cactus-plugin-keychain-google-sm/src/test/typescript/mock/

Resolves hyperledger-cacti#983

Signed-off-by: jagpreetsinghsaan <[email protected]>
@petermetz petermetz merged commit 1419b2c into hyperledger-cacti:main Jul 28, 2021
@petermetz petermetz deleted the feature-983 branch July 28, 2021 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 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-google-sm): create google secret manager plugin
4 participants