-
Notifications
You must be signed in to change notification settings - Fork 584
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
Add some guidance on how to construct CloudEvents #404
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 are attempting to write in guidance for namespace shadowing on event types per our conversation during the sdk call.
I strongly believe that it is bad practice to create an event type that corresponds to what the event producer would like to use if they produced cloudevents natively.
For context, this comes from a disagreement that Knative project Event Sources are doing where an event originates from GitHub, GitHub sends the event payload via webhook to an application running in the cluster local. That application marshals the request into a struct (using a lib that is not owned by GitHub) and then takes the GitHub event type and adds it as a post-fix like
dev.knative.eventing.{GitHubEvent}
.Doug believes we should use
com.github.{GitHubEvent}
. The issue is that if GitHub ever decides to produce cloudevents, there could be events in the cluster that match in terms of Source + EventType + ID but do not match in terms of implementation choice for the data payload. GitHub is a good example of a complicated event struct, and they mix event types into an over shadowed struct, plus auth components in the headers. They could choose to implement their payload differently for easier consumption or routing.My argument is the source application that runs in the cluster is not just a simple proxy, it is a full micro-service with logic and choices. We get to drop some of the auth headers because we validate them, we may not forward the payload correctly if GitHub adds or removes parts of their API, they don't control the lib we use...
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.
Actually, that's not quite what I'm advocating. I'm not suggesting the CE generator try to co-opt the event producer's namespace, rather I think the CE generator should reuse data coming from the event producer rather than inventing data that makes it look like the CE is from the CE generator rather than the original event producer.
So, I'm not suggesting to use
com.github.push
, rather it should be justpush
if that is what github lists the event types as. If I mentionedcom.github.xxx
in previous chats it was just as an example of how it should not be something withknative
in it because consumers don't care if knative is in the flow - I was not formally saying it should start withcom.github.
.While in the Kn case it isn't just a 'proxy' because there is some logic involved in generating the CE attributes, it is much closer to a proxy than it is an "event producer" because 1) it doesn't materially change the
data
or shouldn't, and 2) it is generating CE attributes for the purpose of spec compliance, meaning it needs to fill it in with something, and not doing so to convey that this Kn component is in the flow. IOW, the consumer doesn't not, and should not, care that Kn is involved w.r.t. the data it consumes. The app is subscribing to and wants to process Github events, not Kn events.