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.

Sign up for GitHub

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

Jump to bottom

Clarified MUST requirement for JSON format #644

Merged
merged 2 commits into from
Jun 21, 2020
Merged

Clarified MUST requirement for JSON format #644

merged 2 commits into from
Jun 21, 2020

Conversation

deissnerk
Copy link
Contributor

Addresses #643

@duglin
Copy link
Collaborator

duglin commented Jun 16, 2020

I believe this was the original intent, so LGTM

@deissnerk
Added AMQP,Kafka and MQTT
252efeb 
Signed-off-by: Klaus Deissner <[email protected]>
@deissnerk
Copy link
Contributor Author

deissnerk commented Jun 17, 2020 •
edited
Loading

I realized that the wording is pretty much the same for AMQP, MQTT and Kafka. As MQTT and Kafka do only support structured mode for a certain protocol version, the situation is different there. Nevertheless I think the proposal also fits there.

@clemensv In the MQTT binding I removed a sentence stating that MQTT 5.0 MAY support additional formats. I think that was redundant, as the whole concept of structured mode already suggests that additional formats may be used.

duglin
duglin reviewed Jun 17, 2020
View reviewed changes
kafka-protocol-binding.md
Implementations that use Kafka 0.11.0.0 and above MAY use either _binary_ or
_structured_ modes. Implementations that use Kafka 0.10.x.x and below MUST only
use _structured_ mode and encode the event in JSON. This is because older
versions of Kafka lacked support for message level headers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

just checking... this was just a formatting change, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a typo. the word "use" occurred twice. I thought I could just fix it, while doing my edits.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah missed that - thanks!

@duglin
Copy link
Collaborator

duglin commented Jun 17, 2020

still LGTM

@duglin
Copy link
Collaborator

duglin commented Jun 21, 2020

Approved on 6/18 call

@duglin duglin merged commit bf71de5 into cloudevents:master Jun 21, 2020
n3wscott pushed a commit to n3wscott/cloudevents-spec that referenced this pull request Nov 30, 2020
@deissnerk
Clarified MUST requirement for JSON format (cloudevents#644)
c0577e3 
* Clarified that JSON format is a MUST, if structured content mode is supported.

Signed-off-by: Klaus Deissner <[email protected]>

* Added AMQP,Kafka and MQTT

Signed-off-by: Klaus Deissner <[email protected]>
@duglin duglin mentioned this pull request Dec 7, 2020
Merged
@deissnerk deissnerk deleted the http branch June 25, 2021 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@duglin duglin duglin left review comments

@lance lance lance approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@deissnerk @duglin @lance