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

Adjust AMQP spec to changes in #521 #529

Merged
merged 6 commits into from
Oct 14, 2019
Merged

Adjust AMQP spec to changes in #521 #529

merged 6 commits into from
Oct 14, 2019

Conversation

deissnerk
Copy link
Contributor

@deissnerk deissnerk commented Oct 9, 2019

This PR assumes the changes of #521 to be accepted and fixes #519. It renames the AMQP transport binding into protocol mapping.
With the clear definition of event format in the main spec, a separate AMQP event format became obsolete. I copied the type mapping for the attributes over to the AMQP protocol binding, where it is only relevant for the binary mode, and removed the AMQP format.

Copy link
Contributor

@alanconway alanconway left a comment

Choose a reason for hiding this comment

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

LGTM. I suggested some clarifications to apply at your discretion, but I think the text is good as-is.
At least it matches the assumptions of my latest implementation :)

amqp-protocol-binding.md Outdated Show resolved Hide resolved
amqp-protocol-binding.md Outdated Show resolved Hide resolved
amqp-protocol-binding.md Outdated Show resolved Hide resolved
amqp-protocol-binding.md Outdated Show resolved Hide resolved
@duglin duglin added the v1.0 label Oct 9, 2019
@duglin
Copy link
Collaborator

duglin commented Oct 10, 2019

./documented-extensions.md: Can't find: ./amqp-transport-binding.md

bad link now

@duglin duglin mentioned this pull request Oct 10, 2019
Adjusted the protocol binding to the definition of event format that is introduced in #521.

Signed-off-by: Klaus Deissner <klaus.deissner@sap.com>
Removed the separate AMQP format definition

Signed-off-by: Klaus Deissner <klaus.deissner@sap.com>
Worked on comments from the PR

Signed-off-by: Klaus Deissner <klaus.deissner@sap.com>
README.md Outdated Show resolved Hide resolved
Signed-off-by: Klaus Deissner <klaus.deissner@sap.com>
@clemensv
Copy link
Contributor

LGTM

@duglin
Copy link
Collaborator

duglin commented Oct 10, 2019

Conditionally approved on the 10/10 call with a minor wording change that @deissnerk will make today.

While this missed the 2-day deadline the group decided to approve it on the call and give people until end of day tomorrow (friday) to raise any concerns, then when we get 2 LGTMs we'll merge.

… to what was discussed on the WG call.

Signed-off-by: Klaus Deissner <klaus.deissner@sap.com>
Signed-off-by: Klaus Deissner <klaus.deissner@sap.com>
@duglin
Copy link
Collaborator

duglin commented Oct 14, 2019

LGTM
can I get one more?

@markpeek
Copy link
Contributor

LGTM

@duglin
Copy link
Collaborator

duglin commented Oct 14, 2019

thanks @markpeek
merging

@duglin duglin merged commit 861c11b into cloudevents:master Oct 14, 2019
@deissnerk deissnerk deleted the amqp branch February 6, 2020 14:55
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.

Relation between AMQP Format and Transport Binding is unclear
5 participants