Skip to content

Fixes inconsistencies between listen and emit#926

Closed
JBBianchi wants to merge 0 commit intoserverlessworkflow:mainfrom
neuroglia-io:fix-917-semantic-inconsistencies-listen-vs-emit
Closed

Fixes inconsistencies between listen and emit#926
JBBianchi wants to merge 0 commit intoserverlessworkflow:mainfrom
neuroglia-io:fix-917-semantic-inconsistencies-listen-vs-emit

Conversation

@JBBianchi
Copy link
Member

@JBBianchi JBBianchi commented Jul 18, 2024

Please specify parts of this PR update:

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

Discussion or Issue link:
Closes #917 (and other little minor typo)

What this PR does:

  • Fixes inconsistencies between listen and emit
    • Added new event section in the specification documentation
    • Fixed the sample of emit in the specification documentation
    • Added new event definition in the JSON schema and referenced it in emit.event.with and eventFilter.with
  • Fixes a typo in the set feature
  • Fixed type of an http call endpoint by adding string
  • Added new (missing) endpoint section in the specification documentation
  • Escaped * character
  • Fixed double https://https://
  • Fixed links to runtime expressions
  • Fixed typo in implictly

Additional information:
-

Copy link
Member

@cdavernas cdavernas left a comment

Choose a reason for hiding this comment

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

Looks great to me, thank you so much! ❤️

Aside from a small typo, I'm wondering whether the term event applies to with.
IMHO, we should find a term that best describes it: as a matter of fact, it's not an event, which is a term that could confuse users, but rather eventProperties or something like that. WDYT?

JBBianchi added a commit to neuroglia-io/serverless-workflow-specification that referenced this pull request Jul 18, 2024
Signed-off-by: Jean-Baptiste Bianchi <jb.bianchi@neuroglia.io>
Copy link
Member

@cdavernas cdavernas left a comment

Choose a reason for hiding this comment

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

Thank you so much bro! ❤️

@ricardozanini
Copy link
Member

@JBBianchi can you please rebase?

Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

Great work, @JBBianchi! Just left a few comments if you don't mind. Also, do you mind adding a few examples to validate the change? Thanks!

dsl-reference.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this \ scape char here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise it breaks the table formatting (like adding an extra cell).

dsl-reference.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the double anchor?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a mistake, good catch.

dsl-reference.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Same here

dsl-reference.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
An event object typically includes details such as the event type, source, timestamp, and unique identifier, along with any relevant data payload. The [Cloud Events specification](https://cloudevents.io/), favored by Serveless Workflow, standardizes this structure to ensure interoperability across different systems and services.
An event object typically includes details such as the event type, source, timestamp, and unique identifier along with any relevant data payload. The [Cloud Events specification](https://cloudevents.io/), favored by Serverless Workflow, standardizes this structure to ensure interoperability across different systems and services.

dsl-reference.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| type | `string` | `no` | Describes the type of event related to the originating occurrence. `source` + `id` is unique for each distinct event.<br>*Required when emitting an event using `emit.event.with`.* |
| type | `string` | `no` | Describes the type of event related to the originating occurrence. <br>*Required when emitting an event using `emit.event.with`.* |

dsl-reference.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Worth linking to https://github.com/cloudevents/spec/blob/main/cloudevents/spec.md#extension-context-attributes. Any properties not listed in the CE spec are considered to be extensions.

dsl-reference.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| from | `string`<br>`object` | `no` | A [runtime expression](dsl.md##runtime-expressions), if any, used to filter and/or mutate the workflow/task input. |
| from | `string`<br>`object` | `no` | A [runtime expression](dsl.md#runtime-expressions), if any, used to filter and/or mutate the workflow/task input. |

dsl-reference.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

So this type in the schema can be the new definition introduced by @matthias-pichler-warrify and also a pattern? Maybe it's worth updating the schema?

dsl-reference.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| as | `string`<br>`object` | `no` | A [runtime expression](dsl.md##runtime-expressions), if any, used to filter and/or mutate the workflow/task output. |
| as | `string`<br>`object` | `no` | A [runtime expression](dsl.md#runtime-expressions), if any, used to filter and/or mutate the workflow/task output. |

Copy link
Member

Choose a reason for hiding this comment

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

This one could be a OneOf of expression, pattern, or eventProperties.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about that one. @cdavernas @fjtirado @matthias-pichler-warrify wdyt ?

@JBBianchi JBBianchi closed this Aug 2, 2024
@JBBianchi JBBianchi force-pushed the fix-917-semantic-inconsistencies-listen-vs-emit branch from 2bab1f5 to f25a9f5 Compare August 2, 2024 09:18
@cdavernas cdavernas deleted the fix-917-semantic-inconsistencies-listen-vs-emit branch August 2, 2024 09:19
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.

Semantic inconsistencies between listen and emit tasks

3 participants

Comments