-
-
Notifications
You must be signed in to change notification settings - Fork 54
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
feat: add amqp binding 0.4.0 #555
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
Reference: RabbitMQ
What do you think about next format: Proposed change is: {
"bindings": {
"amqp": {
"is": "routingKey",
"name": "black",
"channel": {
"$ref": "#/components/channels/queue-update"
},
"exchange": {
"name": "CRUD-topic-exchange-1",
"type": "topic",
"durable": true,
"autoDelete": false,
"vhost": "/"
},
"bindingVersion": "0.4.0"
}
}
} add |
What are the next steps? (Do I need to ping someone/post somewhere for a review) The channel binding contains the two new fields ˋnameˋ and ˋchannelˋ, which @Pakisan mentions in the above comment. |
@timonback I'll approve it tomorrow, after test implementation |
@Pakisan Is there something I can do to support? |
@timonback Merge latest master, where I have enabled json schemas test I'll attach patch with test to add and it's done Sorry for delay. I forget to write you back about changes to pull Did you check it in SpringWolf? Everything is working like expected? |
Great. I just rebased the branch, tests are still green - however amqp 0.4.0 is not under test. I assume that is what you Yes, the implementation on the Springwolf side is done. CRUD-topic-exchange-2_id:
address: CRUD-topic-exchange-2
messages:
io.github.springwolf.examples.amqp.dtos.ExamplePayloadDto:
$ref: "#/components/messages/io.github.springwolf.examples.amqp.dtos.ExamplePayloadDto"
bindings:
amqp:
channel:
$ref: "#/channels/queue-read-id"
is: routingKey
name: "#"
exchange:
name: CRUD-topic-exchange-2
type: topic
durable: true
autoDelete: false
vhost: /
bindingVersion: 0.4.0 https://github.com/springwolf/springwolf-core/pull/886/files#diff-678a62a10ef6fa6d072912da05e54720cf9c958e85f114ec5aafad5580384f43R40-R57 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix typo - $ref type
Co-authored-by: Pavel Bodiachevskii <[email protected]>
Quality Gate passedIssues Measures |
Thanks for the help, adding tests for the bindings has paid off. @Pakisan I updated the PR. |
It will be updated on project build npm run build After that new schema will be assembled. With and without references |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once asyncapi/bindings#259 is merged we can merge this 🙂 |
@jonaslagoni My understanding based on @Pakisan comment is, that this is the correct change. The other PR is obsolete, I am happy to close it. |
Bindings is like the spec repository just for bindings. The specification must change before the JSON Schema files. So the markdown files still need to change as they set the "standard" 😊 |
@jonaslagoni Sorry, I am not familiar with the process. How to proceed from here? Merge asyncapi/bindings#259 first, then this PR? Who can approve/merge either of these PRs? |
@jonaslagoni let's allow to merge @timonback is proposing great feature, for their and our community - extend ways do describe interaction with AMQP Lack of documentation it's not blocker for me, because here is already ready PR, which wasn't approved |
As there is no specific codeowners for AMQP, I assume its the core owners: https://github.com/asyncapi/bindings/blob/8ca568958be9316a13514110a4db502bc8d390b8/CODEOWNERS#L9 You need their approval for any merge to happen 🙂 Best way is to spread the word on slack and ping a few codeowners IMO. It's not the documentation thats important @Pakisan, without the bindings merged, there is no feature as it does not exist in the standard. These JSON Schema documents are just tooling. But I agree it should be moved forward ASAP 🙂 |
fyi: @derberg @jonaslagoni i'll merge it at this evening. Everything what's left, you can assign to me and I'll finish it as soon as it possible I need this stuff to present our specification in upcoming talk about Event Mesh, AsyncAPI, Event Clouds and Spring Wolf @timonback how fast new build will be available? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pakisan a codeowner already reviewed asyncapi/bindings#259 (review) and left some comments that need to be addressed.
This PR cannot be merged until the changes of the bindings are accepted therein unfortunately.
@Pakisan I have updated the original PR (asyncapi/bindings#259), which is an incremental improvement, while the Proposed change on the Springwolf side: springwolf/springwolf-core#1033 In case you need something for your presentation, you can checkout the Springwolf PR and run the gradle |
Description
In Java Spring applications, it is possible to define an application that connects to an amqp broker, defines exchanges, queues and listens to incoming message on these queues.
My current understanding of amqp:
In AsyncAPI, the amqp binding supports two types of channels: routingKey (which is an exchange) and queue.
Unfortunately, there is no link between the two.
Proposal:
Add (optional) fields:
name
to specify the actual routing pattern used (in the (topic) exchange or amqp binding)channel.$ref
to connect theis=routingKey
channel type to theis=queue
channel type.I am looking forward to your thoughts.
Please let me know, if there are also tests that need to be updated and/or the amqp v0.4.0 needs to be mentioned somewhere to get bundled.
Example:
(Original PR: asyncapi/bindings#259)