-
Notifications
You must be signed in to change notification settings - Fork 69
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
feat!: expose a version agnostic event emitter #141
Conversation
@danbev thanks for the review - I think I was a bit early in submitting this. I want to add more tests and docs. Marking as WIP for now. |
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.
@danbev I've incorporated your comments - thanks!
OK - I think I've covered my bases here. Tests and docs added. PTAL. @danbev @helio-frota if you wouldn't mind another review, I'd appreciate it. |
@helio-frota @danbev - I've modified the API a little bit. If you have a chance, PTAL. |
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 think these changes are broadly an improvement, thank you for them.
Two concerns that may exceed the appropriate scope of this PR and should be raised as issues:
As expressed on Slack: those who care about ordering will be excluded by the design assumptions currently encoded here.
Another potential point of exclusion is the explicitly expressed transport. Perhaps a transport agnostic API could be offered that allows the specification of the transport? Of course, it would only support HTTP until an alternative is offered but it seems this base-level API change work should prepare for that future.
.type("com.github.pull.create") | ||
.source("urn:event:from:myapi/resource/123"); | ||
By default, the `HTTPEmitter` will emit events over HTTP POST using the | ||
1.0 specification, in binary mode. You can emit 0.3 events by providing |
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.
"1.0" => "latest supported"?
In general, the version specific comments throughout these docs will have a short shelf life.
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.
@erikerikson that's a valid point. I would like to avoid having that hold up the PR, so I've added a new issue to address it #160. Work for you?
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.
Works for me
This is a breaking change. This commit exposes an HTTP based event emitter that simplifes the API. To use it, simply import the SDK and start emitting. The default spec version is 1.0, but you can use 0.3 by supplying that to the constructor. By default, CloudEvents are emitted in binary mode, but this can be changed by providing the "structured" parameter to the `emit()` function. This commit also eliminates the version specific emitters and receivers from the `v1` and `v03` exports, and eliminates the explicit usage of versioned emitters from `lib/bindings/http`. Signed-off-by: Lance Ball <lball@redhat.com>
Signed-off-by: Lance Ball <lball@redhat.com>
Signed-off-by: Lance Ball <lball@redhat.com>
Signed-off-by: Lance Ball <lball@redhat.com>
Signed-off-by: Lance Ball <lball@redhat.com>
Fixes: cloudevents#149 Signed-off-by: Lance Ball <lball@redhat.com>
9b864b4
to
0b27c61
Compare
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.
Looks like improvements all-around.
This is a breaking change.
This commit exposes an HTTP based event emitter that simplifes the API.
To use it, simply import the SDK and start emitting. The default spec
version is 1.0, but you can use 0.3 by supplying that to the constructor.
By default, CloudEvents are emitted in binary mode, but this can be changed
by providing the "structured" parameter to the
emit()
function.This commit also eliminates the version specific emitters and receivers
from the
v1
andv03
exports, and eliminates the explicit usage ofversioned emitters from
lib/bindings/http
.Fixes: #124
Signed-off-by: Lance Ball lball@redhat.com