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

Clarify meaning of missing datacontentype. #521

Merged
merged 1 commit into from
Oct 10, 2019

Conversation

alanconway
Copy link
Contributor

Clarify the meaning of missing datacontentype, when it is allowed, and what that means when translating events between formats/protocols.

Also replaced "transport" with "protocol" everywhere, I believe that is now the preferred term. "Transport" is not defined in the terminology.

Fixes #520

Copy link
Contributor

@deissnerk deissnerk left a comment

Choose a reason for hiding this comment

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

LGTM
This definitely simplifies the handling of datacontenttype

spec.md Outdated Show resolved Hide resolved
spec.md Outdated Show resolved Hide resolved
spec.md Outdated Show resolved Hide resolved
@duglin
Copy link
Collaborator

duglin commented Oct 7, 2019

@alanconway you'll need to sign your commits

@alanconway alanconway force-pushed the datacontentype branch 2 times, most recently from 2d36d07 to d56fda1 Compare October 7, 2019 17:16
@alanconway
Copy link
Contributor Author

Took a shot at clarifying - whaddya think now?

I went with SHOULD, but I really don't think it makes sense to translate an event with a well-known, if not explicitly stated datacontentype (JSON) into say an AMQP or HTTP binary message with a blob of bytes and no clues what to do with them.

spec.md Show resolved Hide resolved
spec.md Outdated Show resolved Hide resolved
spec.md Outdated Show resolved Hide resolved
@duglin duglin added the v1.0 label Oct 7, 2019
@duglin
Copy link
Collaborator

duglin commented Oct 7, 2019

Does this address #519 as well?

@alanconway
Copy link
Contributor Author

Does this address #519 as well?

I think it does.
@deissnerk do you agree?

@alanconway alanconway force-pushed the datacontentype branch 2 times, most recently from 5d3b032 to 95c441d Compare October 8, 2019 02:42
@alanconway
Copy link
Contributor Author

CI failure is an unresponsive external link: https://goextend.io
It's still down 503.

@duglin
Copy link
Collaborator

duglin commented Oct 8, 2019

Yeah, I noticed that and was hoping it was a temporary thing but if it lasts a day or two more I'm gonna just remove that link from our docs

@deissnerk
Copy link
Contributor

deissnerk commented Oct 8, 2019

@alanconway I think it is a prerequisite to address #519. If this PR with its definition for formats gets accepted, the AMQP format could even be removed and the type mapping for attributes be moved over to the AMQP protocol binding. As long as the AMQP format remains protocol specific and does not define a content-type on its own, the only AMQP values I could think of, would be String and Binary. I don't see any advantage in this compared to binary mode where CE data is always put into AMQP data.
Btw the term transport occurs in file names, too. Do we need follow-up PRs to adjust terminology in all the protocol bindings?

@alanconway
Copy link
Contributor Author

@alanconway I think it is a prerequisite to address #519. If this PR with its definition for formats gets accepted, the AMQP format could even be removed and the type mapping for attributes be moved over to the AMQP protocol binding.

That would be my preference.

As long as the AMQP format remains protocol specific and does not define a content-type on its own, the only AMQP values I could think of, would be String and Binary. I don't see any advantage in this compared to binary mode where CE data is always put into AMQP data.

The current spec suggests (and I agree) that you set content-type and use a single data section for the body. That means that AMQP types (even binary and string) are never used as data - there are no value-section messages. I would be forgiving on incoming messages and accept string or binary value-sections as if they had been data sections - some AMQP APIs hide those distinctions from the user.

Btw the term transport occurs in file names, too. Do we need follow-up PRs to adjust terminology in all the protocol bindings?

That's above my pay-grade guvnor.

@duglin duglin mentioned this pull request Oct 10, 2019
@duglin
Copy link
Collaborator

duglin commented Oct 10, 2019

rebase needed

Clarify the meaning of missing datacontentype, when it is allowed, and
what that means when translating events between formats/protocols.

Also replaced "transport" with "protocol" everywhere, I believe that
is now the preferred term. "Transport" is not defined in the terminology.

Fixes cloudevents#520

Signed-off-by: Alan Conway <aconway@redhat.com>
@clemensv
Copy link
Contributor

LGTM

@duglin
Copy link
Collaborator

duglin commented Oct 10, 2019

Approved on the 10/10 call

@duglin duglin merged commit 4796626 into cloudevents:master Oct 10, 2019
duglin pushed a commit that referenced this pull request Oct 14, 2019
* Changed name from transport to protocol binding.
Adjusted the protocol binding to the definition of event format that is introduced in #521.

Signed-off-by: Klaus Deissner <klaus.deissner@sap.com>

* Fixed a link
Removed the separate AMQP format definition

Signed-off-by: Klaus Deissner <klaus.deissner@sap.com>

* Fixed links
Worked on comments from the PR

Signed-off-by: Klaus Deissner <klaus.deissner@sap.com>

* Fixed link to already released amqp-transport-binding.md

Signed-off-by: Klaus Deissner <klaus.deissner@sap.com>

* Rephrased a sentence in type mapping section of binary mode according to what was discussed on the WG call.

Signed-off-by: Klaus Deissner <klaus.deissner@sap.com>

* Further refinements

Signed-off-by: Klaus Deissner <klaus.deissner@sap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to determine if data is binary?
4 participants