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

Adding recursive terminate/purge in workflow API #3969

Merged
merged 5 commits into from
Feb 1, 2024

Conversation

shivamkm07
Copy link
Contributor

@shivamkm07 shivamkm07 commented Jan 25, 2024

Thank you for helping make the Dapr documentation better!

Please follow this checklist before submitting:

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Read the contribution guide
  • Commands include options for Linux, MacOS, and Windows within codetabs
  • New file and folder names are globally unique
  • Page references use shortcodes instead of markdown or URL links
  • Images use HTML style and have alternative text
  • Places where multiple code/command options are given have codetabs

In addition, please fill out the following to help reviewers understand this pull request:

Description

This PR adds details about recursive termination/purge of child workflows in workflow api documentation.

Issue reference

Please reference the issue this PR will close: #3949

Signed-off-by: Shivam Kumar <shivamkm07@gmail.com>
@shivamkm07 shivamkm07 requested review from a team as code owners January 25, 2024 09:42
Copy link
Collaborator

@hhunter-ms hhunter-ms left a comment

Choose a reason for hiding this comment

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

quick review

daprdocs/content/en/reference/api/workflow_api.md Outdated Show resolved Hide resolved
daprdocs/content/en/reference/api/workflow_api.md Outdated Show resolved Hide resolved
daprdocs/content/en/reference/api/workflow_api.md Outdated Show resolved Hide resolved
daprdocs/content/en/reference/api/workflow_api.md Outdated Show resolved Hide resolved
daprdocs/content/en/reference/api/workflow_api.md Outdated Show resolved Hide resolved
@hhunter-ms hhunter-ms added this to the 1.13 milestone Jan 25, 2024
@hhunter-ms hhunter-ms added the waiting-on-code-pr The code PR needs to be merged before the docs are updated label Jan 25, 2024
shivamkm07 and others added 2 commits January 26, 2024 10:48
Co-authored-by: Hannah Hunter <94493363+hhunter-ms@users.noreply.github.com>
Signed-off-by: Shivam Kumar <shivamkm07@gmail.com>
@hhunter-ms hhunter-ms removed the waiting-on-code-pr The code PR needs to be merged before the docs are updated label Jan 29, 2024
@msfussell
Copy link
Member

@shivamkm07 - This needs more updates in the docs. Specifically we should cover this in the Child workflows section

  1. Add a paragraph here https://docs.dapr.io/developing-applications/building-blocks/workflow/workflow-overview/#child-workflows and remove the Note since this is no longer true
  2. Mention here https://docs.dapr.io/developing-applications/building-blocks/workflow/workflow-overview/#child-workflows that child workflows can be terminated from the parent workflow.
  3. Nit: I think it is very strange that we called the property non-recursive and set this to false. Calling this "recursive" and true is more intuitive. Can we change this?
  4. @hhunter-ms - There are no feature docs on purge capability. We should add these into the feature and concepts doc https://docs.dapr.io/developing-applications/building-blocks/workflow/workflow-features-concepts/

@shivamkm07
Copy link
Contributor Author

shivamkm07 commented Jan 31, 2024

@shivamkm07 - This needs more updates in the docs. Specifically we should cover this in the Child workflows section

  1. Add a paragraph here https://docs.dapr.io/developing-applications/building-blocks/workflow/workflow-overview/#child-workflows and remove the Note since this is no longer true
  2. Mention here https://docs.dapr.io/developing-applications/building-blocks/workflow/workflow-overview/#child-workflows that child workflows can be terminated from the parent workflow.
  3. Nit: I think it is very strange that we called the property non-recursive and set this to false. Calling this "recursive" and true is more intuitive. Can we change this?
  4. @hhunter-ms - There are no feature docs on purge capability. We should add these into the feature and concepts doc https://docs.dapr.io/developing-applications/building-blocks/workflow/workflow-features-concepts/

Thanks for the review. Will add sections accordingly.

Nit: I think it is very strange that we called the property non-recursive and set this to false. Calling this "recursive" and true is more intuitive. Can we change this?

This was actually decided to be kept as non_recursive so that the default value of flag is false. The default behavior intended is to be recursive, and keeping the recursive flag would make us to set the default value be true: dapr/dapr#7340 (comment)
cc @mukundansundar

@hhunter-ms
Copy link
Collaborator

hhunter-ms commented Jan 31, 2024

  1. There are no feature docs on purge capability. We should add these into the feature and concepts doc https://docs.dapr.io/developing-applications/building-blocks/workflow/workflow-features-concepts/

@shivamkm07 @msfussell I can do this in a separate PR, unless Shivam you'd rather take this?

@shivamkm07
Copy link
Contributor Author

  1. There are no feature docs on purge capability. We should add these into the feature and concepts doc https://docs.dapr.io/developing-applications/building-blocks/workflow/workflow-features-concepts/

@shivamkm07 @msfussell I can do this in a separate PR, unless Shivam you'd rather take this?

Sure, feel free to create separate PR for this. I will include 1 and 2 above in this PR.

@shivamkm07
Copy link
Contributor Author

@shivamkm07 - This needs more updates in the docs. Specifically we should cover this in the Child workflows section

  1. Add a paragraph here https://docs.dapr.io/developing-applications/building-blocks/workflow/workflow-overview/#child-workflows and remove the Note since this is no longer true
  2. Mention here https://docs.dapr.io/developing-applications/building-blocks/workflow/workflow-overview/#child-workflows that child workflows can be terminated from the parent workflow.
  3. Nit: I think it is very strange that we called the property non-recursive and set this to false. Calling this "recursive" and true is more intuitive. Can we change this?
  4. @hhunter-ms - There are no feature docs on purge capability. We should add these into the feature and concepts doc https://docs.dapr.io/developing-applications/building-blocks/workflow/workflow-features-concepts/

@msfussell @hhunter-ms Added requested changes 1 and 2. Please review.

@msfussell msfussell merged commit 7dea32b into dapr:v1.13 Feb 1, 2024
4 checks passed
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.

None yet

3 participants