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

Implement support for cryptography api #719

Merged
merged 10 commits into from
Jul 2, 2024

Conversation

KentHsu
Copy link
Contributor

@KentHsu KentHsu commented May 24, 2024

Description

  • Implement below in Dapr gRPC client

    • encrypt/decrypt request iterator and response generator and add tests
    • encrypt/decrypt methods in grpc client and add tests
  • Implement below in Dapr gRPC aio client

    • encrypt/decrypt request asynchronous iterator and response asynchronous generator and add tests
    • encrypt/decrypt methods in grpc aio client and add tests
  • Add cryptography examples follow go-sdk crypto example

Issue reference

Please reference the issue this PR will close: #548

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@KentHsu KentHsu requested review from a team as code owners May 24, 2024 06:43
@KentHsu KentHsu changed the title Implement support cryptography api Implement support for cryptography api May 29, 2024
@elena-kolevska
Copy link
Contributor

Thank you very much for the PR Kent, I'll try to get to it asap.

Copy link
Member

@berndverst berndverst left a comment

Choose a reason for hiding this comment

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

Awesome PR. Thanks @KentHsu

Can you run tox -e flake8 and tox -e ruff please?

The linter is failing at the moment.

That step runs tox -e flake8, tox -e type, tox -e mypy and tox -e ruff.

Copy link
Contributor

@elena-kolevska elena-kolevska left a comment

Choose a reason for hiding this comment

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

Great PR @KentHsu, thank you! I have just a few nits.

tests/clients/test_dapr_grpc_request.py Show resolved Hide resolved
examples/crypto/crypto.py Outdated Show resolved Hide resolved
examples/crypto/README.md Show resolved Hide resolved
@elena-kolevska
Copy link
Contributor

This PR should address the linter failing: #725

@berndverst
Copy link
Member

@KentHsu we are beyond code freeze - are you able to address the comments from @elena-kolevska by end of next week? Otherwise, this is unfortunately going to miss the 1.14 release of the SDK.

@berndverst
Copy link
Member

Currently seeing type check failures as well.

I recommend running the above mentioned commands to resolve type check, lint, autoformatting etc

@KentHsu
Copy link
Contributor Author

KentHsu commented Jun 28, 2024

Hi @berndverst, @elena-kolevska,
Thank you for reviewing this PR. I just added some commits to address @elena-kolevska's comments.
Please let me know if there's anything you would like to change. Hopefully we can make this PR into the 1.14 release.

@mikeee mikeee mentioned this pull request Jun 28, 2024
Copy link

codecov bot commented Jun 28, 2024

Codecov Report

Attention: Patch coverage is 96.75676% with 6 lines in your changes missing coverage. Please review.

Project coverage is 86.59%. Comparing base (fc0e9d1) to head (048d0c1).
Report is 29 commits behind head on main.

Files Patch % Lines
dapr/aio/clients/grpc/_request.py 95.23% 2 Missing ⚠️
dapr/clients/grpc/_request.py 94.87% 2 Missing ⚠️
dapr/aio/clients/grpc/_response.py 96.66% 1 Missing ⚠️
dapr/clients/grpc/_response.py 96.42% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #719      +/-   ##
==========================================
+ Coverage   86.37%   86.59%   +0.22%     
==========================================
  Files          79       84       +5     
  Lines        4094     4467     +373     
==========================================
+ Hits         3536     3868     +332     
- Misses        558      599      +41     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@elena-kolevska elena-kolevska left a comment

Choose a reason for hiding this comment

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

Really great work @KentHsu.
I'm approving the PR so as not to block the release, but to make this contribution 100% complete we would need a little example in the docs (this file) so that developers can discover and use your work.
It would be great if you could send a new PR with that addition.
Thanks again for your contribution, the community appreciates it!

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.

[Crypto] Implement support for EncryptAlpha1/DecryptAlpha1 APIs
3 participants