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

Added .NET client for new Dapr Jobs API support #1320

Closed
wants to merge 65 commits into from

Conversation

WhitWaldo
Copy link
Contributor

@WhitWaldo WhitWaldo commented Jul 6, 2024

Description

Added a Dapr Jobs client for the new Jobs API

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.

Please reference the issue this PR will close: #1321

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

This isn't yet ready to be merged as I need to figure out how the sidecar signals job triggers back to the app.

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
@WhitWaldo
Copy link
Contributor Author

/assign

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whitwaldo@innovian.net>
…endencies in both Dapr.Client and Dapr.AspNetCore

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
…riate gRPC types + client

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
… Jobs

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
…namespace the job is being requested by

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
…ace so nothing is changed in Dapr.Client

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
@WhitWaldo
Copy link
Contributor Author

Oh good, all the tests are passing again. Glad to see that! I'll see if I can't hammer out some unit tests to go with and get this to run locally so I can trial out that sample project too and get this wrapped up.

…apr"

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
…e project

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
…ck in GRPC client implementation to check for negative values.

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
… instead of creating a new instance from the DaprClientGenericBuilder.

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
…nt - caught while augmenting unit tests

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
…compatible objects.

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
…dule method to the next

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
… used anywhere. Rather, previous commits provided helper extensions that will allow configurable serialization, but the methods exposed on the DaprJobsClient assume the developer will bring their own serialization approach to bear.

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
…cy on Dapr.Client.

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
…references through project references. This is unfortunate because it requires some updates for nullability changes which was hopefully out of scope for this initative.

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
….Extensions.Http 8.0.0 by rolling that version back to 3.1.32 (updated Dapr.Extensions.Configuration to the latest minor release of 3.1.32 from 3.1.2 in the process).

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
@WhitWaldo
Copy link
Contributor Author

What are you thoughts on dropping the serialization altogether here and just accepting a byte[] for the job payload? It puts the responsibility back on the developer to handle serialization however they want and means that we avoid the inevitable bug report of someone experiencing a runtime error because they've specified a different type to deserialize fom than they used whenever they created the job. By instead only accepting a byte[], they can use whatever serialization they desire on their side and gets the SDK out of having to support pluggable serialization.

@WhitWaldo We could take the .NET runtime approach and have the client offer a "plain" method that accepts the raw input (probably better to be ReadOnlyMemory<byte> rather than byte[]) and then offer extension methods that wrap the method with JSON-specific flavors (see HttpClient.PostAsync() and HttpClientJsonExtensions.PostAsJsonAsync<TValue>()). Those extension methods could have an additional overload that accepts, for example, a JsonSerializerOptions instance to configure things, which then offers very similar behavior to today's Dapr client class but without embedding all of the serialization logic in the "main" methods. While we still technically implement the serialization, it's more directly configurable by the user.

@philliphoff In the latest batch of commits, I added a collection of extension methods that'll support JSON and string serialization (with a JsonSerializationOptions passed in as an optional argument) and deserialization along with unit tests for the latter set.

As I discovered after a few failing unit tests, Moq doesn't work with static methods, which makes this project rather difficult to get full coverage on. Is there any interest at some point in seeing if there's a commercial mocking framework that would grant a license for this open-source project (e.g. Telerik's JustMock)? I've got to imagine there are a great many opportunities for richer coverage we just can't test with open-source mocking tooling.

Finally, I added a change to the generic Dapr client builder that took a dependency on Microsoft.Extensions.Http so we could source the HttpClient from its IHttpClientFactory.CreateClient method instead of creating one every time the builder is used and it'd be registered in DI alongside anything else. Unfortunately, this package takes a hard dependency on the 8.0.* logging which broke Dapr.Extensions.Configuration because of its chain of references and its existing reference to 3.1.2 for Microsoft.Extensions.Configuration. Updating to 8.0.* on the configuration project introduces a nullable incompatibility minefield, so I've instead I've fixed it for now by reverting the Microsoft.Extensions.Http to 3.1.32 (and upgrading the configuration project to the same). All that to say that when we drop .NET 6, it'd be a great time to both review #1004 and keep working through #1316 because with package updates, we're likely going to introduce more nullability work.

…PI token

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
@WhitWaldo
Copy link
Contributor Author

Unclear why the build is failing on CI/CD - it's building all the way through locally.

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
@WhitWaldo
Copy link
Contributor Author

@philliphoff Closing this PR in favor of #1331 now that all the unit tests are passing. Did a squash merge per your suggestion to fix the DCO error.

@WhitWaldo WhitWaldo closed this Jul 25, 2024
@WhitWaldo WhitWaldo deleted the job-api-sdk branch September 19, 2024 13:30
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.

Jobs API needs a .NET client
3 participants