-
-
Notifications
You must be signed in to change notification settings - Fork 744
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
docs: add document for adding binding page #2042
Conversation
✅ Deploy Preview for shimmering-choux-eb0798 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Let's focus on getting a technical review, we need to make sure the content is technically accurate ✌🏽 |
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.
good work so far, let's take it to the next level and get more feedback :)
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.
Love the direction you took in this document.
Overall improvements:
- for diagrams I suggest using same shapes but with highlighting. I gave such feedback to @TRohit20 some time ago. Would be good to follow the same everywhere docs: tags in a AsyncAPI document #1957 (comment)
- There are also operation level bindings, you did not add them to document
- In Server there is wrong example, AMQP do not have server bindings
- In Message there is wrong example, you have Operation level bindings there
- Please make sure you follow correct indentation in yaml examples,
amqp
and others, are in the end objects, so related binding keys cannot be on the same level
All bindings are in https://github.com/asyncapi/bindings with examples. You can definitely pick different protocol examples per different bindings. I mean AMQP for channel but MQTT for server.
@alequetzalli if you remember, last year Nelson was working on Binding
concept document but in the end it was not created. How is this document related to it? we will just have this one only right?
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.
I made 3 comments but stopped further review
please look again at #2042. You have wrong examples. Always test examples with Studio, it supports 3.0 for very long, not preview but at least validation
there are 2 problems with this PR
to get better examples, please go to https://github.com/asyncapi/bindings, probably clone so it is easier to navigate in vscode, and search for bindings with examples for given binding that you describe. For example Kafka has nice example for Server Bindings, and MQTT might be good for operation binding example. The more diverse examples (different protocols) the better. Remember to use |
instead of making suggestions, I basically made change on my own f4dc937
I'm not sure about diagrams as your look different than https://github.com/asyncapi/website/blob/50e208bf1b20c8280993905a34931b4be6e0fb29/pages/docs/concepts/asyncapi-document/tags.md and I'm confused who is right (even colour do not match). Also asked for help in Slack -> https://asyncapi.slack.com/archives/C02QY9FMM18/p1699980537364299 |
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.
/rtm |
Description
Added Adding binding page.
It is a part of GSoD'23 project.
Related issue(s):
fixes #1713