-
Notifications
You must be signed in to change notification settings - Fork 26
Add introduction to CDEvents #34
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
Conversation
e-backmark-ericsson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I embrace this initiative, and I believe it captures the background well enough as it is now written.
Maybe this page deserves a ToC now as its content has grown quite a lot?
Thanks @m-linner-ericsson ! |
afrittoli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Matthias, it looks good!
I left a few comments
README.md
Outdated
| We thus build on top of the work from other players as the following stack | ||
| pictures exemplifies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would skip this sentence, I think the picture below ties well with the one on L30.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 1ad4def
| @@ -0,0 +1 @@ | |||
| <mxfile host="Electron" modified="2022-03-09T14:09:43.285Z" agent="5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) draw.io/16.5.1 Chrome/96.0.4664.110 Electron/16.0.7 Safari/537.36" etag="WJztc4Q_z2C4iVk_88a6" version="16.5.1" type="device"><diagram id="rZAp1BOKmzg2OGIajMNH" name="Page-1">5ZbdbqMwEIWfhsuVAEM2uUwJTaRVpLap1N1LC0/AG2MjY0rSp69dTAhxK3Wl7Y9aiQv7zNhmvmNsPJSU+6XEVbEWBJgX+mTvoYUXhkEUhp55fHLolJkfd0IuKbFJg7ChD2BF36oNJVCPEpUQTNFqLGaCc8jUSMNSinacthVsvGqFc3CETYaZq95RoopOncb+oK+A5kW/cuDbSIn7ZCvUBSaiPZFQ6qFECqG6VrlPgBl4PZdu3OUL0eOLSeDqNQOK9E+6WM5n02v/7m9THbLfy/aHneUes8YWvLq9vfLCRIu/8HaHu+Z8fX1lq1CHHo0UDSdgZg88dNEWVMGmwpmJtnozaK1QJbNhuw5IBfsXCwiOWPR+AlGCkgedYgegnqTdSnFk++1gTNDnFCemTKyG7V7Ij1MPuHTDEvsHeqFDL2GiIem9rqj+dLyi6UfzQi6vxWeFFX40rNiBdSMaRXnusNJVqzGQWkmxg0QwIbXCBdeZF1vK2JmEGc257jLYmhkMQarPvrmVS0qIWeRZ/oND/v+xIJqcfd9x7FgwecYB9FYOTBwH1lDX5sIwF4nUJ/x3sSKauVYcb7p38eKn48UGSsw1I/fs+KomoLczQXeHP5Gn2Mn/HEofAQ==</diagram></mxfile> No newline at end of file | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for including the source!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😃
spec.md
Outdated
| ## Abstract | ||
|
|
||
| CDEvents is a common specification for Continuous Delivery events. | ||
| CDEvents is a common specification for Continuous Delivery events. For more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not add this to the abstract.
We could link the README and primer docs in the overview below instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to make something in 1ad4def. Do you have any other good ideas?
README.md
Outdated
| * Create a coupled architecture - using point-to-point communication creates a | ||
| tightly intertwined architecture difficult to expand and monitor. | ||
|
|
||
| #### Why not webhooks? |
There was a problem hiding this comment.
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 this. I see webhooks as a way to deliver events, and I didn't know we recommended explicitly against them. CloudEvents include a spec for webhooks https://github.com/cloudevents/spec/blob/v1.0.2/cloudevents/http-webhook.md and they could be an interesting option in cases where authorization is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Felt this was a bit on the edge so I removed it in 1ad4def
Co-authored-by: Emil Bäckmark <emil.backmark@ericsson.com>
spec.md
Outdated
| - The [*vocabulary*](vocabulary), which identifies *event types*, structures as | ||
| *subjects* and *predicates* | ||
|
|
||
| For an introduction see [README.md](README.md) and for more background |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: extra space "for more"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 5bea30c
afrittoli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for all the updates.
Just a nit but looks good to me
|
@e-backmark-ericsson ok to merge? |
| <!-- toc --> | ||
| - [History](#history) | ||
| - [Relations to CloudEvents](#relations-to-cloudevents) | ||
| - [Design reflections](#design-reflections) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ToC entry should be swapped with the previous line to match the actual content
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can run ./tools/update-toc.sh to keep the ToCs updated - I should probably setup a CI job for that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed in 610d408
e-backmark-ericsson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a very small change needed
Adding docs updates from cdevents/spec#34 Signed-off-by: Andrea Frittoli <andrea.frittoli@gmail.com>
Adding docs updates from cdevents/spec#34 Signed-off-by: Andrea Frittoli <andrea.frittoli@gmail.com>
I wanted to create a introduction to CDEvents explaining to a larger audience why we created the project and what it provides.
I have taken inspiration from previous PR material and other markdown documents, but also added a few idea myself.
If you feel the text should live in another document please feel free suggest that.
Signed-off-by: Mattias Linnér mattias.linner@ericsson.com