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

Dapr client registration consolidation #1417

Closed
wants to merge 10 commits into from

Conversation

WhitWaldo
Copy link
Contributor

@WhitWaldo WhitWaldo commented Dec 3, 2024

Description

I updated the Jobs and Messaging client builder factories to achieve some uniformity with the other client builders so they all allow client configuration via IConfiguration when available via DI.

This approach means we don't have to do either of the following:

  • Pass a whole Action<DaprClientBuilder>? into every AddXClient() method
  • Register a single DaprClient that's used by all Dapr clients for a singular means of configuring Dapr clients

I also consolidated several methods to Dapr.Common that were duplicated throughout the builders. Once #1382 is merged, I'll also mirror these changes to it, but Workflows should already be covered, though it uses a completely different builder style.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

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

@WhitWaldo WhitWaldo added this to the v1.15 milestone Dec 3, 2024
@WhitWaldo WhitWaldo self-assigned this Dec 3, 2024
@WhitWaldo WhitWaldo requested review from a team as code owners December 3, 2024 14:44
Copy link

codecov bot commented Dec 3, 2024

Codecov Report

Attention: Patch coverage is 62.71186% with 22 lines in your changes missing coverage. Please review.

Project coverage is 70.03%. Comparing base (1b7c9f4) to head (f70933e).
Report is 75 commits behind head on master.

Files with missing lines Patch % Lines
src/Dapr.Common/DaprClientUtilities.cs 35.00% 12 Missing and 1 partial ⚠️
src/Dapr.Jobs/DaprJobsGrpcClient.cs 41.66% 7 Missing ⚠️
...PublishSubscribe/DaprPublishSubscribeGrpcClient.cs 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1417      +/-   ##
==========================================
+ Coverage   67.28%   70.03%   +2.74%     
==========================================
  Files         174      196      +22     
  Lines        6025     6564     +539     
  Branches      671      715      +44     
==========================================
+ Hits         4054     4597     +543     
  Misses       1802     1802              
+ Partials      169      165       -4     
Flag Coverage Δ
net6 70.01% <62.71%> (+2.75%) ⬆️
net7 70.01% <62.71%> (+2.75%) ⬆️
net8 70.01% <62.71%> (+2.74%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@WhitWaldo WhitWaldo changed the title Jobs registration update Dapr client registration consolidation Dec 3, 2024
@WhitWaldo WhitWaldo closed this Dec 10, 2024
@WhitWaldo
Copy link
Contributor Author

This PR is no longer necessary as most of it was included in #1382

@WhitWaldo WhitWaldo mentioned this pull request Dec 10, 2024
3 tasks
@WhitWaldo WhitWaldo deleted the jobs-registration-update branch December 11, 2024 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant