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(keychain-google-sm): complete request handler and endpoints #1097 #1188

Merged
merged 3 commits into from
Oct 6, 2021

Conversation

twiggins120
Copy link
Contributor

@twiggins120 twiggins120 commented Aug 6, 2021

Fixes #1097

Depends on #1196
Depends on #1406

@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2021

Codecov Report

Merging #1188 (bdeeffa) into main (01a5eb4) will increase coverage by 0.80%.
The diff coverage is 83.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1188      +/-   ##
==========================================
+ Coverage   70.53%   71.33%   +0.80%     
==========================================
  Files         302      306       +4     
  Lines       10902    11081     +179     
  Branches     1332     1346      +14     
==========================================
+ Hits         7690     7905     +215     
+ Misses       2508     2452      -56     
- Partials      704      724      +20     
Impacted Files Coverage Δ
.../typescript/mock/plugin-keychain-google-sm-mock.ts 73.68% <0.00%> (ø)
...ript/webservices/get-keychain-entry-endpoint-v1.ts 80.55% <80.55%> (ø)
...m/src/main/typescript/plugin-keychain-google-sm.ts 81.48% <83.33%> (+19.48%) ⬆️
...pescript/generated/openapi/typescript-axios/api.ts 84.09% <84.00%> (+70.09%) ⬆️
...t/webservices/delete-keychain-entry-endpoint-v1.ts 84.84% <84.84%> (ø)
...ript/webservices/set-keychain-entry-endpoint-v1.ts 84.84% <84.84%> (ø)
...ript/webservices/has-keychain-entry-endpoint-v1.ts 85.71% <85.71%> (ø)
...escript/generated/openapi/typescript-axios/base.ts 58.33% <0.00%> (+16.66%) ⬆️
...-sm/src/main/typescript/plugin-factory-keychain.ts 83.33% <0.00%> (+16.66%) ⬆️
... and 3 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 01a5eb4...bdeeffa. Read the comment docs.

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.

@twiggins120

A few nits:

  1. Please squash the left-over changes from Jagpreet's commit together with your own commit (you can use fixup so that it disappears the commit message from Jagpreet's commit)
  2. delete the package-lock.json, we don't need it since we migrated over to yarn
  3. Remove the package specific dependency from the root package .json file
  4. Make sure you coordinate with Wen about the additional PR that Wen is preparing to make the necessary changes to the core-api package's openapi.json file
  5. Once that PR from Wen is open from point 4), please make sure to mark this PR as a dependent on that one.
  6. Then once that PR gets merged this'll need to be updated to use the newly published core-api openapi.json file's link (I can show how that's done later)

cc: @ty-lazar @Leeyoungone

@decommissioned-account
Copy link

decommissioned-account commented Aug 10, 2021

@twiggins120

A few nits:

  1. Please squash the left-over changes from Jagpreet's commit together with your own commit (you can use fixup so that it disappears the commit message from Jagpreet's commit)
  2. delete the package-lock.json, we don't need it since we migrated over to yarn
  3. Remove the package specific dependency from the root package .json file
  4. Make sure you coordinate with Wen about the additional PR that Wen is preparing to make the necessary changes to the core-api package's openapi.json file
  5. Once that PR from Wen is open from point 4), please make sure to mark this PR as a dependent on that one.
  6. Then once that PR gets merged this'll need to be updated to use the newly published core-api openapi.json file's link (I can show how that's done later)

cc: @ty-lazar @Leeyoungone

Addressed points 1, 2, and 3

@Leeyoungone Leeyoungone force-pushed the feature-1097 branch 11 times, most recently from 7ac2996 to 8da0ecb Compare August 25, 2021 16:04
@Leeyoungone Leeyoungone force-pushed the feature-1097 branch 3 times, most recently from 6394b78 to b7dcf39 Compare September 3, 2021 19:46
@Leeyoungone
Copy link
Contributor

Hello again! It seems like I can't press the button to request review because the requested changes haven't been addressed? This time I'm not sure what they are but I'm going to go ahead and ping you for review @petermetz. Sorry about having to ask for reviews this way.

@Leeyoungone Leeyoungone force-pushed the feature-1097 branch 3 times, most recently from f78f33c to 7b9b2f1 Compare September 16, 2021 15:48
@Leeyoungone
Copy link
Contributor

Just finished rebasing and changed relative paths! @petermetz

@petermetz petermetz requested review from petermetz and takeutak and removed request for jonathan-m-hamilton September 16, 2021 15:49
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.

@Leeyoungone packages/cactus-core-api/src/main/json/openapi.json has changes in it, those should to the a parent PR and then once that's merged+tagged those changes can be used by this PR

@Leeyoungone Leeyoungone force-pushed the feature-1097 branch 4 times, most recently from a1da81d to 4050410 Compare September 23, 2021 16:08
@petermetz petermetz force-pushed the feature-1097 branch 3 times, most recently from 7d3a8e6 to 8009514 Compare October 2, 2021 23:28
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.

LGTM

@github-actions github-actions bot removed the dependent label Oct 5, 2021
@github-actions
Copy link

github-actions bot commented Oct 5, 2021

🎉 Great news! Looks like all the dependencies have been resolved:

💡 To add or remove a dependency please update this issue/PR description.

Brought to you by Dependent Issues (:robot: ). Happy coding!

Fixes hyperledger-cacti#1097
Depends on hyperledger-cacti#1349

Signed-off-by: Tommesha Wiggins <[email protected]>
Signed-off-by: Peter Somogyvari <[email protected]>
Signed-off-by: Tyler Lazar <[email protected]>
Signed-off-by: Youngone Lee <[email protected]>
Signed-off-by: Peter Somogyvari <[email protected]>
@petermetz petermetz merged commit 9c7bab5 into hyperledger-cacti:main Oct 6, 2021
@petermetz petermetz deleted the feature-1097 branch October 6, 2021 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(keychain-google-sm): complete request handler and endpoints
6 participants