Skip to content

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
Contributor

@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

@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
Contributor

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.

Signed-off-by: Shivam Kumar <shivamkm07@gmail.com>
@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.

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.

3 participants