Remove reliance on key order in maps/objects

[matthias-pichler]

Signed-off-by: Matthias Pichler m.pichler@warrify.com

Please specify parts of this PR update:

• Specification
• Schema
• Examples
• Extensions
• Use Cases
• Community
• CTK
• Other

Discussion or Issue link:

Fixes #875

What this PR does:

Additional information:

[fjtirado]

I think we should also change do schema for retries and loops

[matthias-pichler]

I think we should also change do schema for retries and loops

what about try?

[fjtirado]

I mean changing it here https://github.com/serverlessworkflow/specification/blob/main/schema/workflow.yaml#L526 and here https://github.com/serverlessworkflow/specification/blob/main/schema/workflow.yaml#L354
Probably it is better to move do: under $ref

[matthias-pichler]

I mean changing it here https://github.com/serverlessworkflow/specification/blob/main/schema/workflow.yaml#L526 and here https://github.com/serverlessworkflow/specification/blob/main/schema/workflow.yaml#L354 Probably it is better to move do: under $ref

yes already done 👍 but try also takes a task reference ... should that become an array as well?

specification/schema/workflow.yaml

Lines 506 to 507 in 0a48db9

	try:
	$ref: '#/$defs/task'

[fjtirado]

@matthias-pichler-warrify
Yes, in my opinion, in any place where a task is allowed, multiple tasks should be allowed too.

[matthias-pichler]

@fjtirado where does the if come from? Is this a mistake?

specification/dsl-reference.md

Lines 560 to 565 in 2ac27be

	while: .vet != null
	do:
	checkForFleas:
	if: $pet.lastCheckup == null
	listen:
	to:

[fjtirado]

@matthias-pichler-warrify Looks like an unintentional mistake. @cdavernas

[matthias-pichler]

@matthias-pichler-warrify Yes, in my opinion, in any place where a task is allowed, multiple tasks should be allowed too.

I will also update before and after of extensions then 👍

By the way ... I guess the order of extensions matters as well? 🤔

[cdavernas]

should that become an array as well?

No it should not imo and @fjtirado know it's something on which there are diverging opinions

[cdavernas]

I will also update before and after of extensions then 👍
By the way ... I guess the order of extensions matters as well? 🤔

Neither, or the purpose composite task is defeated.

The order is indeed important imo, in a pipeline perspective

[cdavernas]

If is a runtime expression that determines whether or not the task should be run or skipped

[matthias-pichler]

I will also update before and after of extensions then 👍

By the way ... I guess the order of extensions matters as well? 🤔

Neither, or the purpose composite task is defeated.

The irder us indeed important, in a pipeline perspective

ok then:

• extensions will be an array 👍
• before & after will be a single task and one should use composite if needed. understood

what about try, catch and for.do? should they be an array or single task?

[cdavernas]

Single task imo. Otherwise it defeats composite imo.

[matthias-pichler]

Single task imo. Otherwise it defeats composite imo.

ok fine by me

If is a runtime expression yhat déterminés whether or not the task should be run ir skipped

but its nowhere defined ... not in the schema or the dsl

[cdavernas]

but its nowhere defined ... not in the schema or the dsl

Indeed, it seems it left it out. It was a feature already supported in older versions.

[ricardozanini]

@cdavernas

Indeed, it seems it left it out. It was a feature already supported in older versions.

Do we have an issue for this one?

[fjtirado]

In my opinion,
before and after should be single task
but loops if we are using do:, should be arrays. For two reasons:

1. consistency, if as a writer I see a do: Im expecting do to behave equal in all places where is used,
2. because loop iterations, for non trivial workflows, will consisnt on multiple tasks

Anyway, just to find a compromise, I guess that if we are using do, we can include an array of task and if using single task we can put the task just after the for, without the do.
But to use do: with two different schemas just because one is on the top and other is under a loop and based of the personal preference (or feeling) that most loops are going to be a single task is not logical to me.

[cdavernas]

@fjtirado That's your take on it, I respect it, we all do, and that's why you opened a PR for your proposal, which I personnaly find excellent.
Regardless, this is not the objective of that PR, which aims at solving ordering bug.
And as said, I've got a different perspective that I also documented multiple times.
Please let's move that discussion to #869, and take a vote. I'll go happilly with whichever proposal we can get a consensus on.

[fjtirado]

@cdavernas
Once we move from map to array, the main justification for single task loop (not need to name it) is not there, so keeping the inconsistence in the do: schema is not logical
Just for understanding, keeping that incosisntency, is like when defining C Ritchie was forcing the usage of { } (our do: ) in for and while and allowing just one statement there. In reality, they allow one statement without {} and if you want multiple statement you have to put {}.
So either we remove the do: and just use execute: for composition everywhere (include main workflow) or we accept that our do: is indeed equivalent to quick composition (a sequence of sequential task) and in any place where do: appears the user should write a sequence of task.
Hope this clarifies this is not a question of personal preference, but internal schema consistency. Now that we are changing the do: schema, this is the PR to be used.

[cdavernas]

For the tenth time, whatever you think your proposal addresses (the main argument being defeated by renaming for/do), this is NOT what this PR addresses. It is however addressed by the PR you created #869 and by dozens of messages on Slack, in both public and PM. Hell, I even created a poll for it.

[matthias-pichler]

@cdavernas Once we move from map to array, the main justification for single task loop (not need to name it) is not there, so keeping the inconsistence in the do: schema is not logical Just for understanding, keeping that incosisntency, is like when defining C Ritchie was forcing the usage of { } (our do: ) in for and while and allowing just one statement there. In reality, they allow one statement without {} and if you want multiple statement you have to put {}. So either we remove the do: and just use execute: for composition everywhere (include main workflow) or we accept that our do: is indeed equivalent to quick composition (a sequence of sequential task) and in any place where do: appears the user should write a sequence of task. Hope this clarifies this is not a question of personal preference, but internal schema consistency. Now that we are changing the do: schema, this is the PR to be used.

I think your reasoning makes sense. Loops will probably have multiple tasks in them most of the time. I do not have a strong opinion on that so I will go with whatever is decided upon. However I would like to move forward with this PR and I do not think that a timely consensus will be reached here so adding a new issue to 1.0.0-alpha2 for the for.do proposal would be my preference since then a cohesive release can be cut 👍

[fjtirado]

@matthias-pichler-warrify
I also have to implement some stuff and for me is quite important that we have a single do: schema, so I cannot, by any means, accept a release cut and start discussing the do thing during ages as part of another release (the same you cannot deal with an unsorted map, which, by the way, I can).
do: schema MUST be consistent. If you want single task for loops, use a different word than do;

[matthias-pichler]

@cdavernas whenever you are ready 👍

[JBBianchi]

ok, Should I open a new issue for the do: execute:?

I'll try to sum up the ideas and open the issue tomorrow

[cdavernas]

@matthias-pichler-warrify Thanks! Gonna review it first thing tomorrow morning, I'm off for tonight ;)
Many thanks for your awesome work

[cdavernas]

@JBBianchi Yes, please! ❤️

[cdavernas]

LGTM! Thanks for your work!

[JBBianchi]

ok, Should I open a new issue for the do: execute:?

I'll try to sum up the ideas and open the issue tomorrow

Discussion open at #889