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

Move extension serialization into other specs & rename extensions.md #277

Merged
merged 3 commits into from Aug 30, 2018

Conversation

Projects
None yet
@duglin
Collaborator

duglin commented Jul 25, 2018

Replacement for PR #225

Made spec.md into more of an infoSet spec that just defines the properties
of a CloudEvent and abstractly talks about how extension properties can be
defined by others.

Added some text to the primer to help clarify when extensions should be
used and when extra properties are meant to be part of the event
(data) itself.

Renamed "extensions.md" to "documented-extensions.md" because there was some
confusion that CE extensions were only allowed to come from that doc.
Hopefully, renaming it will make it clear that extensions can come from
anythere - not just that doc.

Left each serialization/transport-binding spec to deal with extensions
in their own way. Most say nothing special about them, which means they
will be serialized just like any other CE attribute.

Based on the discussions in:
https://docs.google.com/document/d/1h4A4ys3x6OmfEIv1gqlOt0kroVRfRO3JewlLunlxTIg/edit# ,
while it wasn't unanimous it did seem there was more people leaning towards
serializing extensions like all other attributes due to the pain involved
in: dealing with promotion of extension to 1st class properties, the desire
to pass along all extensions (top-level or nested ones) to backend components,
and support for future attributes added to the spec. See the doc for more
details.

Signed-off-by: Doug Davis dug@us.ibm.com

@duglin duglin force-pushed the duglin:infosetExtensions branch from 830f884 to 45ab43e Jul 25, 2018

@duglin

This comment has been minimized.

Collaborator

duglin commented Jul 25, 2018

The travis failure is expected and will be fixed once this is merged. Its due to the file name.

@inlined

This comment has been minimized.

Contributor

inlined commented Jul 25, 2018

In general I think the word "experimental" is a bit pejorative. It's also potentially misleading since we also put some features in extensions when we know we don't want to be kingmakers (e.g. tracing specs)

@duglin

This comment has been minimized.

Collaborator

duglin commented Jul 25, 2018

I'm open to a better word. Personally, I'm ok with "extensions" but it seems to cause some confusion for some people so I was trying to address that. But maybe if we drop the "extensions" property and bag (in JSON) then it lessens it. Dunno.

@rperelma

This comment has been minimized.

Contributor

rperelma commented Jul 26, 2018

@duglin do you have a link to your 3-slide presentation on extensions?

@duglin

This comment has been minimized.

@duglin duglin force-pushed the duglin:infosetExtensions branch from 45ab43e to d954616 Jul 26, 2018

@duglin

This comment has been minimized.

Collaborator

duglin commented Jul 26, 2018

Based on today's call I renamed "experimental" to "documented-extensions" per @inlined's suggestion. That sounds much better. But if someone has an even better name please speak up.

@shalka

This comment has been minimized.

shalka commented Aug 1, 2018

I spoke with @duglin today and this proposal works for me. Once the changes are pushed to master I'll open a separate PR detailing the OCI use case for extended properties as an example of additional metadata on the event envelope.

Also, I see in the comments on this PR that "experimental" is deprecated in favor of "documented-extensions," but I still see "experimental" in the file diff. Are you going to comb through and update the nomenclature before committing the changes?

@duglin duglin force-pushed the duglin:infosetExtensions branch from d954616 to 0c5da5c Aug 1, 2018

@duglin

This comment has been minimized.

Collaborator

duglin commented Aug 1, 2018

@shalka thanks - i fixed the file name issue.

@hsaliak

This comment has been minimized.

hsaliak commented Aug 2, 2018

I am from the grpc & protobuf team in Google and we are working to define how a protobuf/grpc spec for Cloud Events would look like. We are running into some design considerations around cleanly mapping proto to json. Specifically, we are having some issues around implementing extensions cleanly with proto in a manner that would be idiomatic to protocol buffer users (or potentially other binary serialization formats, and also map cleanly to the canonical json format).

I'll join the working group call to discuss the details, but wanted to bring this to your attention beforehand.

@duglin duglin force-pushed the duglin:infosetExtensions branch from 0c5da5c to 35d33ba Aug 8, 2018

@@ -35,4 +35,4 @@ specific headers).
## Documented Extensions
* [Distributed Tracing](extensions/distributed-tracing.md)

This comment has been minimized.

@rachelmyers

rachelmyers Aug 8, 2018

Contributor

What is this change?

This comment has been minimized.

@duglin

duglin Aug 8, 2018

Collaborator

hmm I have no idea since that line appears both in master and this PR. I wonder if there was a missing LF at the end and my editor added it???

This comment has been minimized.

@cathyhongzhang

cathyhongzhang Aug 9, 2018

Contributor

are we renaming extensions.md to documented-extensions.md?

This comment has been minimized.

@duglin

duglin Aug 9, 2018

Collaborator

yes to address some of your concerns. If you are no longer concerned about it I'm happy to undo the rename.

spec.md Outdated
This specification places no restriction on the type or semantics of the
extension attributes. Each definition of an extensions SHOULD fully
define all aspects of the attribute - e.g. its name, semantic meaning
and possible values. When defining a new extension care SHOULD be taken to

This comment has been minimized.

@rachelmyers

rachelmyers Aug 8, 2018

Contributor

I know this is a nit, but it keeps bugging me. In "Care should be taken", should is about our state of mind (being careful), not about reducing name collisions in the extensions. Because it's not indicating that this is a recommended, not required, part of the spec, it doesn't need to be cap cased.

It could be rewritten to make the should modify how to name the extension, and then it does need to be cap cased. For example, "New extension definitions SHOULD use a name that is descriptive enough to reduce name collisions with other extensions."

This comment has been minimized.

@duglin

duglin Aug 8, 2018

Collaborator

works for me - thanks!

This comment has been minimized.

@duglin

duglin Aug 8, 2018

Collaborator

actually I did add a slight tweak, see if you're ok with it

provides a generic mapping model that applies to all current and future Cloud
Event attributes.
provides a generic mapping model that applies to all current, future and
extension, Cloud Event attributes.

This comment has been minimized.

@rachelmyers

rachelmyers Aug 8, 2018

Contributor

The commas here confuse me, especially the comma after extension. This change doesn't seem important to me, and I'd prefer to remove it. If it's important to others to emphasize that extensions are attributes, could we rewrite it as "[this] applies to all current and future Cloud Event attributes, including extensions"?

This comment has been minimized.

@duglin

duglin Aug 8, 2018

Collaborator

I like your proposed text much better - thanks! I wasn't thrilled with the wording I had.

@@ -40,7 +40,8 @@ interpreted as described in [RFC2119][RFC2119].
This section defines how CloudEvents attributes are mapped to JSON. This
specification does not explicitly map each attribute, but provides a generic
mapping model that applies to all current and future CloudEvents attributes.
mapping model that applies to all current, future and extension, CloudEvents
attributes.

This comment has been minimized.

@rachelmyers

rachelmyers Aug 8, 2018

Contributor

Same comment as above in the AMQP doc:

The commas here confuse me, especially the comma after extension. This change doesn't seem important to me, and I'd prefer to remove it. If it's important to others to emphasize that extensions are attributes, could we rewrite it as "[this] applies to all current and future Cloud Event attributes, including extensions"

This comment has been minimized.

@duglin

duglin Aug 8, 2018

Collaborator

ditto - grabbed your text.

@duglin duglin force-pushed the duglin:infosetExtensions branch from 35d33ba to d87bdb4 Aug 8, 2018

"extensions" : {
"comExampleExtension" : "value"
},
"comExampleExtension" : "value",

This comment has been minimized.

@zpencer

zpencer Aug 9, 2018

Contributor

Previously created messages can suddenly have undefined behaviors if they collide with this extension. At best, the colliding value is of a different type (like a Map), so the previously valid message is now malformed. At worst, the colliding value is of the same type (String) so the reader has no idea that the input is garbage, which leads to undefined behavior.

Granted, even if we confine extensions to a separate bag we can still have collisions. But extensions are understood to be a testing ground that comes with risks, and people can choose to ignore any extension and still have basic functionality. With this PR, we are encouraging people to put arbitrary key/values at the top level with the caveat that at any time the CloudEvent spec may define an official interpretation that's different from theirs. I think restricting extensions to a separate bag would minimize this sort of problem.

This comment has been minimized.

@duglin

duglin Aug 9, 2018

Collaborator

Correct, collisions can happen but no one has expressed a desire to add a ton more properties to the spec - a few perhaps, but not a lot, so its still a pretty small chance. I believe that most 3rd party extensions will have names that are going to be either scoped by some unique part (e.g. company- prefix) or the purpose will be so specific to a usecase that the odds of it being picked up as "generic" and part of the spec are slim - and thus its name should be pretty specific as well.

As for a generic extension, my hope is that people bring those to the WG so we can work on them in our extensions doc. I view this as a similar potential problem as HTTP headers. Sure extension HTTP headers could have collisions with future HTTP spec defined headers but in practice it hasn't really been an issue.

This comment has been minimized.

@cathyhongzhang

cathyhongzhang Aug 9, 2018

Contributor

I am not sure if this is the best place to add a classification bag example. But to avoid people's confusion that only key-value pair format is allowed in the main spec.md and extensions.md, please add the following to this example:
“identityLabels” : {

}

This comment has been minimized.

@duglin

duglin Aug 9, 2018

Collaborator

I'll add a complex type extension to make it clear its not just simple name/value pairs.

### Extension Attributes
CloudEvent producers MAY include additional extension attributes within the
event. This enables event producers, or middleware, to include additional
metadata that might be used for any purpose in the processing of the

This comment has been minimized.

@deissnerk

deissnerk Aug 9, 2018

Contributor

Does this mean that middleware can change or add extension attributes of an event while passing it on?

This comment has been minimized.

@duglin

duglin Aug 9, 2018

Collaborator

IMO yes. To me, middleware can be viewed as first a consumer, and then a producer. How it generates the outgoing event is up to it. If people don't like what it produces they won't use it. :-)

This comment has been minimized.

@deissnerk

deissnerk Aug 9, 2018

Contributor

@duglin In my opinion, consuming and then producing again should lead to a new event with a new eventID. I realize that this question does not directly touch the main issue addressed by this PR. Shall I open a new issue raising the question of immutability of an event?

This comment has been minimized.

@duglin

duglin Aug 9, 2018

Collaborator

@deissnerk yea, please open a new issue for this topic since I agree with you that its an important, but different, topic since this would be a question for us regardless of whether we have a bag or not.

This comment has been minimized.

@deissnerk

deissnerk Aug 16, 2018

Contributor

@duglin
done -> #290

This comment has been minimized.

@duglin

duglin Aug 16, 2018

Collaborator

thanks!

@@ -35,7 +35,8 @@ The following specifications are available:
| **AMQP Event Format** | - | [master](https://github.com/cloudevents/spec/blob/master/amqp-format.md) |
| **AMQP Transport Binding** | - | [master](https://github.com/cloudevents/spec/blob/master/amqp-transport-binding.md) |
There is also the [CloudEvents Extension Attributes](https://github.com/cloudevents/spec/blob/master/extensions.md)
There is also the
[CloudEvents documented extension attributes](https://github.com/cloudevents/spec/blob/master/documented-extensions.md)

This comment has been minimized.

@cathyhongzhang

This comment has been minimized.

@duglin

duglin Aug 9, 2018

Collaborator

yes, it will be fixed once the PR is merged. For now its just within the scope of the branch/PR.

primer.md Outdated
process the event.
should appear in that message. This metadata is meant to be the minimal
set of information needed to route the request to the proper component that
will process the event. So, while this might mean that some of the application

This comment has been minimized.

@cathyhongzhang

cathyhongzhang Aug 9, 2018

Contributor

Metadata is not just used for routing purpose. Should be changed to something like "set of information needed to route the event to the proper component and to facilitate proper processing of the event by the component".

This comment has been minimized.

@duglin

duglin Aug 9, 2018

Collaborator

done

primer.md Outdated
will process the event. So, while this might mean that some of the application
data of the event itself might be duplicated as part of the CloudEvent's set
of properties, this is to be done solely for the purpose of proper delivery
of the message. Data that is meant strictly for use by the component

This comment has been minimized.

@cathyhongzhang

cathyhongzhang Aug 9, 2018

Contributor

I don't think we should restrict the metadata purpose to be solely for routing/delivery. Some of existing CloudEvents metadata are not for routing/delivery purpose.

This comment has been minimized.

@cathyhongzhang

cathyhongzhang Aug 9, 2018

Contributor

Suggest to add the following, which I presented in today's WG meeting, to avoid misunderstanding that only "key-value" pair format is allowed:
Metadata can be in the format of “key-value” pair or in the format of “classification/property bag” at the root level in both the spec.md and the extension spec.

This comment has been minimized.

@duglin

duglin Aug 10, 2018

Collaborator

I modified the text to address your first comment. The 2nd comment is better addressed down below in the "extensions" section and I tried to add some text in there - see what you think.

This comment has been minimized.

@cathyhongzhang

cathyhongzhang Aug 10, 2018

Contributor

@duglin My point is that we should not restrict the format of metadata in main spec.md either. Metadata can be in the format of “key-value” pair or in the format of “classification/property bag” at the root level in both the spec.md and the extension spec.

This comment has been minimized.

@duglin

duglin Aug 10, 2018

Collaborator

This feels like its leaving the boundary of this PR. If you'd like to add some text to the "Type System" part of the spec to make it clear that the types listed there are just the ones we happen to use today and that we could add more later, then please open up a new PR and suggest some text.

Extension attributes to the CloudEvent specification are meant to
be additional metadata that needs to be included to help ensure proper
routing and processing of the CloudEvent. Additional metadata for other
purposes, that is related to the event itself and not needed in the

This comment has been minimized.

@cathyhongzhang

cathyhongzhang Aug 9, 2018

Contributor
  1. Again, why do we restrict the metadata purpose to be just "transportation of the CloudEvent"?
  2. Could you clarify what is meant by "extensibility points of the event itself."?

This comment has been minimized.

@duglin

duglin Aug 10, 2018

Collaborator

I added some text to try to clear it up.

CloudEvent producers MAY include additional extension attributes within the
event. This enables event producers, or middleware, to include additional
metadata that might be used for any purpose in the processing of the
CloudEvent, such as identifying or correlating event sources. See

This comment has been minimized.

@cathyhongzhang

cathyhongzhang Aug 9, 2018

Contributor

I think the identityLabels property will be used by a large number of use cases and thus we might need to define it in the main spec.md as an optional attribute instead of defining it in the extensions.md. Suggest to discuss this in the next week's meeting.

This comment has been minimized.

@duglin

duglin Aug 10, 2018

Collaborator

We have a separate PR to discuss that - let's tackle that after this one is resolved.

spec.md Outdated
This specification places no restriction on the type or semantics of the
extension attributes. Each definition of an extensions SHOULD fully
define all aspects of the attribute - e.g. its name, semantic meaning
and possible values. New extension definitions SHOULD use a name that is

This comment has been minimized.

@cathyhongzhang

cathyhongzhang Aug 9, 2018

Contributor

For some metadata that is in the format of property/classification bag, we should only define the name of the bag and the format of the bag, but leave the values inside the bag to be customizable and added by the event producer. Suggest to remove "Each definition of an extensions SHOULD fully define all aspects of the attribute - e.g. its name, semantic meaning and possible values. ".

To avoid name collision, property will be used by a large number of use cases, then the WG should define a consistent name for that property in the main spec.md.

This comment has been minimized.

@duglin

duglin Aug 10, 2018

Collaborator

I tried to clarify this.

This comment has been minimized.

@cathyhongzhang

cathyhongzhang Aug 10, 2018

Contributor

@duglin Where can I see your new texts and clarifications. Are you going to post a new diff?

This comment has been minimized.

@duglin

duglin Aug 10, 2018

Collaborator

I just pushed a new commit

spec.md Outdated
define all aspects of the attribute - e.g. its name, semantic meaning
and possible values. New extension definitions SHOULD use a name that is
descriptive enough to reduce the chances of name collisions with other
extensions.

This comment has been minimized.

@cathyhongzhang

cathyhongzhang Aug 9, 2018

Contributor

New extension definition should use a name that does not collide with other attributes defined in the spec.md or extensions.md

This comment has been minimized.

@duglin

duglin Aug 10, 2018

Collaborator

I tried to add some text to address this.

spec.md Outdated
"extensions" : {
"comExampleExtension" : "value"
},
"comExampleExtension" : "value",

This comment has been minimized.

@cathyhongzhang

cathyhongzhang Aug 10, 2018

Contributor

To avoid people's confusion that only key-value pair format is allowed in the main spec.md and extensions.md, please add the following to this example:
“identityLabels” : {

}

This comment has been minimized.

@duglin

duglin Aug 10, 2018

Collaborator

added some text here to address it

@rachelmyers

This comment has been minimized.

Contributor

rachelmyers commented Aug 16, 2018

This pull request makes this JSON implementation of the spec incompatible with the JSON generated by Proto. That would lead to us (Google) supporting two conflicting JSON formats for CloudEvents.

Proto doesn’t support arbitrary top-level properties, because binary formats require integer IDs and JSON format requires string IDs. Instead, to implement CloudEvents in Proto, we rely on the spec to specify which top level properties will be included and assign a unique integer ID to each one. If the JSON implementation uses a property bag, we can use the google.protobuf.Struct for the property bag, which already has special treatment in the case of JSON conversions.

In other words, this PR makes CloudEvents practically impossible to implement in Proto so that the JSON is cleaner.

Next week we’ll be ready to present a proof of concept that demonstrates that this issue crops up not just in proto but in any well-typed language as well (e.g. golang).

@duglin

This comment has been minimized.

Collaborator

duglin commented Aug 16, 2018

@clemensv

This comment has been minimized.

Contributor

clemensv commented Aug 16, 2018

@rachelmyers There is no event format for Protobuf in this WG, so your concern is strictly a private one of your implementation. Also if a "JSON generated by Proto" is not compliant to our specification, it's a problem of your tooling and not one of the JSON spec. A Protobuf event format ought to be concerned about the projection of a cloudevent into the Protobuf wire format and not into JSON.

That said, my PoC clearly demonstrates that the concern you raise is unsubstantiated and your statement that an implementation in Proto is impossible is factually false. It's provably possible to take a cloudevent and flow it through JSON>XML>Thrift>Protobuf without fidelity loss with off-the-shelf serializers, while having top-level extensibility in tagged data formats that's simple to use for scripting languages and while having an extension bag in schema-bound data formats and make it all cleanly upgrade between versions as attributes might become promoted into the standard.

The point your raise is at worst a tooling deficiency of Protobuf, but as the prototype shows, it's one that's trivial to work around with a post-serialization step.

@inlined

This comment has been minimized.

Contributor

inlined commented Aug 22, 2018

Hi Clemens, thank you for the PoC. I think the group as a whole (us included) is frustrated because we're speaking past each other on a few things. We've taken this week to guess where that is and our presentation tomorrow will try to be more precise.

The word "proto" is frankly used to mean two related but different things: a .proto file in the Protobuffer IDL and the protocol buffer binary transport format. At Google we work with both so much that we've become used to using the words imprecisely; that's not appropriate in this WG and we'll do better.

It should be no surprise that Google is actively building proto tools (in both senses of the word) for CloudEvents. That's why the gRPC team sent delegates to our f2f meeting and they have been joining our calls. We will send out a PR for a binary proto binding shortly.

Still, there are two distinct concerns that we're trying to raise and only one really has to do with protocol buffers at all:

  1. Any wire protocol or binding that has struct-like features (e.g. proto or C-structs) will necessarily face a breaking change when an extension is ratified to the main spec. We are looking for a commitment from the working group that they would not consider this a minor version. I've shown this issue without using protobuffers in https://github.com/inlined/versioningishard
  2. Binary proto APIs are written in the proto IDL. Because proto is an IDL it also defines JSON interfaces as well. We are hoping for a JSON spec for which we can reverse engineer a proto shape to match. Otherwise Google will not be able to keep from releasing an alternate JSON binding.

I actually think there are two separate issues we need the WG to help us decide:

  1. Will the working group agree to only promote extensions in major revision changes?
  2. Does the working group prefer to have two JSON specs or to have a JSON spec that is proto-compatible?

duglin added some commits Jul 24, 2018

Move extension serialization into other specs & rename extensions.md
Replacement for PR #225

Made spec.md into more of an infoSet spec that just defines the properties
of a CloudEvent and abstractly talks about how extension properties can be
defined by others.

Added some text to the primer to help clarify when extensions should be
used and when extra properties are meant to be part of the event
(`data`) itself.

Renamed "extensions.md" to "experimental.md" because there was some
confusion that CE extensions were only allowed to come from that doc.
Hopefully, renaming it will make it clear that extensions can come from
anythere - not just that doc.

Left each serialization/transport-binding spec to deal with extensions
in their own way. Most say nothing special about them, which means they
will be serialized just like any other CE attribute.

Based on the discussions in:
https://docs.google.com/document/d/1h4A4ys3x6OmfEIv1gqlOt0kroVRfRO3JewlLunlxTIg/edit# ,
while it wasn't unanimous it did seem there was more people leaning towards
serializing extensions like all other attributes due to the pain involved
in: dealing with promotion of extension to 1st class properties, the desire
to pass along all extensions (top-level or nested ones) to backend components,
and support for future attributes added to the spec. See the doc for more
details.

Signed-off-by: Doug Davis <dug@us.ibm.com>
More editorial tweaks
Signed-off-by: Doug Davis <dug@us.ibm.com>
More tweaks
Signed-off-by: Doug Davis <dug@us.ibm.com>

@duglin duglin force-pushed the duglin:infosetExtensions branch from ae803b6 to 5cd8c06 Aug 22, 2018

@duglin

This comment has been minimized.

Collaborator

duglin commented Aug 22, 2018

@inlined couple of questions/comments - and if they're best answered on the call tomorrow that's ok:

on your 1), the notion of "promotion of an extension" might be misleading. From a spec perspective, there isn't the notion of "promotion" to me - its just "adding a new optional property to the spec" since the spec doesn't know about any extension. If it did they wouldn't be extensions they would be optional properties. So, your 1) isn't about "promotion" to me, its banning us from ever adding a new optional property w/o a major version bump and that seems very uncommon. Lots of specs have JSON bindings and don't have this requirement, so I'm not sure what's special about ours.

on your 2) you seem to be suggesting that any valid choice we make w.r.t. JSON serialization that might diverge from what valid choice proto made would cause this split you mentioned. In essence, you're sort of asking us defer our JSON serialization choices to proto. Am I reading that correctly? Perhaps you could list some of the differences that are of concern?

And a tangential question for you: let's assume no one ever defined or used any extension until this WG produced a v1.1 of the spec with a new top-level optional property. What will a v1.0 receiver (using proto) do with that new property?

I'm asking because I only see 2 choice:

  1. ignore it - which, while a valid choice, I think would be a bad UX for the devs using that platform
  2. pass it along to the app and the tooling/libraries/etc... would need to decide how to surface that property. It could choose to do so as a top-level property, or it could choose to put all unknowns into a "unknown properties" type of bag, like proto does (I believe). How that tooling does this is up to it, and just as important, that tooling's versioning scheme is up to it as well. Meaning, it does not necessarily need to be bumped at the same time as our spec. If the tooling changes its interfaces/structures in a way that would break an existing app, then that might require a major version bump of the tooling, but not necessarily of our spec. And this could happen even if we didn't change our list of properties - the tooling could redesign its APIs/schema independent of what we do.
@clemensv

This comment has been minimized.

Contributor

clemensv commented Aug 22, 2018

@inlined what I hear you say is that your tooling isn't able to produce compliant JSON format as per the spec that's in the repo. If that's so, it'll be best to consider using different tooling for reading/writing JSON rather than trying to bend the spec to your tooling.

If the WG believes that the Protobuf binary format is eligible for inclusion into the standard, that will define the binary rendering. I am very doubtful that there will be support for a second JSON event format that exists solely because your tooling can't render the straightforward flat format we have here.

I also really don't understand the minor/major version concern. If there's an addition you make a new schema and version that. Why does it matter whether the change is major/minor?

@duglin

This comment has been minimized.

Collaborator

duglin commented Aug 23, 2018

A vote on this PR was kicked off during the 8/23 call. The follow companies voted during the call:

LGTM: Adobe, IBM, Microsoft
NOT LGTM: Google
Abstain: Nordstrom

Other voting companies (per the attendance tracker) have until 12pm ET next Thursday (start of next call) to vote.

@duglin

This comment has been minimized.

Collaborator

duglin commented Aug 23, 2018

To be clear... to vote add a comment to this PR with your vote (LGTM, NOT LGTM or Abstain). No vote at all == abstain.

@jlbutler

This comment has been minimized.

jlbutler commented Aug 23, 2018

LGTM from Oracle.

@Vlaaaaaaad

This comment has been minimized.

Vlaaaaaaad commented Aug 25, 2018

LGTM

@deissnerk

This comment has been minimized.

Contributor

deissnerk commented Aug 29, 2018

LGTM from SAP
We approve the PR, but we will watch the efforts around CloudEvent SDKs to see, how well the approach of top level extensions works out. We are open to collaborate on SDK development, especially in the space of Golang and gRPC.

@cathyhongzhang

This comment has been minimized.

Contributor

cathyhongzhang commented Aug 29, 2018

LGTM from Huawei. I support most part of this PR, but would like this PR to spell out that instead of one big "extension" bag, we allow multiple classified property bags with their syntax, semantics, and content type clearly defined but leave what goes inside the bag open. If we allow property bags with its type defined but leave what goes inside the bag open, the issues Google brought up w.r.t. gRPC and Protobuf will be gone. gRPC is being adopted by many companies and I would like our CloudEvents to not break them. Another concern is that if we do not allow property bags with its type defined but leave what goes inside the bag open, we could run into proliferation of top-level attributes/key-value pairs.

Changed the vote to "LGTM" since Doug and I will work together on a new PR to add clarification that we allow multiple classified property bags with their syntax, semantics, and content type clearly defined but leave what goes inside the bag open.

@barkerd427

This comment has been minimized.

Contributor

barkerd427 commented Aug 29, 2018

LGTM from NAIC
This was a hard decision. I don't necessarily agree that this PR is the best option for 1.0, but I believe it's the right decision for now. @cathyhongzhang brings up a possible good solution to some of the issues. I think as we begin to work with this spec, we will find the right way to solve these issues in a way where everyone can be happy with the result. To me, the discussion will continue, but we need to move forward with something. In this case, I prefer to move forward with something simple that works well with JSON. I have worked with Protobufs in the past, and understand the challenges being faced as they were fairly finicky. As SDK development advances, we should keep Protobufs in mind so we can solve for that use case as well.

@ac360

This comment has been minimized.

Contributor

ac360 commented Aug 30, 2018

LGTM from Serverless Inc.

@markpeek

This comment has been minimized.

Contributor

markpeek commented Aug 30, 2018

LGTM from VMware. I like and agree with @barkerd427 comment above.

@markito

This comment has been minimized.

Contributor

markito commented Aug 30, 2018

LGTM from Red Hat.

@duglin

This comment has been minimized.

Collaborator

duglin commented Aug 30, 2018

Final vote tally:
LGTM: 10 - Adobe, Huawei, IBM, Microsoft, NAIC, Oracle, SAP, Serverless, VMWare, Vlaad(self)
NOT LGTM: 1 - Google
Formal abstain: 1 - Nordstrom
Non-voting-right member - LGTM: 1 - RedHat

PR as was approved on 8/30

@duglin duglin merged commit 2144543 into cloudevents:master Aug 30, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
DCO All commits have a DCO sign-off from the author
Details

@clemensv clemensv referenced this pull request Sep 4, 2018

Merged

Add protobuf format #295

@duglin duglin deleted the duglin:infosetExtensions branch Sep 20, 2018

@duglin duglin referenced this pull request Nov 30, 2018

Merged

v0.2 #358

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment