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.

Sign up for GitHub

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

Jump to bottom

Workflow addition of Pause/Resume and Purge #1080

Merged
merged 30 commits into from
May 9, 2023
Merged

Workflow addition of Pause/Resume and Purge #1080

merged 30 commits into from
May 9, 2023

Conversation

RyanLettieri
Copy link
Contributor

@RyanLettieri RyanLettieri commented Apr 28, 2023 •
edited
Loading

Description

Added in support for Pause, Resume, and Purge for workflow

Due to a naming mismatch in a few methods, the associated runtime change for this PR is here: dapr/dapr#6296

Issue reference

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

@RyanLettieri
Initial Push for new workflow methods
971afbc 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Updating proto and cleaning up workflow functions
07de5c0 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Updating runtime ver
ab17fc9 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@codecov
Copy link

codecov bot commented Apr 28, 2023 •
edited
Loading

Codecov Report

Merging #1080 (2592a35) into master (3d4ae5b) will decrease coverage by 0.85%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1080      +/-   ##
==========================================
- Coverage   68.93%   68.09%   -0.85%     
==========================================
  Files         169      169              
  Lines        5534     5604      +70     
  Branches      588      592       +4     
==========================================
+ Hits         3815     3816       +1     
- Misses       1579     1648      +69     
  Partials      140      140
Flag Coverage Δ
net6 68.00% <0.00%> (-0.87%) ⬇️
net7 68.00% <0.00%> (-0.87%) ⬇️
netcoreapp3.1 68.05% <0.00%> (-0.85%) ⬇️

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 75.23% <0.00%> (-6.50%) ⬇️
src/Dapr.Client/GetWorkflowResponse.cs 0.00% <0.00%> (ø)
src/Dapr.Client/StartWorkflowResponse.cs 0.00% <0.00%> (ø)

@RyanLettieri
Updating go ver
8cf20e1 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Updating go ver
1d946b1 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Temp removal of new workflow stuff to see if the test passes without it
f01ad45 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Another fix attempt for workflow test
e003c14 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Attempting to change input for workflow E2E test
7898cbb 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Commenting out pause/resume for testing
3ead042 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Correcting assert statement on workflow purge
24cdc2a 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Fixing exception check
9ccbb42 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Fixing exception check on purge
1042d00 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
added in testing for raise event in workflow
28b5086 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Pointing to wip for pause/resume fixes
62f55b5 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
First round of addresing feedback for workflow
a6a88be 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri RyanLettieri marked this pull request as ready for review May 1, 2023 22:12
@RyanLettieri RyanLettieri requested review from a team as code owners May 1, 2023 22:12
@RyanLettieri RyanLettieri mentioned this pull request May 1, 2023
Merged
7 tasks
philliphoff
philliphoff reviewed May 1, 2023
View reviewed changes
src/Dapr.Client/DaprClientGrpc.cs
};

try
{
var options = CreateCallOptions(headers: null, cancellationToken);
var response = await client.GetWorkflowAlpha1Async(request, options);
var dateTimeValue = new DateTime(response.StartTime, DateTimeKind.Utc);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not necessarily related to this change, but wonder if DateTimeOffset should be used in the public APIs. (It's the generally preferred type for most application purposes.)

@artursouza artursouza mentioned this pull request May 2, 2023
Closed
@RyanLettieri
Merging in master
6fb2666 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Addressing feedback
88f1715 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
addressing more feedback
9ba600d 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
fixing startup.cs for workflow test
b493cb0 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
fixing startup.cs for workflow test again
dcb6c75 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
changing variable type for workflow start
0e4b8c1 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Making code look nicer
fe2fe6b 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
cgillum
cgillum reviewed May 3, 2023
View reviewed changes
examples/Workflow/WorkflowConsoleApp/Workflows/OrderProcessingWorkflow.cs Outdated Show resolved Hide resolved
src/Dapr.Client/DaprClient.cs Outdated Show resolved Hide resolved
src/Dapr.Client/DaprClient.cs Outdated Show resolved Hide resolved
src/Dapr.Client/DaprClient.cs Outdated Show resolved Hide resolved
src/Dapr.Client/DaprClient.cs Outdated Show resolved Hide resolved
test/Dapr.E2E.Test/Workflows/WorkflowTest.cs Outdated Show resolved Hide resolved
test/Dapr.E2E.Test/Workflows/WorkflowTest.cs Outdated Show resolved Hide resolved
test/Dapr.E2E.Test/Workflows/WorkflowTest.cs Outdated Show resolved Hide resolved
test/Dapr.E2E.Test.App/Startup.cs Outdated Show resolved Hide resolved
src/Dapr.Client/GetWorkflowResponse.cs Outdated
/// <param name="runtimeStatus">The current runtime status of the workflow.</param>
/// <param name="properties">The response properties.</param>
public record GetWorkflowResponse(string instanceId, string workflowName, DateTime createdAt,
DateTime lastUpdatedAt, string runtimeStatus, IReadOnlyDictionary<string, string> properties);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a heads up that we need to make additional changes to this type, as mentioned here: #1027. This can probably be a separate PR, though.

halspang
halspang requested changes May 3, 2023
View reviewed changes
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.

Overall, this is looking pretty good. Some nitpicks and some questions around why we are introducing a breaking change.

examples/Workflow/WorkflowConsoleApp/Program.cs Outdated Show resolved Hide resolved
examples/Workflow/WorkflowConsoleApp/Workflows/OrderProcessingWorkflow.cs Outdated Show resolved Hide resolved
src/Dapr.Client/DaprClient.cs Show resolved Hide resolved
src/Dapr.Client/DaprClientGrpc.cs Outdated Show resolved Hide resolved
src/Dapr.Client/GetWorkflowResponse.cs Outdated Show resolved Hide resolved
test/Dapr.E2E.Test/Workflows/WorkflowTest.cs Outdated Show resolved Hide resolved
test/Dapr.E2E.Test/Workflows/WorkflowTest.cs Outdated Show resolved Hide resolved
examples/Workflow/WorkflowWebApp/Program.cs Outdated Show resolved Hide resolved
examples/Workflow/WorkflowWebApp/Program.cs Outdated Show resolved Hide resolved
examples/Workflow/WorkflowWebApp/WorkflowWebApp.csproj Outdated Show resolved Hide resolved
@cgillum
Copy link
Contributor

cgillum commented May 3, 2023

@halspang sorry that this wasn't included in the PR description, but we planned to have a whole host of breaking changes in the v1.11 release. Some of what you're seeing here was outlined in: #1027

cgillum
cgillum reviewed May 3, 2023
View reviewed changes
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 couple more comments.

src/Dapr.Client/DaprClient.cs Outdated Show resolved Hide resolved
src/Dapr.Client/DaprClientGrpc.cs Outdated Show resolved Hide resolved
cgillum
cgillum previously approved these changes May 3, 2023
View reviewed changes
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.

LGTM

halspang
halspang previously approved these changes May 3, 2023
View reviewed changes
@RyanLettieri
Updating workflow get for testing
b3d88ba 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Checking against null updated time for workflow updated time
0c9f260 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Adding in a delay before getting info on the workflow
d46e016 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Adding in a larger delay before getting info on the workflow
d202b0e 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Attempting to test against latest dapr dapr commit
59206b6 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Removing other sleeps from workflow test
9b492cb 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Addressing more feedback
11e7f28 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri
Fixing assert statement on workflow purge test
2592a35 
Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
@RyanLettieri RyanLettieri dismissed stale reviews from halspang and cgillum via b3d88ba May 4, 2023 18:11
cgillum
cgillum reviewed May 4, 2023
View reviewed changes
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 I found while doing a final pass.

src/Dapr.Client/DaprClientGrpc.cs Outdated Show resolved Hide resolved
src/Dapr.Client/GetWorkflowResponse.cs Outdated Show resolved Hide resolved
test/Dapr.E2E.Test/Workflows/WorkflowTest.cs Show resolved Hide resolved
cgillum
cgillum previously approved these changes May 4, 2023
View reviewed changes
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.

I'm good with this iteration, though I'm confused about the latest update to the test. I'm otherwised signing off.

test/Dapr.E2E.Test/Workflows/WorkflowTest.cs Show resolved Hide resolved
@halspang halspang merged commit 5a57035 into dapr:master May 9, 2023
onionhammer pushed a commit to onionhammer/dotnet-sdk that referenced this pull request May 30, 2023
@RyanLettieri @onionhammer
Workflow addition of Pause/Resume and Purge (dapr#1080)
addd15d 
* Initial Push for new workflow methods

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

* Updating proto and cleaning up workflow functions

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

* Updating runtime ver

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

* Updating go ver

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

* Updating go ver

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

* Temp removal of new workflow stuff to see if the test passes without it

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

* Another fix attempt for workflow test

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

* Attempting to change input for workflow E2E test

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

* Commenting out pause/resume for testing

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

* Correcting assert statement on workflow purge

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

* Fixing exception check

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

* Fixing exception check on purge

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

* added in testing for raise event in workflow

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

* Pointing to wip for pause/resume fixes

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

* First round of addresing feedback for workflow

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

* Addressing feedback

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

* addressing more feedback

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

* fixing startup.cs for workflow test

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

* fixing startup.cs for workflow test again

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

* changing variable type for workflow start

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

* Making code look nicer

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

* Updating workflow get for testing

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

* Checking against null updated time for workflow updated time

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

* Adding in a delay before getting info on the workflow

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

* Adding in a larger delay before getting info on the workflow

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

* Attempting to test against latest dapr dapr commit

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

* Removing other sleeps from workflow test

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

* Addressing more feedback

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

* Fixing assert statement on workflow purge test

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

---------

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>
Signed-off-by: Erik O'Leary <erik.m.oleary@gmail.com>
@halspang halspang added this to the v1.11 milestone Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philliphoff philliphoff philliphoff left review comments

@cgillum cgillum cgillum approved these changes

@halspang halspang halspang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.11
Development

Successfully merging this pull request may close these issues.
4 participants
@RyanLettieri @cgillum @philliphoff @halspang