Skip to content

Group End and Transition properties into an Outcome object #706

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

Closed
wants to merge 0 commits into from

Conversation

cdavernas
Copy link
Member

@cdavernas cdavernas commented Oct 28, 2022

Many thanks for submitting your Pull Request ❤️!

Please specify parts of this PR update:

  • Specification
  • Schema
  • Examples
  • Extensions
  • Roadmap

Discussion or Issue link:
#684

What this PR does / why we need it:

What?

Group End and Transition properties, in such way that we can address it as an (activity | state | workflow) outcome.

Why?

Because End and Transition properties are mutually exclusive, and implicitly defines the outcome of an (activity | state | workflow), it makes sense to call is as such. In addition, it simplifies processing of the whole definition, by checking that an outcome has been specified instead of having to do checks on multiple properties.

  • Easier to validate
  • Simplifies workflow schema (a lot), by getting rid of OneOfs transition/end requirements
  • More ubiquitous

Special notes for reviewers:

Work In Progress. Will require thorough reviewing as this requires a ton of little changes in the specification.md, for instance.

@cdavernas cdavernas changed the title Outcome Group End and Transition properties into an Outcome object Oct 28, 2022
@ricardozanini
Copy link
Member

@cdavernas still WIP?

@tsurdilo tsurdilo changed the title Group End and Transition properties into an Outcome object [WIP] Group End and Transition properties into an Outcome object Nov 28, 2022
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@cdavernas
Copy link
Member Author

@ricardozanini Yes it it. I'm now waiting for #768 to be merged as I'd prefer to avoid having to cherry pick changes to the specification.md 😆

@cdavernas cdavernas changed the title [WIP] Group End and Transition properties into an Outcome object Group End and Transition properties into an Outcome object May 30, 2023
@cdavernas cdavernas added this to the v0.9 milestone May 30, 2023
@cdavernas cdavernas self-assigned this May 30, 2023
@cdavernas cdavernas linked an issue May 30, 2023 that may be closed by this pull request
@cdavernas
Copy link
Member Author

Outcome schema is incomplete as far as I can see.
In addition, two approaches are possible here:

  1. Simply moving transition and end properties into outcome, just like I did here
  2. Merging both properties, like I actually suggested in related issue

Not sure, but I'm now thinking the second way makes more sense (unhappilly 😭). @ricardozanini @tsurdilo WDYT?

@cdavernas cdavernas added change: feature New feature or request. Impacts in a minor version change area: spec Changes in the Specification impacts SDK 🔧 labels May 30, 2023
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@cdavernas
Copy link
Member Author

cdavernas commented Feb 16, 2024

@ricardozanini @JBBianchi @fjtirado @ElanHasson

The PR's stale, I'll reword it. In the meanwhile, what do you guys prefer?

  1. Move end and transition into a new object, like so:
outcome:
  end:
     terminate: true | false
     produceEvents: {}
     compensate: true | false
  transition:
     nextState: some-state-name
     produceEvents: {}
     compensate: true | false
  1. Merge transition and end into a outcome object, because they both share produceEvents and compensate. This will also allow adding features more easily:
outcome:
  type: end | transition
  produceEvents: {}
  compensate: true | false
  terminate: true | false #only if type `end`
  nextState: some-state-name #only if type `transition`

@cdavernas
Copy link
Member Author

cdavernas commented Feb 16, 2024

Furthermore, I'd like to take the opportunity to rename the transition's nextState property into to, which feels more ubiquitous and call to action like:

transition:
  to: my-next-state

vs

transition:
  nextState: my-next-state

@ricardozanini @JBBianchi @fjtirado @ElanHasson WDYT?

@JBBianchi
Copy link
Member

JBBianchi commented Feb 16, 2024

My preference goes to the second option, merging everything into one outcome object.

In that case, nextState vs to: in the case of a transition, to makes sense. In the case of an outcome, to doesn't sound so great.

@fjtirado
Copy link
Collaborator

@cdavernas @JBBianchi
I think we can skip type and rename outcome to next
Therefore, we have

next: 
  to: <state name>

or

next: 
  terminate: true | false

And you can add publishedEvents and compesate to both of them

@cdavernas
Copy link
Member Author

@fjtirado I love fluent/call to action terminology too, but we should then use it accross the whole state(s) concept (instead of actions, use the term do, for instance - why I'm ok with btw). In the time being, I'd rather stick with a noun.

What about?

outcome:
  transitionTo: my-state-name
  compensate: true

or

outcome:
  end: {} #we cannot use terminate here, because end also defines continueAs
  compensate: true

@fjtirado
Copy link
Collaborator

@cdavernas ok, lets use substantives ;)
why not use "transition" rather than "outcome", so it looks like

transition:
   state: my-state-name

or

transition:
   end: {}

At the end of the day, ending is transitioning to the end ;)

@cdavernas
Copy link
Member Author

@fjtirado transition or next sounds imo like its related to transitioning, and therefore does not encompass both concerns.
And you are right, outcome is definitly not the right term after all.

What about flow or flowControl?

flow:
  to: my-state

or

flow:
  end: {}

I think we still need to address the proper definition of what is state, an action or even a function. This is related to #677 too as well as to your legit concern of denormalizing custom function types etc.

The reason I speak of this is because I believe that what states are is a foggy mix of stateData,flowControlActions(conditions, switch statements, etc.) and even, in some cases, actions (inject, sleep, etc.).

I think this simple problem clearly shows some crack in the consistency of core "components" of the spec which we will need to address at some point anyways. Why not now?

@fjtirado
Copy link
Collaborator

fjtirado commented Feb 16, 2024

Yes, I think we should separate states from flow control.
Right now, if you want to reuse a state, you have to define it twice if the next state is different.
Lets illustrate with an example, we have states A, B and C
And the flow is start->A->B->C-B->end, since we are including transition information in the state, right now we need to define B twice with two different names, when it will be better to define the states in one place and then the "connections" between them

@cdavernas
Copy link
Member Author

Yes, exactly! Something like:

  • States: Represent the state of a workflow at a specific point in time, and is made out of actions with shared data. Should be named using present participles (ex: retrieving-info, registering-user, ...).
  • Flow controls: Used to transition from one state to another or to end workflow execution.
  • Actions/activities: Individual units of work that perform specific tasks or operations within the workflow.

Right?

Where should we even start 😭

@ricardozanini
Copy link
Member

That should be done for a 0.10 version since it is a quite huge change that will break many things :D

@ElanHasson
Copy link
Contributor

Hi,

I'm ok with that change @cdavernas. I don't think it necessarily matters what it's called but to works.

My mental model is very different from the way the spec seems to be written. I view each state as a function with an input tuple of state (data) and the prior states output and an output tuple of flow control (function ptr), new state (data), and output (data).

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 change: feature New feature or request. Impacts in a minor version change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Group End and Transition properties
5 participants