Workflow Authoring: initial methods #981
Conversation
Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>
Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>
Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>
Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>
Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>
Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>
Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>
Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>
Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>
Signed-off-by: Amulya Varote <amulyavarote@microsoft.com>
This commit re-adds commit dapr@d5b9189 but with DCO. Signed-off-by: Tiago Alves Macambira <tmacam@burocrata.org>
Codecov Report
@@ Coverage Diff @@
## master #981 +/- ##
=======================================
Coverage 69.90% 69.90%
=======================================
Files 157 157
Lines 5224 5224
Branches 562 562
=======================================
Hits 3652 3652
Misses 1439 1439
Partials 133 133
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
tmacam
changed the title
Signed-off-by: Chris Gillum <cgillum@microsoft.com>
Update Durable Task SDK version
| /// </summary> | ||
| /// <param name="serviceCollection">The <see cref="IServiceCollection"/>.</param> | ||
| /// <param name="configure">A delegate used to configure actor options and register workflow functions.</param> | ||
| public static IServiceCollection AddWorkflow( |
There was a problem hiding this comment.
I could go either way, but the Actors SDK names the equivalent extension method AddActors() (no "Dapr") so AddDaprWorkflow would be a bit inconsistent with the actor SDK.
There was a problem hiding this comment.
I don't have a strong opinion, but .NET has (had) a number of "workflow" related frameworks, so it could be helpful to have "Dapr" in the method name. Actors isn't as common, so there's less potential for confusion.
There was a problem hiding this comment.
I'm fine either way. I'm not super familiar with the existing options though, so my concern would be if this would clobber anything. Do we expect people to have our libraries and the other workflow libraries together?
* Rename ActivityContext to WorkflowActivityContext * Change example webapp port away from 8080x Signed-off-by: Tiago Alves Macambira <tmacam@burocrata.org>
| // Example of registering a "Hello World" workflow function | ||
| options.RegisterWorkflow<string, string>("HelloWorld", implementation: async (context, input) => | ||
| { | ||
| return await context.CallActivityAsync<string>("SayHello", "World"); | ||
| }); | ||
|
|
||
| // Example of registering a "Say Hello" workflow activity function | ||
| options.RegisterActivity<string, string>("SayHello", implementation: (context, input) => | ||
| { | ||
| return Task.FromResult($"Hello, {input}!"); | ||
| }); |
There was a problem hiding this comment.
How do workflows and activities relate to each other? Should they be tied together in the API at all?
There was a problem hiding this comment.
I will let other correct me but if you think of a workflow as defining a graph, activities are the nodes. Both concepts are linked. https://docs.temporal.io/activities
There was a problem hiding this comment.
I wouldn't say this is required, just musing since this is a preview/alpha feature:
That was my initial thought, but I guess my question was more do the activities need to be directly related to the workflow, or can they exist outside of one? Should we have some kind of validation that checks that "SayHello" is defiend? Or should we define the activity in the workflow registration/via actual object references?
| /// </summary> | ||
| /// <param name="serviceCollection">The <see cref="IServiceCollection"/>.</param> | ||
| /// <param name="configure">A delegate used to configure actor options and register workflow functions.</param> | ||
| public static IServiceCollection AddWorkflow( |
There was a problem hiding this comment.
I'm fine either way. I'm not super familiar with the existing options though, so my concern would be if this would clobber anything. Do we expect people to have our libraries and the other workflow libraries together?
* Rename workflow and activity in example to be more meangniful * Add parameter documentation to some methods * Use local project references when appropriate Signed-off-by: Tiago Alves Macambira <tmacam@burocrata.org>
Signed-off-by: Tiago Alves Macambira <tmacam@burocrata.org>
Signed-off-by: Tiago Alves Macambira <tmacam@burocrata.org>
…atch what it does Signed-off-by: Tiago Alves Macambira <tmacam@burocrata.org>
…ntAsync Signed-off-by: Tiago Alves Macambira <tmacam@burocrata.org>
tmacam
changed the title
There was a problem hiding this comment.
Overall, the code seems fine. I think the big thing for me is that we should remove the example entirely to start off. Examples should show how we want users to invoke/use the code instead of just providing the syntax.
What I'd prefer to do here is create an issue to create a workflow and once the code is complete, spend some time to make a more real-world example that shows workflows as part of a larger picture.
Signed-off-by: Tiago Alves Macambira <tmacam@burocrata.org>
@cgillum, you will be updating the example in a follow up PR, yes? So I will defer comment regarding the removal of the example to you. |
|
Yes, I can update the example to be closer to the recent demo I gave. I agree that would be much better. However, I don't necessarily agree that we should exclude the existing example from this PR. That's just creating unnecessary work for us, and I think it's honestly better than having nothing at all. |
Signed-off-by: Tiago Alves Macambira <tmacam@burocrata.org>
If it we're planning on updating it shortly, that works for me. I just wanted to make sure we had a full example before we eventually hit a full release for workflows. |
Description
(This PR is a re-submission of PR #973 with extra fixes when appropriate)
This is a work in progress PR for dapr workflow initial methods for dotnet-sdk.
This needs e2e tests still.
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: #979
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: