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

Workflow Management - Initial Methods #1003

Merged
merged 33 commits into from
Jan 31, 2023
Merged

Conversation

RyanLettieri
Copy link
Contributor

Signed-off-by: Ryan Lettieri ryanLettieri@microsoft.com

Description

Initial draft for the workflows SDK in DotNET

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: #[issue number]

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

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@codecov
Copy link

codecov bot commented Jan 3, 2023

Codecov Report

Merging #1003 (49a0c94) into master (6e77f12) will decrease coverage by 0.72%.
The diff coverage is 0.00%.

❗ Current head 49a0c94 differs from pull request most recent head 938e349. Consider uploading reports for the commit 938e349 to get more accurate results

@@            Coverage Diff             @@
##           master    #1003      +/-   ##
==========================================
- Coverage   70.16%   69.45%   -0.72%     
==========================================
  Files         160      162       +2     
  Lines        5377     5432      +55     
  Branches      583      585       +2     
==========================================
  Hits         3773     3773              
- Misses       1464     1519      +55     
  Partials      140      140              
Flag Coverage Δ
net6 69.38% <0.00%> (-0.71%) ⬇️
net7 69.38% <0.00%> (-0.71%) ⬇️
netcoreapp3.1 69.42% <0.00%> (-0.72%) ⬇️

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

Impacted Files Coverage Δ
src/Dapr.Client/DaprClient.cs 92.59% <ø> (ø)
src/Dapr.Client/DaprClientGrpc.cs 81.73% <0.00%> (-5.54%) ⬇️
src/Dapr.Client/GetWorkflowResponse.cs 0.00% <0.00%> (ø)
src/Dapr.Client/WorkflowReference.cs 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

Initial round of feedback.

string instanceID,
string workflowComponent,
string workflowType,
Dictionary<string, string> workflowOptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at other APIs in this class, I wonder if workflowOptions should instead by IReadOnlyDictionary<string, string> and be an optional parameter (you'll need to move it down one for that, though)? Something like this:

IReadOnlyDictionary<string, string> metadata = default

string workflowComponent,
string workflowType,
Dictionary<string, string> workflowOptions,
ByteString input,
Copy link
Contributor

Choose a reason for hiding this comment

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

I've never heard of ByteString before. Do we use this in other places in the Dapr SDK? I was expecting we'd simply allow object and just document/require that the parameter value be JSON-serializable. That might be an easier interface for developers to consume.

/// </summary>
/// <param name="instanceID">Identifier of the specific run.</param>
/// <param name="workflowComponent">The component to interface with.</param>
/// <param name="workflowType">Name of the workflow to run (function name).</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

Workflows aren't always going to be implemented as functions. For example, in Temporal, I think the name comes from something else. I suggest removing (function name) from the description here and in the APIs below.

/// <param name="cancellationToken">A <see cref="CancellationToken" /> that can be used to cancel the operation.</param>
/// <returns>A <see cref="Task"/> containing a <see cref="WorkflowReference"/></returns>
[Obsolete("This API is currently not stable as it is in the Alpha stage. This attribute will be removed once it is stable.")]
public abstract Task<WorkflowReference> StartWorkflow(
Copy link
Contributor

Choose a reason for hiding this comment

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

Normal convention in .NET is to suffix all method names that return Task or Task<T> with Async. However, I see this isn't being done consistently in the DaprClient class. I'd love to get the opinion of a .NET SDK maintainer on whether these new API names should be suffixed with Async.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should I we have a "Deprecation" warning to the Workflow Authoring SDK too?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tmacam my initial thinking is that this isn't necessary in the WF authoring SDK because it's a new SDK, whereas Ryan's changes are to an existing SDK. However, we may want @halspang to weigh in on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the point of the attribute here is to make people aware they're using a new feature. If they're using a new SDK, I think that's part of the implication.

Also, since we're here, I would like to have the Async in the method name (and the one below). I know we aren't consistent but we should follow standards where we can and be sad about our lack of consistency elsewhere.

/// <param name="instanceID">Identifier of the specific run.</param>
/// <param name="workflowComponent">The component to interface with.</param>
/// <param name="cancellationToken">A <see cref="CancellationToken" /> that can be used to cancel the operation.</param>
/// <returns>A <see cref="Task" /> that will complete when the operation has completed. If the wrapped value is true the operation suceeded.</returns>
Copy link
Contributor

Choose a reason for hiding this comment

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

It's currently ambiguous as to what "operation" refers to here. For example, developers reading this might incorrectly assume that this task completes when the workflow is terminated. However, that's not actually the case. Terminate operations are normally async and aren't guaranteed to terminate the workflow right away. I suggest we say that this task completes when the terminate operation has been scheduled.

}

/// <summary>
/// Gets the instance ID assocated with this response.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should clarify in this description that this is a workflow instance ID. The current description is a bit ambiguous.

/// <summary>
/// Gets the time that the workflow started.
/// </summary>
public Int64 StartTime { set; get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be DateTime (or DateTimeOffset)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The proto definition of start_time is an int64 so StartTime in this scenario was created as an Int64

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but it's not very friendly to present a .NET developer with an integer value when a much more friendly types are available. I think we really need to use DateTime or DateTimeOffset here, and do the conversion from int64 internally.

public Int64 StartTime { set; get; }

/// <summary>
/// Gets the response metadata.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we be more descriptive about what this is? For example, is it metadata associated with the response or is it metadata associated with the workflow?

}

/// <summary>
/// Gets the instance ID assocated with this response.
Copy link
Contributor

Choose a reason for hiding this comment

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

This description is misleading as there is no "response" here.

@@ -287,6 +300,53 @@ message PublishEventRequest {
map<string, string> metadata = 5;
}

// BulkPublishRequest is the message to bulk publish events to pubsub topic
message BulkPublishRequest {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a little confusing that there are non-workflow changes in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just updating the protos to match what was done in Dapr to add management support for workflows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So my understanding is that there're proto changes inside of Dapr/dapr unrelated to the work I've done that still need to be pulled into the DotNet SDK. When I'm pulling in the protos for the workflows, they're also getting pulled in.

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri RyanLettieri changed the title Initial work for workflows DotNET SDK Workflow Management - Initial Methods Jan 12, 2023
Comment on lines +1403 to +1405
if (workflowOptions?.Count > 0)
{
foreach (var item in workflowOptions)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this if here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need it for the null check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is here to check against the possibility of workflowOptions being Null.

Comment on lines 1474 to 1477
Autogenerated.TerminateWorkflowResponse response = new Autogenerated.TerminateWorkflowResponse(); // RRL Do we need a response?
try
{
response = await client.TerminateWorkflowAlpha1Async(request, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

We aren't returning or consuming response. Why is it needed?

/// <param name="instanceId">The instance ID assocated with this response.</param>
/// <param name="startTime">The time at which the workflow started executing.</param>
/// <param name="metadata">The response metadata.</param>
public GetWorkflowResponse(string instanceId, Int64 startTime, IReadOnlyDictionary<string, string> metadata)
Copy link
Contributor

@tmacam tmacam Jan 13, 2023

Choose a reason for hiding this comment

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

Could the class be a record? Aside construction, we never really overwrite any of its fields, right? I guess we would lose the ability to verify those arguments if this was a record, correct?

I know this can be done (https://stackoverflow.com/questions/68915019/how-to-check-if-parameters-are-null-when-using-positional-record-constructors-an) but I wonder if it would make the code any better. Again, C# is not my forté.

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@@ -1397,10 +1397,10 @@ private Autogenerated.StateItem ToAutogeneratedStateItem(StateTransactionRequest
InstanceId = instanceId,
WorkflowComponent = workflowComponent,
WorkflowName = workflowType,
Input = input
Input = (ByteString)input
Copy link
Contributor

Choose a reason for hiding this comment

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

Yikes, this is not what I had in mind when I suggested allowing object instead of ByteString. What you have here seems very problematic because it will result in InvalidCastException whenever a user specifies an input that can't be cast directly into ByteString. What I was actually proposing is that we accept any object, JSON-serialize it, and then convert the serialized input JSON into whatever the protobuf contract requires (ByteString, in this case).

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@@ -1446,7 +1446,8 @@ private Autogenerated.StateItem ToAutogeneratedStateItem(StateTransactionRequest
{
var options = CreateCallOptions(headers: null, cancellationToken);
var response = await client.GetWorkflowAlpha1Async(request, options);
return new GetWorkflowResponse(response.InstanceId, response.StartTime, response.Metadata);
var dateTimeValue = new DateTime(response.StartTime);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the this constructor overload and specify DateTimeKind.Utc. Developers will appreciate this.

@@ -19,15 +19,15 @@ namespace Dapr.Client
/// <summary>
/// Represents the response from invoking a binding.
/// </summary>
public sealed class GetWorkflowResponse
public sealed record GetWorkflowResponse
Copy link
Contributor

Choose a reason for hiding this comment

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

This actually isn't the right way to use record, so I'm a little surprised that C# lets you do this. The right way would be to do something like this:

public record GetWorkflowResponse(string InstanceId, DateTime StartTime, IReadOnlyDictionary<string, string> Metadata);

And that's it - you can remove your constructor and the properties below because the C# compiler will auto-generate them.

RyanLettieri and others added 8 commits January 20, 2023 12:46
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri RyanLettieri marked this pull request as ready for review January 27, 2023 18:06
@RyanLettieri RyanLettieri requested review from a team as code owners January 27, 2023 18:06
Copy link
Contributor

@halspang halspang left a comment

Choose a reason for hiding this comment

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

Minor comments, thanks for the work on this!

/// <param name="cancellationToken">A <see cref="CancellationToken" /> that can be used to cancel the operation.</param>
/// <returns>A <see cref="Task"/> containing a <see cref="WorkflowReference"/></returns>
[Obsolete("This API is currently not stable as it is in the Alpha stage. This attribute will be removed once it is stable.")]
public abstract Task<WorkflowReference> StartWorkflow(
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the point of the attribute here is to make people aware they're using a new feature. If they're using a new SDK, I think that's part of the implication.

Also, since we're here, I would like to have the Async in the method name (and the one below). I know we aren't consistent but we should follow standards where we can and be sad about our lack of consistency elsewhere.

/// <param name="workflowComponent">The component to interface with.</param>
/// <param name="workflowType">Name of the workflow to run.</param>
/// <param name="workflowOptions">The list of options that are potentially needed to start a workflow.</param>
/// <param name="input">The input input for the given workflow.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we've had this conversation, so forgive me. But is there a reason we went with Object instead of just a Generic? Is it just because we don't have a good means of using the generic so we get nothing from the type checking side of the world?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, there's no benefit to using a generic here. We don't really care what the type of the input is. We just need to be able to serialize it to JSON.

Comment on lines 24 to 25
/// <param name="metadata">The response metadata.</param>
public record GetWorkflowResponse(string instanceId, DateTime startTime, IReadOnlyDictionary<string, string> metadata);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: indentation seems messed up here.

@@ -531,6 +543,23 @@ message RegisteredComponents {
repeated string capabilities = 4;
}

message PubsubSubscription {
Copy link
Contributor

Choose a reason for hiding this comment

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

@yash-nisar - Does this impact your changes in #1009?

Copy link
Contributor

Choose a reason for hiding this comment

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

@halspang Nope, I don't think we're calling any auto generated code for bulk subscribe but I'll update the proto anyway just to be consistent with what we have in dapr/dapr.

@@ -2,7 +2,7 @@

<!-- NuGet configuration -->
<PropertyGroup>
<TargetFramework>net6</TargetFramework>
<TargetFrameworks>netcoreapp3.1;net5;net6</TargetFrameworks>
Copy link
Contributor

Choose a reason for hiding this comment

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

No more net5 please :) Also can we add net7?

@cgillum - Does DurableTask build for 3.1?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it targets netstandard2.0, so we should be good with any of these targets for the SDK.

I would recommend net6 instead of net7, however, since some users are hesitant to move to a non-LTS version of .NET.

[Fact]
public async Task TestWorkflows()
{
Thread.Sleep(10000); // Sleep for 10s to wait for engine to start
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this after we figured out that startup issue?

var health = await daprClient.CheckHealthAsync();
health.Should().Be(true, "DaprClient is not healthy");

Thread.Sleep(10000); // Sleep for 10s to wait for engine to start
Copy link
Contributor

Choose a reason for hiding this comment

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

Uh oh, we do it twice!


// START WORKFLOW TEST
var startResponse = await daprClient.StartWorkflow(instanceID, workflowComponent, workflowType, input, workflowOptions, cts);
startResponse.InstanceId.Should().Be("TestWorkflowInstanceID", "Instance ID was not correct");
Copy link
Contributor

Choose a reason for hiding this comment

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

Include the ID we got from the response in the error.

Comment on lines 54 to 59
getResponse.metadata["dapr.workflow.runtime_status"].Should().Be("RUNNING", "The workflow is not running when it is expected to be running");

// TERMINATE TEST:
await daprClient.TerminateWorkflow(instanceID, workflowComponent);
getResponse = await daprClient.GetWorkflow(instanceID, workflowComponent, workflowType);
getResponse.metadata["dapr.workflow.runtime_status"].Should().Be("TERMINATED", "The workflow is still running when it is expected to be terminated");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with these two, include the response we got in the error.

Copy link
Contributor

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

A few more things to consider. Not necessarily blocking, except maybe the last one about how we can make the test a bit more robust.

@@ -997,7 +997,7 @@ public async Task<StateEntry<TValue>> GetStateEntryAsync<TValue>(string storeNam
/// <summary>
/// Attempt to get information about the given workflow.
/// </summary>
/// <param name="instanceID">Identifier of the specific run.</param>
/// <param name="instanceID">Identifier of the workflow specific run.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

How about this:

Suggested change
/// <param name="instanceID">Identifier of the workflow specific run.</param>
/// <param name="instanceID">The unique ID of the target workflow instance.</param>

public abstract Task<WorkflowReference> StartWorkflow(
string instanceID,
string workflowComponent,
string workflowType,
Copy link
Contributor

@cgillum cgillum Jan 27, 2023

Choose a reason for hiding this comment

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

Based some feedback I got from Mark, I suggest we rename workflowType to workflowName.

Suggested change
string workflowType,
string workflowName,

public abstract Task<GetWorkflowResponse> GetWorkflow(
string instanceID,
string workflowComponent,
string workflowType,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a little awkward that I didn't notice this until now, but we don't actually need the workflowType parameter at all for GetWorkflow. I suppose we can keep it in for now since it's a little late to be making significant changes, but we should remove it in the next release. Assuming we agree that we should keep this for now, I suggest we rename it here to workflowName to be consistent with my feedback on StartWorkflow.

/// <returns>A <see cref="Task"/> containing a <see cref="WorkflowReference"/></returns>
[Obsolete("This API is currently not stable as it is in the Alpha stage. This attribute will be removed once it is stable.")]
public abstract Task<WorkflowReference> StartWorkflow(
string instanceID,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: In normal C# coding conventions, this would actually be instanceId (lowercase "d") instead of instanceID. We don't need to worry about this now, but might be good to consider for the next release.

/// <summary>
/// Attempt to get terminate the given workflow.
/// </summary>
/// <param name="instanceID">Identifier of the specific run.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// <param name="instanceID">Identifier of the specific run.</param>
/// <param name="instanceID">The unique ID of the target workflow instance.</param>

namespace Dapr.Client
{
/// <summary>
/// Initializes a new <see cref="GetWorkflowResponse" />.`
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra character at the end:

Suggested change
/// Initializes a new <see cref="GetWorkflowResponse" />.`
/// Initializes a new <see cref="GetWorkflowResponse" />.

@@ -517,6 +528,7 @@ message GetMetadataResponse {
repeated ActiveActorsCount active_actors_count = 2;
repeated RegisteredComponents registered_components = 3;
map<string, string> extended_metadata = 4;
repeated PubsubSubscription subscriptions = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there are several unrelated pubsub changes in this PR?

@@ -2,7 +2,7 @@

<!-- NuGet configuration -->
<PropertyGroup>
<TargetFramework>net6</TargetFramework>
<TargetFrameworks>netcoreapp3.1;net5;net6</TargetFrameworks>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we making this change?

@@ -25,6 +25,8 @@ namespace Dapr.E2E.Test
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Dapr.Workflow;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This should probably be moved up with the other Dapr using statements.


// TERMINATE TEST:
await daprClient.TerminateWorkflow(instanceID, workflowComponent);
getResponse = await daprClient.GetWorkflow(instanceID, workflowComponent, workflowType);
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest doing this in a loop for maybe 10 seconds max. Workflow termination is asynchronous, so it may take a second or two for GetWorkflow to actually see that the workflow has terminated.

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Copy link
Contributor

@halspang halspang left a comment

Choose a reason for hiding this comment

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

Issue with string interpolation in the tests, but otherwise seems good. If @cgillum also agrees I think we can get this merged once you fix the error message.


// START WORKFLOW TEST
var startResponse = await daprClient.StartWorkflowAsync(instanceId, workflowComponent, workflowName, input, workflowOptions, cts);
startResponse.InstanceId.Should().Be("TestWorkflowInstanceID", "Instance ID $(startResponse.InstanceId) was not correct");
Copy link
Contributor

Choose a reason for hiding this comment

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

This string interpolation isn't correct.

startResponse.InstanceId.Should().Be("TestWorkflowInstanceID", $"Instance ID {startResponse.InstanceId} was not correct");

Copy link
Contributor

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

Signing off, but added some non-blocking comments for how we can make this E2E test more robust.

// GET INFO TEST
var getResponse = await daprClient.GetWorkflowAsync(instanceId, workflowComponent, workflowName);
getResponse.instanceId.Should().Be("TestWorkflowInstanceID");
getResponse.metadata["dapr.workflow.runtime_status"].Should().Be("RUNNING", $"Instance ID {getResponse.metadata["dapr.workflow.runtime_status"]} was not correct");
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking, we can probably do this later: I'm not sure what happened to my other comment, but I'm still worried about a race condition here that could cause the test to be flakey. Starting a workflow is asynchronous and the call to GetWorkflowAsync might return a "PENDING" status before it returns "RUNNING". We should put the GetWorkflowAsync call in a loop so that we can check a few times.

// TERMINATE TEST:
await daprClient.TerminateWorkflowAsync(instanceId, workflowComponent);
getResponse = await daprClient.GetWorkflowAsync(instanceId, workflowComponent, workflowName);
getResponse.metadata["dapr.workflow.runtime_status"].Should().Be("TERMINATED", $"Instance ID {getResponse.metadata["dapr.workflow.runtime_status"]} was not correct");
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking, we can probably do this later: I'm still worried about a race condition here that could cause the test to be flakey. Workflow termination is asynchronous, so we can't guarantee that it will be terminated by the time we call GetWorkflowAsync. We should put the GetWorkflowAsync call in a loop so that we can check a few times.

@halspang halspang merged commit 152d190 into dapr:master Jan 31, 2023
RyanLettieri added a commit to RyanLettieri/dotnet-sdk that referenced this pull request Feb 10, 2023
Initial work for workflows DotNET SDK

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
halspang pushed a commit that referenced this pull request Feb 10, 2023
…ity. (#1020)

* Workflow Management - Initial Methods (#1003)

Initial work for workflows DotNET SDK

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Beefed up the workflows example program and added in statestore functionality

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Addressing a bunch of review comments

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Updates to readme and demo for workflows

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Changed webapp to console app

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Update DurableTask SDK dependency to get ARM64 compatibility (#1024)

* Update DurableTask SDK dependency to get ARM64 compatibility

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

* Fix issue with gRPC address override behavior

Signed-off-by: Chris Gillum <cgillum@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Remove Web APIs and web dependencies

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Renaming WorkflowWebApp to WorkflowConsoleApp

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Various updates to the sample app

- Replaced DaprClient with WorkflowEngineClient
- Removed unused etag logic
- Fixed incorrect usage of certain model types
- Cleaned up logs and console output
- Simplified program loop
- Cleaned up console output and added some coloring
- Added error handling in the console interactions
- Various other tweaks/simplifications/enhancements

Signed-off-by: Chris Gillum <cgillum@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Updates to README and demo http commands

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Make README copy/paste-able and some other minor tweaks

Signed-off-by: Chris Gillum <cgillum@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Adding in Paul's devcontainer work

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* More README touch-ups

Signed-off-by: Chris Gillum <cgillum@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* [docs] Add workflows to .NET client doc (#1019)

* add workflows to client page

Signed-off-by: Hannah Hunter <hannahhunter@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Updating workflows readme and example

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Fixing README for letting users know which .NET is needed

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* moving using statements above the namespace

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

---------

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Chris Gillum <cgillum@microsoft.com>
Signed-off-by: Hannah Hunter <hannahhunter@microsoft.com>
Co-authored-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Co-authored-by: Chris Gillum <cgillum@microsoft.com>
Co-authored-by: Hannah Hunter <94493363+hhunter-ms@users.noreply.github.com>
@halspang halspang added this to the v1.10 milestone Feb 15, 2023
yash-nisar pushed a commit to yash-nisar/dotnet-sdk that referenced this pull request Feb 27, 2023
…ity. (dapr#1020)

* Workflow Management - Initial Methods (dapr#1003)

Initial work for workflows DotNET SDK

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Beefed up the workflows example program and added in statestore functionality

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Addressing a bunch of review comments

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Updates to readme and demo for workflows

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Changed webapp to console app

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Update DurableTask SDK dependency to get ARM64 compatibility (dapr#1024)

* Update DurableTask SDK dependency to get ARM64 compatibility

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

* Fix issue with gRPC address override behavior

Signed-off-by: Chris Gillum <cgillum@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Remove Web APIs and web dependencies

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Renaming WorkflowWebApp to WorkflowConsoleApp

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Various updates to the sample app

- Replaced DaprClient with WorkflowEngineClient
- Removed unused etag logic
- Fixed incorrect usage of certain model types
- Cleaned up logs and console output
- Simplified program loop
- Cleaned up console output and added some coloring
- Added error handling in the console interactions
- Various other tweaks/simplifications/enhancements

Signed-off-by: Chris Gillum <cgillum@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Updates to README and demo http commands

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Make README copy/paste-able and some other minor tweaks

Signed-off-by: Chris Gillum <cgillum@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Adding in Paul's devcontainer work

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* More README touch-ups

Signed-off-by: Chris Gillum <cgillum@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* [docs] Add workflows to .NET client doc (dapr#1019)

* add workflows to client page

Signed-off-by: Hannah Hunter <hannahhunter@microsoft.com>
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Updating workflows readme and example

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* Fixing README for letting users know which .NET is needed

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

* moving using statements above the namespace

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

---------

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Chris Gillum <cgillum@microsoft.com>
Signed-off-by: Hannah Hunter <hannahhunter@microsoft.com>
Co-authored-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Co-authored-by: Chris Gillum <cgillum@microsoft.com>
Co-authored-by: Hannah Hunter <94493363+hhunter-ms@users.noreply.github.com>
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.

5 participants