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(azure-kv): added keychain plugin for azure keyvault #991

Merged
merged 2 commits into from
Jul 29, 2021

Conversation

jagpreetsinghsasan
Copy link
Contributor

@jagpreetsinghsasan jagpreetsinghsasan commented May 27, 2021

Commit to be reviewed


feat(azure-kv): added keychain plugin for azure keyvault

    Primary Change
    ---
    1. Added new package cactus-plugin-keychain-azure-kv under packages/
    2. Added PluginKeychainAzureKvMock class to mock the functions of SecretClient under packages/cactus-plugin-keychain-azure-kv/src/test/typescript/mock/plugin-keychain-azure-kv-mock.ts

Resolves #971

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

@jagpreetsinghsasan jagpreetsinghsasan force-pushed the feature-971 branch 3 times, most recently from b3748f1 to aec199e Compare June 1, 2021 07:21
@petermetz
Copy link
Contributor

@jagpreetsinghsasan See also this for a good example on mocking for unit tests: https://github.com/petermetz/cactus/tree/feat-object-store-plugin-ipfs

packages/cactus-plugin-object-store-ipfs/src/test/typescript/fixtures/mock/ipfs/ipfs-files-api-mock.ts
packages/cactus-plugin-object-store-ipfs/src/test/typescript/fixtures/mock/ipfs/ipfs-http-client-mock.ts
packages/cactus-plugin-object-store-ipfs/src/main/typescript/plugin-object-store-ipfs.ts

@jagpreetsinghsasan jagpreetsinghsasan force-pushed the feature-971 branch 5 times, most recently from 3a06dc8 to b58059a Compare June 11, 2021 08:21
@petermetz
Copy link
Contributor

@jagpreetsinghsasan Should I give this one another go? (I see the code changes but you haven't yet clicked 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.

@elenaizaguirre @jagpreetsinghsasan Please coordinate your PRs merging in order to make sure that the openapi.json spec references are updated "at the right time" depending on who's PR gets merged first.

Context: Elena is working on a change that necessitates for us to use the http URLs instead of relative file paths.

@petermetz petermetz requested a review from takeutak July 7, 2021 21:33
@petermetz petermetz added the Keychain Tasks/bugs related to the Keychain plugin core interfaces or any of the implementations themselves. label Jul 7, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jul 7, 2021

Codecov Report

Merging #991 (02f7302) into main (c6057a7) will decrease coverage by 1.59%.
The diff coverage is 49.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #991      +/-   ##
==========================================
- Coverage   73.30%   71.71%   -1.60%     
==========================================
  Files         248      270      +22     
  Lines        8782     9411     +629     
  Branches     1025     1114      +89     
==========================================
+ Hits         6438     6749     +311     
- Misses       1796     2087     +291     
- Partials      548      575      +27     
Impacted Files Coverage Δ
...enerated/openapi/typescript-axios/configuration.ts 9.09% <9.09%> (ø)
...enerated/openapi/typescript-axios/configuration.ts 9.09% <9.09%> (ø)
...pescript/generated/openapi/typescript-axios/api.ts 14.00% <14.00%> (ø)
...pescript/generated/openapi/typescript-axios/api.ts 14.00% <14.00%> (ø)
...cript/generated/openapi/typescript-axios/common.ts 23.91% <23.91%> (ø)
...cript/generated/openapi/typescript-axios/common.ts 23.91% <23.91%> (ø)
...escript/generated/openapi/typescript-axios/base.ts 41.66% <41.66%> (ø)
...escript/generated/openapi/typescript-axios/base.ts 41.66% <41.66%> (ø)
...kv/src/main/typescript/plugin-keychain-azure-kv.ts 55.38% <55.38%> (ø)
...m/src/main/typescript/plugin-keychain-google-sm.ts 60.78% <60.78%> (ø)
... and 35 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 9ca7f6e...02f7302. 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.

@jagpreetsinghsasan jagpreetsinghsasan force-pushed the feature-971 branch 3 times, most recently from 66162ed to 3a9304a Compare July 22, 2021 11:01
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 rebase onto latest upstream/main to resolve the conflict(s) and then we are good to go.

@petermetz
Copy link
Contributor

petermetz commented Jul 28, 2021

@jagpreetsinghsasan (same thing a second time again) Please rebase onto latest upstream/main to resolve the conflict(s) and then we are good to go.

I just resolved the conflict because it was needed for Jeffrey's dependent PR as well.

	Primary Change
	---
	1. Added new package cactus-plugin-keychain-azure-kv under packages/
	2. Added PluginKeychainAzureKvMock class to mock the functions of SecretClient under
	packages/cactus-plugin-keychain-azure-kv/src/test/typescript/mock/plugin-keychain-azure-kv-mock.ts

Resolves hyperledger-cacti#971

Signed-off-by: jagpreetsinghsaan <jagpreet.singh.sasan@accenture.com>
Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz petermetz merged commit 69e7b50 into hyperledger-cacti:main Jul 29, 2021
@petermetz petermetz deleted the feature-971 branch July 29, 2021 01:42
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-azure-kv): create azure key vault plugin
4 participants