-
Notifications
You must be signed in to change notification settings - Fork 147
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
Invalid do:switch
#894
Comments
Can we do this for sequentiality
and this for concurrency
If we agree on the syntax, I think that will be doable schema wise (by putting do: array as required and onParallel: object as optional of a new schema construct) |
This comment was marked as duplicate.
This comment was marked as duplicate.
@JBBianchi with this example: do:
- processOrder:
switch:
- case1:
when: .orderType == "electronic"
then: processElectronicOrder
- case2:
when: .orderType == "physical"
then: processPhysicalOrder
- default:
then: handleUnknownOrderType
- processElectronicOrder:
do:
- validatePayment: {}
- fulfillOrder: {}
then: exit
- processPhysicalOrder:
branch:
compete: true
on: # `multitask`, `parallelize` or `fork`
- checkInventory: {}
- packItems: {}
- scheduleShipping: {}
then: exit
- handleUnknownOrderType: #... I think you are mixing some proposals together because now here: - processElectronicOrder:
do:
- validatePayment: {}
- fulfillOrder: {}
then: exit
But actually it makes sense here as a substitute for the lost |
But it seems that on this one we are already very close to a consensus:
that includes the main flow, do:
- processOrder:
switch:
- case1:
when: .orderType == "electronic"
then: processElectronicOrder
- case2:
when: .orderType == "physical"
then: processPhysicalOrder
- default:
then: handleUnknownOrderType
- processElectronicOrder:
do: # do instead of execute:sequentially
- validatePayment: {}
- fulfillOrder: {}
then: exit
- processPhysicalOrder: #...
- handleUnknownOrderType: #...
that makes absolute sense to me because in my head "doing a set of tasks in order" should be the default mode for a workflow anyway. We just need to decide on how to substitute all its different use-cases, namely in
that also makes sense and we already have a couple of good ideas how to structure and name it ( |
I think it should also apply to |
agreed. But what should we do about nested sequential tasks? like @JBBianchi tried to show here: - processElectronicOrder:
do:
- validatePayment: {}
- fulfillOrder: {}
then: exit |
Exactly what @JBBianchi proposed I guess: basically use So, to summarize for sake of brievety, here's my understanding of what's proposed:
|
@cdavernas A variant for 4) will be to use do: both for sequential and concurrency, as proposed here #894 (comment). |
Nice idea! I'm however not a fan of the suggestion, for multiple reasons:
|
I am also more a fan of a dedicated task type for parallel execution since
|
I came from an OO world, so let me try to justfy why I feel adding a property to same object where do: is defined is a good alternative to some new keyword that reflect the concept of do concurrently. |
Anyway, I guess we are taking this approach
That also works for me (my previous message was just for the record ;)) |
I'm myself an OO dev, I understand your (very good and valid) point of view. My preference still remains the same. I do not like having the workflow becoming some kind of a do task with additional properties. I do not like having a non verb top level property. I also feel, as said by @matthias-pichler-warrify, that the proposed solution is much more intuitive. It is also much more consistent: feature get its own container class. @JBBianchi @matthias-pichler-warrify @ricardozanini surely we can reach a consensus? |
Thanks for your clarification. Very good points 👍 Although my Java days lie a couple of years in the past, I feel like there is an equally justified but opposite view on this. In Java the fundamental ordered collection is a classDiagram
class Iterable
<<interface>> Iterable
class Collection
<<interface>> Collection
class List
<<interface>> List
class Set
<<interface>> Set
Iterable <|-- Collection
Collection <|-- List
Collection <|-- Set
so I feel like it depends on interpretation where one places
So yeah I would prefer a dedicated keyword. |
@matthias-pichler-warrify |
@matthias-pichler-warrify
or
So, removing "do" from top level, "do" as a task (just rename do as execute to experiment), add a reference to the"task" object as it is currently in the schema from the top. It seems to work. I'm bringing up here because although this is not going to be the new schema, I believe do: (and the keyword for concurrent), in the modified form we are discussing, should be a task. And the top level should refer to either any task or just the do/concurrent task. Basically, its removing do and adding task
I include the whole schema I used for reference |
@fjtirado type: object
required:
- document
properties:
document: {}
use: {}
...
allOf:
- $ref: "#/$defs/task" which given the semantics of JSON schema would be the equivalent but more idiomatic way of doing it. But I thought it was thoroughly discussed that we did not want to make the top level workflow a task itself, because with your schema the following is valid: document:
dsl: 1.0.0-alpha1
namespace: default
name: composite-sequential
version: 1.0.0
call: http
with:
method: get
endpoint: '...' which defines a call task at the top level. Furthermore it does not answer the question what should happen to the other occurrences of |
@matthias-pichler-warrify I understood that it was not possible, sorry |
Nice summary, I think we already reached a consensus then. @JBBianchi are you opening the PR since you opened the issue? @matthias-pichler-warrify @fjtirado I understand that you guys are discussing the implementation in the schema, but the idea has been settled, correct? |
currently writing something on this ... just a sec |
@ricardozanini Yes, Im discussing how to implement the proposal with two keywords, do: and other word for concurrently in the schema |
@fjtirado document: {}
concurrently:
- task1: {}
- task2: {} vs document: {}
do:
- task1:
concurrently:
- task2: {}
- task3: {} |
@matthias-pichler-warrify |
I don't think it's acceptable, for the reasons explained above.
Doing so would forbid the user to use a |
for the implementation in the schema it is relatively straightforward (IMO) $schema: ""
type: object
required:
- document
properties:
document: {}
do:
type: array
items:
type: object
minProperties: 1
maxProperties: 1
additionalProperties:
$ref: "#/$defs/task"
$defs:
task:
type: object
properties: {}
oneOf: [...]
forTask:
type: object
required:
- for
- do
properties:
for: {}
do:
type: array
items:
type: object
minProperties: 1
maxProperties: 1
additionalProperties:
$ref: "#/$defs/task"
tryTask:
type: object
required:
- try
properties:
try:
type: array
items:
type: object
minProperties: 1
maxProperties: 1
additionalProperties:
$ref: "#/$defs/task"
# similar for catch
branchTask:
type: object
required:
- branch
properties:
branch:
type: array
items:
type: object
minProperties: 1
maxProperties: 1
additionalProperties:
$ref: "#/$defs/task"
doTask:
type: object
required:
- do
properties:
do:
type: array
items:
type: object
minProperties: 1
maxProperties: 1
additionalProperties:
$ref: "#/$defs/task" this schema would allow for the following definition document: {}
do:
- branchTask:
branch:
- doTask:
do:
- task1: {}
- task2: {}
- task3: {}
- switchTask:
switch:
- case1:
when: "..."
then: "forTask"
- case2:
when: "..."
then: "tryTask"
- forTask:
for: {}
do:
- task4: {}
- task5: {}
- tryTask:
try:
- task6: {}
- task7: {}
catch:
do:
- task8: {}
- task9: {} |
I would not add
These should IMO not refer to a task object but to a task array. Just as the top level |
100% agree with @matthias-pichler-warrify |
@matthias-pichler-warrify |
I understand, the schema to do this properly would be: $schema: ""
type: object
required:
- document
properties:
document: {}
do:
oneOf:
- $ref: "#/$defs/task"
- $ref: "#/$defs/taskList"
$defs:
task:
type: object
properties: {}
oneOf: [...]
taskList:
type: array
items:
type: object
minProperties: 1
maxProperties: 1
additionalProperties:
$ref: "#/$defs/task"
forTask:
type: object
required:
- for
- do
properties:
for: {}
do:
oneOf:
- $ref: "#/$defs/task"
- $ref: "#/$defs/taskList"
tryTask:
type: object
required:
- try
properties:
try:
oneOf:
- $ref: "#/$defs/task"
- $ref: "#/$defs/taskList"
# similar for catch
branchTask:
type: object
required:
- branch
properties:
branch:
$ref: "#/$defs/taskList"
doTask:
type: object
required:
- do
properties:
do:
$ref: "#/$defs/taskList" we could also implement that and I'd not really have an issue with that but I think it's just weird that although all |
@matthias-pichler-warrify I was thinking in
And then you write
|
except it would be for:
do:
do:
- taks1: {}
- taks2: {} with your schema, you'd need: forTask:
type: object
required:
- for
- do
properties:
for: {}
do:
oneOf:
- $ref: "#/$defs/task"
- $ref: "#/$defs/taskList" and this part is incorrect: $schema: ""
type: object
required:
- document
properties:
document: {}
oneOf:
- $ref: "#/$defs/doTask"
- $ref: "#/$defs/branchTask" it should be $schema: ""
type: object
required:
- document
oneOf:
- $ref: "#/$defs/doTask"
- $ref: "#/$defs/branchTask"
properties:
document: {} |
@matthias-pichler-warrify with that you can write
|
how do I now nest loops? |
@fjtirado I believe we are going in circles ... all this was discussed before |
@matthias-pichler-warrify |
@matthias-pichler-warrify I believe we need a quick chat on slack or zulip, How can I talk with you more fluently? |
it would need to be: - forTask:
for: {}
do:
- forTask2:
for: {}
call: http no I need a |
Slack but how do I join the workspace? |
It's the CNCF slack workspace, channel #serverless-workflow (https://communityinviter.com/apps/cloud-native/cncf I think ?) |
Ok, after discussing it in slack. |
then I will get started with the PR 👍 |
Thank you @matthias-pichler-warrify @fjtirado 🙏 |
we actually still need to pick a name from:
|
I have a little preference for |
@JBBianchi |
I also prefer |
As said by @matthias-pichler-warrify, both Amazon Step Functions and Google Cloud Workflows use the term "branches" to identify the collections of tasks to run in parallel, so I guess On a side note for a plausible next good issue is that the branch task should define an array of branch:
- branch1:
- setFoo:
set:
foo: bar
- branch2:
- setBar:
set:
bar: baz |
interesting 🤔 that could make a lot of sense and is consistent with on another note the current proposal (I think) includes document: {}
do:
- branchTask:
branch:
# compete goes here
on:
- task1: {}
- task2: {} |
👍
I would then suggest using the term do:
- branchTask:
fork:
compete: true
branches:
- task1: {}
- task2: {} And that would also set the ground for defining the |
@cdavernas As you mention since order is not relevant for concurrent, we migth write
|
I'm ok with @cdavernas proposal with @fjtirado suggestion is also valid but I think it can be a bit confusing for the user as task lists are an array everywhere and then in |
I also like @fjtirado the suggestion is valid but a little inconsistent as tasks only appear as arrays now |
@JBBianchi @matthias-pichler-warrify |
#895 should be ready for review 👍 |
What seems off:
After #875, and because of, #884 (comment), #882 was merged, making
do
a single task entry. This breaks the possibility to usedo:switch
as there is nothing to switch to. It forces the user to usedo:execute:sequentially:switch
.What you expected to be:
We might need to rollback to the original idea of #875, which is to have
do
asmap[string, task]
, but not only for top level.Anything else we need to know?:
As mentioned by @matthias-pichler-warrify in #875, making
do
amap[string, task]
, theexecute.sequentially
directive doesn't really make sense anymore. We might want to refactorexecute
at the same time (or keep that for another issue).The text was updated successfully, but these errors were encountered: