Remove reliance on key order in maps/objects by matthias-pichler · Pull Request #882 · serverlessworkflow/specification · GitHub
Skip to content

Remove reliance on key order in maps/objects#882

Merged
cdavernas merged 53 commits into
serverlessworkflow:mainfrom
matthias-pichler:matthias-pichler/remove-reliance-875
Jun 6, 2024
Merged

Remove reliance on key order in maps/objects#882
cdavernas merged 53 commits into
serverlessworkflow:mainfrom
matthias-pichler:matthias-pichler/remove-reliance-875

Conversation

@matthias-pichler

@matthias-pichler matthias-pichler commented Jun 3, 2024

Copy link
Copy Markdown
Collaborator

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

fjtirado commented Jun 3, 2024

Copy link
Copy Markdown
Collaborator

@matthias-pichler

Copy link
Copy Markdown
Collaborator Author

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

what about try?

@fjtirado

fjtirado commented Jun 3, 2024

Copy link
Copy Markdown
Collaborator

@matthias-pichler

matthias-pichler commented Jun 3, 2024

Copy link
Copy Markdown
Collaborator Author

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?

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

@fjtirado

fjtirado commented Jun 3, 2024

Copy link
Copy Markdown
Collaborator

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

Comment thread schema/workflow.yaml Outdated
@matthias-pichler

Copy link
Copy Markdown
Collaborator Author

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

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

@fjtirado

fjtirado commented Jun 3, 2024

Copy link
Copy Markdown
Collaborator

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

@matthias-pichler

Copy link
Copy Markdown
Collaborator Author

@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? 🤔

@matthias-pichler matthias-pichler marked this pull request as ready for review June 3, 2024 17:58
@cdavernas

cdavernas commented Jun 3, 2024

Copy link
Copy Markdown
Member

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

cdavernas commented Jun 3, 2024

Copy link
Copy Markdown
Member

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

cdavernas commented Jun 3, 2024

Copy link
Copy Markdown
Member

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

@matthias-pichler

matthias-pichler commented Jun 3, 2024

Copy link
Copy Markdown
Collaborator Author

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

Copy link
Copy Markdown
Member

Single task imo. Otherwise it defeats composite imo.

@matthias-pichler

matthias-pichler commented Jun 3, 2024

Copy link
Copy Markdown
Collaborator Author

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

@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

fjtirado commented Jun 4, 2024

Copy link
Copy Markdown
Collaborator

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

cdavernas commented Jun 4, 2024

Copy link
Copy Markdown
Member

@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

fjtirado commented Jun 4, 2024

Copy link
Copy Markdown
Collaborator

@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

cdavernas commented Jun 4, 2024

Copy link
Copy Markdown
Member

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

matthias-pichler commented Jun 4, 2024

Copy link
Copy Markdown
Collaborator Author

@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

fjtirado commented Jun 4, 2024

Copy link
Copy Markdown
Collaborator

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

Signed-off-by: Matthias Pichler <m.pichler@warrify.com>
Signed-off-by: Matthias Pichler <m.pichler@warrify.com>
Signed-off-by: Matthias Pichler <m.pichler@warrify.com>
Signed-off-by: Matthias Pichler <m.pichler@warrify.com>
Signed-off-by: Matthias Pichler <m.pichler@warrify.com>
Signed-off-by: Matthias Pichler <m.pichler@warrify.com>
Signed-off-by: Matthias Pichler <m.pichler@warrify.com>
This reverts commit 33488a5.

Signed-off-by: Matthias Pichler <m.pichler@warrify.com>
This reverts commit 7d9807c.

Signed-off-by: Matthias Pichler <m.pichler@warrify.com>
Signed-off-by: Matthias Pichler <m.pichler@warrify.com>
Signed-off-by: Matthias Pichler <m.pichler@warrify.com>
This reverts commit ab2228c.

Signed-off-by: Matthias Pichler <m.pichler@warrify.com>
Signed-off-by: Matthias Pichler <m.pichler@warrify.com>
Signed-off-by: Matthias Pichler <m.pichler@warrify.com>
Signed-off-by: Matthias Pichler <m.pichler@warrify.com>
This reverts commit 0533865.

Signed-off-by: Matthias Pichler <m.pichler@warrify.com>
Signed-off-by: Matthias Pichler <m.pichler@warrify.com>
@matthias-pichler matthias-pichler force-pushed the matthias-pichler/remove-reliance-875 branch from a21a580 to dc721a3 Compare June 5, 2024 17:18
@matthias-pichler

Copy link
Copy Markdown
Collaborator Author

@cdavernas whenever you are ready 👍

@JBBianchi

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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

@cdavernas

Copy link
Copy Markdown
Member

@JBBianchi Yes, please! ❤️

@cdavernas cdavernas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thanks for your work!

@cdavernas cdavernas merged commit 3424d38 into serverlessworkflow:main Jun 6, 2024
@JBBianchi

Copy link
Copy Markdown
Member

@JBBianchi JBBianchi mentioned this pull request Jun 11, 2024
@cdavernas cdavernas mentioned this pull request Jun 13, 2024
8 tasks
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.

Remove reliance on key order in "do"

5 participants