-
Notifications
You must be signed in to change notification settings - Fork 217
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
Update CloudEvents SDK to support most recent spec change to extensions #2
Conversation
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.
Can you squash your commits and provide a clean commit message? This does seem like quite a big change and I'd like to understand what is going on without having to read the spec (both before and after).
Also, please comment the code a bit more. It's not straightforward, what's being done, and we want an 3rd party to be able to understand. I'll give a more thorough review once there's more commentary.
Great job on the tests.
http.go
Outdated
if ok := ptr.Implements(eventType); ok { | ||
e := reflect.New(t) | ||
version := e.MethodByName("CloudEventVersion").Call([]reflect.Value{})[0].Interface().(string) | ||
println(version) |
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.
Probably want to remove this
http.go
Outdated
e := &v01.Event{} | ||
err := e.FromHTTPRequest(req) | ||
return e, err | ||
eventType := reflect.TypeOf((*Event)(nil)).Elem() |
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 needs comments. I have no idea why we are doing all this reflection
v01/event.go
Outdated
// Data is an optional property | ||
// https://github.com/cloudevents/spec/blob/v0.1/spec.md#data-1 | ||
Data interface{} `json:"data,omitempty"` | ||
cloudEventsVersion string |
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.
Why get rid of the comments?
…erializers/deserializers
8e48d25
to
78075ad
Compare
* Introduce OpenTelemetry observability service (#2) Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com> Co-authored-by: Georg Pirklbauer <georg.pirklbauer@dynatrace.com> Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com> * Upgrade OTel package to 1.0.0 Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com> Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com> Co-authored-by: Georg Pirklbauer <georg.pirklbauer@dynatrace.com>
* allow context to override the default timeout Signed-off-by: Noah Kreiger <noahkreiger@gmail.com>
* allow context to override the default timeout Signed-off-by: Noah Kreiger <noahkreiger@gmail.com> Signed-off-by: James Lewis <james.lewis2@anz.com>
I feel like I may have over engineered this one. Any input is welcome, especially with regards to the end user experience.