Skip to content
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

CloudEvents integration #404

Merged
merged 64 commits into from
Apr 12, 2018
Merged

CloudEvents integration #404

merged 64 commits into from
Apr 12, 2018

Conversation

RaeesBhatti
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Mar 29, 2018

Codecov Report

Merging #404 into master will decrease coverage by 0.93%.
The diff coverage is 70.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #404      +/-   ##
==========================================
- Coverage   65.13%   64.19%   -0.94%     
==========================================
  Files          26       29       +3     
  Lines        1592     1648      +56     
==========================================
+ Hits         1037     1058      +21     
- Misses        512      547      +35     
  Partials       43       43
Impacted Files Coverage Δ
event/http.go 0% <0%> (ø)
internal/http/headers.go 100% <100%> (ø)
router/event.go 84.44% <100%> (-2.13%) ⬇️
event/event.go 69.35% <75%> (ø)
router/router.go 59.27% <83.33%> (-2.99%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ffe17c...f4ae257. Read the comment docs.

@ganeshvlrk
Copy link
Contributor

@mthenw @elsteelbrain I don't see any tests that validate the event format. What are the plans to add the same

Copy link
Contributor

@alexdebrie alexdebrie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First review of changes based on shifting CloudEvents spec.

event/event.go Outdated
ReceivedAt uint64 `json:"receivedAt"`
Data interface{} `json:"data"`
DataType string `json:"dataType"`
Namespace string `json:"namespace"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates from today's CloudEvents meeting: Namespace was removed as a property.

event/event.go Outdated
Source struct {
SourceType string `json:"source-type"`
SourceID string `json:"source-id"`
} `json:"source"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updates from today's CloudEvents meeting: The source object was changed to a string that's a URI.

event/event.go Outdated
SourceID string `json:"source-id"`
} `json:"source"`
EventID string `json:"event-id"`
EventTime uint64 `json:"event-time"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

event-time needs to be in an RFC3339 format. I wonder if it's better to store as a time.Time instance? cc @mthenw

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, it should be time.Time. time.Time marshals to RFC 3339 by default.

event/event.go Outdated
} `json:"source"`
EventID string `json:"event-id"`
EventTime uint64 `json:"event-time"`
ContentType string `json:"content-type"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a schema-url property that's been added to the spec. It's optional and must be a URI.

event/event.go Outdated
EventType: eventType,
EventID: uuid.NewV4().String(),
EventTime: uint64(time.Now().UnixNano() / int64(time.Millisecond)),
ContentType: mime,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New() should include a value for cloud-events-version (0.1) and source (TBD),

Copy link
Contributor

@mthenw mthenw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few things that are missing:

  • update readme with info that we now use Cloud Events format.
  • add integration test (tests package) that checks the CloudEvents format.
  • update source for http and custom event.

@RaeesBhatti
Copy link
Contributor Author

@alexdebrie What would be the value for the source field?

@alexdebrie
Copy link
Contributor

@elsteelbrain Hmm, I'm not sure. The new spec requires a URI, and one example shows http://example.com/repomanager.

@mthenw @ganeshvlrk Any ideas on what would be a good value for source here?

Full description:

  • Type: URI
  • Description: This describes the event producer. Often this will include
    information such as the type of the event source, the organization
    publishing the event, and some unique idenfitiers. The exact syntax and
    semantics behind the data encoded in the URI is event producer defined.
  • Constraints:
    • REQUIRED

@ganeshvlrk
Copy link
Contributor

Based on the definition, I am not sure we can provide sane ootb values here except for cases where we eventually configure the event sources. Should we consider leaving this empty for now?

mthenw
mthenw previously requested changes Apr 3, 2018
docs/api.md Outdated
* `receivedAt` - `number` - the time (milliseconds) when the Event was received by the Event Gateway (provided by the event gateway)
* `data` - type depends on `dataType` - the event payload
* `dataType` - `string` - the mime type of `data` payload
* `event-type` - `string` - the event name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove list of fields from here and leave only example and link to original spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want me to remove the example too or just this list of fields?

docs/api.md Outdated
@@ -16,23 +16,27 @@ for invoking function. By default Events API runs on `:4000` port.

### Event Definition

All data that passes through the Event Gateway is formatted as an Event, based on our default Event schema:
All data that passes through the Event Gateway is formatted as an CloudEvent, based on CloudEvent schema:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"as a CloudEvent"

docs/api.md Outdated
@@ -16,23 +16,27 @@ for invoking function. By default Events API runs on `:4000` port.

### Event Definition

All data that passes through the Event Gateway is formatted as an Event, based on our default Event schema:
All data that passes through the Event Gateway is formatted as an CloudEvent, based on CloudEvent schema:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please specify spec version here.


// Strings is a string array that implements MarshalLogArray.
type Strings []string

// MapStringInterface is a map that implements MarshalLogObject.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move it under MarshalLogArray function

@RaeesBhatti RaeesBhatti dismissed stale reviews from alexdebrie and mthenw April 3, 2018 09:02

Updated as requested

event/event.go Outdated
EventTypeVersion string `json:"eventTypeVersion"`
CloudEventsVersion string `json:"cloudEventsVersion" validate:"required"`
Source string `json:"source" validate:"url,required"`
EventID string `json:"eventId" validate:"required"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eventID, not eventId

event/event.go Outdated
event.Extensions = zap.MapStringInterface{
"eventgateway": map[string]interface{}{
"transformed": true,
"transformation-version": "0.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.1 string is used in few places, please defined it as const

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TransformationVersion

router/event.go Outdated
event.Data = string(body)
}
var sourceURL string
if r.TLS != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking more about this one I think it won't work in all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not?

@serverless serverless deleted a comment from RaeesBhatti Apr 11, 2018
mthenw
mthenw previously requested changes Apr 11, 2018
event/event.go Outdated
event := &Event{
EventType: eventType,
CloudEventsVersion: "0.1",
Source: source + "#transformationVersion=0.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"https://serverless.com/event-gateway#transformationVersion=" + TransformationVersion

event/event.go Outdated
event.Extensions = zap.MapStringInterface{
"eventgateway": map[string]interface{}{
"transformed": true,
"transformation-version": "0.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TransformationVersion

* `receivedAt` - `number` - the time (milliseconds) when the Event was received by the Event Gateway (provided by the event gateway)
* `data` - type depends on `dataType` - the event payload
* `dataType` - `string` - the mime type of `data` payload
All data that passes through the Event Gateway is formatted as a CloudEvent, based on [CloudEvents v0.1 schema](https://github.com/cloudevents/spec/blob/master/spec.md):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexdebrie can you talk a final look here. Should we add something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mthenw @elsteelbrain Docs are fine by me

docs/api.md Outdated
"eventType": "myapp.user.created",
"eventID": "66dfc31d-6844-42fd-b1a7-a489a49f65f3",
"cloudEventsVersion": "0.1",
"source": "https://slsgateway.com/",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elsteelbrain please update the example

event := &Event{
EventType: eventType,
CloudEventsVersion: "0.1",
Source: "https://serverless.com/event-gateway/#transformationVersion=" + TransformationVersion,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://serverless.com/event-gateway without trailing /

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The correct address for the page is https://serverless.com/event-gateway/ with the slash.

event/event.go Outdated
mimeJSON = "application/json"
mimeFormMultipart = "multipart/form-data"
mimeFormURLEncoded = "application/x-www-form-urlencoded"
TransformationVersion = "0.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's define it outside of this mime const, under Type consts.

eventpkg.Event{
EventType: eventpkg.Type("user.created"),
CloudEventsVersion: eventpkg.TransformationVersion,
Source: "https://slsgateway.com#transformationVersion=0.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests have to be updated.

@RaeesBhatti RaeesBhatti dismissed mthenw’s stale review April 11, 2018 17:05

Changes made as requested

@mthenw mthenw merged commit 10ba8a1 into master Apr 12, 2018
@mthenw mthenw deleted the cloudevents branch April 12, 2018 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants