Add syntax details for attributes in CDEvents #30

afrittoli · 2022-02-21T16:34:05Z

Expand the definition of mandatory attributes by adding syntax
details and examples.

Introduce the "version" as a mandatory attribute.
Add the subject as an optional attribute as discussed in the WG.

Signed-off-by: Andrea Frittoli [email protected]

spec.md

m-linner-ericsson

Loads of questions :)

spec.md

m-linner-ericsson · 2022-02-23T09:48:03Z

spec.md

+
+- Constraints:
+  - REQUIRED
+  - MUST be a non-empty string


When we have it some versions available, should we link to list of valid versions?

Each version of the spec will refer to its own version only, link in L156.
Once we have more than one version, the docs website will include a dropdown with list of available versions.

Line 156 in this document?

Was your idea to have state the version as the only valid value?

My idea with a list was for to ease for the reader finding the valid range of version available

The list of valid version will be listed on top of the documentation, so it should be easy to discover.

The main reason for me is that if I'm browsing a specific version of the spec, I should only use that version in my events. Also I would not like to keep cross-version information in version specific pages.

I'd be happy to include the list of versions in the main README as long as we keep it independent of the versioned docs.

So your idea was to put the spec version as the only valid field?

Maybe add something like

MUST be the version of the event/spec used for producing the event

m-linner-ericsson · 2022-02-23T09:55:53Z

spec.md

+        "taskrun" : {
+          "id": "my-taskrun-123",
+          "task": "my-task",
+          "status": "Running",


I guess we don't need to include this as the event type predicate already tells us this?

Possibly, or it could provide more insight. Again, this matches what we have in the vocabulary today - if we change the vocabulary we need to update these examples accordingly, but for this PR I would not go and touch the vocabulary part

Ok, then you need to help me. Were do we state in the vocabulary that the should have the status here?

I would not include the status field in subject object but only use it to define the subject itself as I guess it will be the same subject object in all events of the same subject disregarding the predicate. For example both taskrun.started and taskrun.finished would have the same subject object.

Ok, then you need to help me. Were do we state in the vocabulary that the should have the status here?

It's in here https://cdevents.dev/docs/core/ but it's only defined for PipelineRun, not for TaskRun. I didn't realise that, it seems to be incomplete. I can use a PipelineRun instead of a TaskRun in the example if you want.

I would not include the status field in subject object but only use it to define the subject itself as I guess it will be the same subject object in all events of the same subject disregarding the predicate. For example both taskrun.started and taskrun.finished would have the same subject object.

Can we discuss the objects in the vocabulary in the next PR once I update those?
If we change the structure or add/remove fields, I will update this example accordingly.

Ok, so you want to include all the pipeline attributes in the subject object? From your comment above I got it that you didn't want to. I would still vote to not include it but if you want to proceed with the PR you can keep it 😄

spec.md

m-linner-ericsson · 2022-02-23T15:49:59Z

spec.md

+  - REQUIRED
+  - MUST be a non-empty string
+
+### OPTIONAL Event Attributes


If you feel this part drags out in time you could maybe move this part to an own PR as the previous items were quite straight forward.

m-linner-ericsson · 2022-02-23T16:06:47Z

spec.md

+  - Considering the event type `dev.cdevents.taskrun.started`, an example of
+    subject, serialised as JSON, is:
+
+    ```json


So in Eiffel we have our two events:

Activity Triggered giving the activity a name, including the possibility to give a category, and also describe how the activity was triggered

Activity Started linking back to the Activity Triggered but containing the execution URI and links to logs to find more information.

In your example I guess the id would be the same as name in our Activity Triggered event. The URL would be the executionUri Activity Started event. I am trying to figure out how the task would map. What would you use it for? The closest thing I can think of is the categories filed in the Activity Triggered event. Would you say the task would be used to group events together?

The ID usually represents the unique identifier of the subject within the event source.
I think all subjects we define will have an ID, but this is defined in the vocabulary really.

Subjects will have different attributes, task is only valid for taskrun for instance.
The task is an identifier of the definition that is executed in the taskrun.
Here I'm just including what we have in the vocabulary today.

In terms "triggered" vs "started" - in CDEvents, they would be predicates and - at least for now - they vary event by event. There may be an opportunity to reduce the number of predicates and make them standard across events, but we'll have to see if it works well or not.

The separation of subjects in "long running" and "run to completion" could help create a standard set of predicates, one for each group... but it may turn out that it's not actually possible.

In Eiffel as we have a triggered event and then link back to it so we don't need to worry about the uniqueness of an ID. So from my view we are talking about:

ID: unique identifier

task: A human friendly display name for the task instance

URL: the link to the task instance

If I understand correctly it would be more logical if task was Run of my-task #123

The data model for PipelineRun and TaskRun was inspired by Tekton - here is what we have today: https://cdevents.dev/docs/core/

In Tekton, Task and Pipeline are the static definition of what a Task and a Pipeline should do.
TaskRun and PipelineRun are executions of a Task and a Pipeline respectively.

In CDEvents we only have the object TaskRun and PipelineRun and they include a few bits of data, including the name of the Task / Pipeline. The URL is an optional field that points to the execution - it could be for instance a deep-link URL into the Tekton dashboard to show the specific PipelineRun.

That said, I just wanted to provide an example based on the existing content of the vocabulary.
I didn't really mean to get into any vocabulary specific discussion for this PR 🙏

I think examples are vital for understanding as the subject object is a new concept and it wasn't that easy to follow your train of thoughts. If you want to keep the example here we could go back and change it later. 😄

I totally agree that examples are vital! I'll try to make more of them in the next PRs and align these accordingly.

Expand the definition of mandatory attributes by adding syntax details and examples. Introduce the "version" as a mandatory attribute. Add the subject as an optional attribute as discussed in the WG. Signed-off-by: Andrea Frittoli <[email protected]>

afrittoli · 2022-02-23T22:03:08Z

Thank you @m-linner-ericsson for the thorough review!
I think I either addressed or replied to all your comments.
I expanded on the source based on the discussion in the working group and the feedback I got from the CloudEvents folks on slack. I think for now we don't need source to be an object- I'd rather have the source being as simple as possible, at least until we hit a use case where we need more.

m-linner-ericsson · 2022-03-02T10:26:40Z

@afrittoli To ease the job of the reviewer it would be nice to have the complete history so you can see what has changed. A humble request would be that, if you want to force push could you force push the branch with all the commits?

afrittoli · 2022-03-02T11:45:37Z

@afrittoli To ease the job of the reviewer it would be nice to have the complete history so you can see what has changed. A humble request would be that, if you want to force push could you force push the branch with all the commits?

Yes, sure - I can do that.

My usual workflow is to force-push because that keeps the git history clean. Unfortunately GitHub, unlike Gerrit, doesn't have a concept of patchsets, only commits, which means that to keep a readable git history one then has to squash the commits into one before merging the PR. Assuming that in most cases we will only have one commit per PR that works relatively well.

spec.md

e-backmark-ericsson · 2022-03-02T15:33:16Z

spec.md

+  `source` + `id` combination.
+- Constraints:
+  - REQUIRED
+  - MUST adhere to the format specified in [RFC 3339](https://datatracker.ietf.org/doc/html/rfc3339)


Just want to mention this, in case someone wonders:
In Eiffel we define the timestamp using UNIX Epoch time (in milliseconds), and the main reason to that is probably as that makes it easier for a machine to calculate the diff between two events, and since the Eiffel events are not primarily intended for human consumption. I guess the benefit of having human readable timestamps should overrule that though, for CDEvents, since it makes it a lot easier for a human to troubleshoot the events.

spec.md

e-backmark-ericsson · 2022-03-02T15:45:25Z

spec.md

@@ -87,17 +87,129 @@ predicate are defined in the [vocabulary](#vocabulary).



There is a mix of markup used in this PR, when it comes to referencing attributes and sections in the document. Some references are encapsulated within back-quotes and some are not. Some references render as active links and some do not. Please streamline that.

I think I've addressed this now, let me know if that looks good

The vocabulary section is in some places referenced with back-quotes and sometimes without back-quotes.

Thanks, fixed that

Co-authored-by: Emil Bäckmark <[email protected]>

Signed-off-by: Andrea Frittoli <[email protected]>

afrittoli commented Feb 22, 2022

View reviewed changes

spec.md Outdated Show resolved Hide resolved

m-linner-ericsson reviewed Feb 23, 2022

View reviewed changes

afrittoli force-pushed the syntax branch from 42aed98 to 8f89952 Compare February 23, 2022 22:00

e-backmark-ericsson requested changes Mar 2, 2022

View reviewed changes

afrittoli and others added 8 commits March 2, 2022 22:48

Update spec.md

87207d7

Co-authored-by: Emil Bäckmark <[email protected]>

Update spec.md

2e8ec96

Co-authored-by: Emil Bäckmark <[email protected]>

Update spec.md

901cb12

Co-authored-by: Emil Bäckmark <[email protected]>

Update spec.md

2f1c3bc

Co-authored-by: Emil Bäckmark <[email protected]>

Update spec.md

94c3bc4

Co-authored-by: Emil Bäckmark <[email protected]>

Update spec.md

882f00e

Co-authored-by: Emil Bäckmark <[email protected]>

Streamline the notation for attributes

2e0a3eb

Signed-off-by: Andrea Frittoli <[email protected]>

Remove back-quotes from vocabulary

d1e4739

Signed-off-by: Andrea Frittoli <[email protected]>

afrittoli mentioned this pull request Mar 4, 2022

Clarify versioning of CDEvents spec and events #32

Closed

afrittoli requested a review from e-backmark-ericsson March 4, 2022 11:22

e-backmark-ericsson approved these changes Mar 4, 2022

View reviewed changes

m-linner-ericsson approved these changes Mar 4, 2022

View reviewed changes

afrittoli merged commit c395b78 into cdevents:main Mar 4, 2022

afrittoli mentioned this pull request Jun 16, 2022

Versioning strategies #43

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add syntax details for attributes in CDEvents #30

Add syntax details for attributes in CDEvents #30

afrittoli commented Feb 21, 2022

m-linner-ericsson left a comment

m-linner-ericsson Feb 23, 2022

afrittoli Feb 23, 2022

m-linner-ericsson Mar 2, 2022

afrittoli Mar 2, 2022

m-linner-ericsson Mar 4, 2022

m-linner-ericsson Feb 23, 2022

afrittoli Feb 23, 2022

m-linner-ericsson Mar 2, 2022

m-linner-ericsson Mar 2, 2022 •

edited

Loading

afrittoli Mar 2, 2022

afrittoli Mar 2, 2022

m-linner-ericsson Mar 4, 2022

m-linner-ericsson Feb 23, 2022

m-linner-ericsson Feb 23, 2022

afrittoli Feb 23, 2022

m-linner-ericsson Mar 2, 2022

afrittoli Mar 2, 2022

m-linner-ericsson Mar 4, 2022

afrittoli Mar 4, 2022

afrittoli commented Feb 23, 2022

m-linner-ericsson commented Mar 2, 2022

afrittoli commented Mar 2, 2022

e-backmark-ericsson Mar 2, 2022

e-backmark-ericsson Mar 2, 2022

afrittoli Mar 3, 2022

e-backmark-ericsson Mar 4, 2022

afrittoli Mar 4, 2022

		@@ -87,17 +87,129 @@ predicate are defined in the [vocabulary](#vocabulary).

Add syntax details for attributes in CDEvents #30

Add syntax details for attributes in CDEvents #30

Conversation

afrittoli commented Feb 21, 2022

m-linner-ericsson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-linner-ericsson Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afrittoli commented Feb 23, 2022

m-linner-ericsson commented Mar 2, 2022

afrittoli commented Mar 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-linner-ericsson Mar 2, 2022 •

edited

Loading