Skip to content

Conversation

jorgenj
Copy link
Contributor

@jorgenj jorgenj commented Apr 30, 2021

Remove workflowId from Parallel state (use subFlow actions instead).

Signed-off-by: Jorgen Johnson jorgenj@gmail.com

Many thanks for submitting your Pull Request ❤️!

Please specify parts this PR updates:

  • [ ✅ ] Specification
  • [ ✅ ] Schema
  • [ ✅ ] Examples
  • Extensions
  • Roadmap
  • Use Cases
  • Community
  • TCK
  • Other

What this PR does / why we need it:

I found some updates that I missed in the recent subFlowRef change that should be cleaned up. Apologies for missing this in the previous PR.

Special notes for reviewers:

Additional information (if needed):

@tsurdilo
Copy link
Contributor

@jorgenj if we are removing something from the DSL (workflowid for parallel state branches) we should mention it in the roadmap readme.md, please add it there.

Also, in the specification.md for SubflowRef definition it does not say atm that it can also be a string type, just noticed, could we update that as well?

Last thing I noticed was that idk how this slipped review but

"waitForCompletion": {
              "type": "boolean",
              "default": false,
              "description": "Workflow execution must wait for sub-workflow to finish before continuing"
            }

the default value of this should be true please. Doing anything async should be explicit choice by user.

@tsurdilo
Copy link
Contributor

tsurdilo commented Apr 30, 2021

@jorgenj if its possible let's do a separate pr for removing the workflowid from parallel state branches. it would allow us to track this change and discuss it in meeting.

@tsurdilo tsurdilo added the area: spec Changes in the Specification label Apr 30, 2021
@tsurdilo
Copy link
Contributor

waitForCompletion default value fixed here: #361

@jorgenj jorgenj changed the title Cleanup some references to workflowId that were missed in the recent … Remove workflowId from Parallel state Apr 30, 2021
Signed-off-by: Jorgen Johnson <jorgenj@gmail.com>
@jorgenj
Copy link
Contributor Author

jorgenj commented Apr 30, 2021

@jorgenj if its possible let's do a separate pr for removing the workflowid from parallel state branches. it would allow us to track this change and discuss it in meeting.

@tsurdilo There's really not much in this PR except for that, so I renamed this PR to call it that and added the item in roadmap.

@tsurdilo
Copy link
Contributor

tsurdilo commented May 1, 2021

@jorgenj thanks for this,
i can add the specification.md update to subflowRef in a separate pr (type string is not explained atm)

@@ -27,6 +27,7 @@ _Status description:_
| ✔️| Rename switch state `default` to `defaultCondition` to avoid keyword conflicts for SDK's | [spec doc](../specification.md) |
| ✔️| Add description of additional properties | [spec doc](../specification.md) |
| ✔️| Rename Parallel `completionType` values | [spec doc](../specification.md) |
| ✔️| Rename `workflowId` from ParallelState (use subFlow action instead) | [spec doc](../specification.md) |
Copy link
Contributor

Choose a reason for hiding this comment

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

"Remove" - I'll fix in a separate pr cause I think this needs to go in with us not having a meet for a while

Signed-off-by: Tihomir Surdilovic <tihomir@temporal.io>
@tsurdilo
Copy link
Contributor

tsurdilo commented May 1, 2021

Will go ahead and merge this . no need to wait long time for our next meet for it

@tsurdilo tsurdilo merged commit 99d3f21 into serverlessworkflow:main May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: spec Changes in the Specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants