-
-
Notifications
You must be signed in to change notification settings - Fork 75
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 IBM MQ binding #52
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.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
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.
@rcoppen thanks a lot for this PR. Once we process it and merge consider writing a short blog post about it on asyncapi.com/blog
@fmvilas I think I'll leave it up to you as for me I don't see any bigger issues with this PR, especially that it is an initial release, except for one comment, which I guess means I suck at it and you should have a look 😄
|
||
AsyncAPI Object Fixed Field Name | Reserved Value for IBM MQ Protocol | Description | ||
---|:---|:--- | ||
<a name="serverObjectProtocolFieldValueIbmmq"></a>`server.protocol` | ibmmq | IBM MQ protocol. |
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.
@fmvilas shouldn't we consider this new protocol and binding as something reserved, like names we already have in the spec https://github.com/asyncapi/asyncapi/blob/master/versions/2.0.0/asyncapi.md#server-object and therefor this part should also be extended in the spec? I don't think it is must-have, but nice to have anyway?
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.
Absolutely. In AsyncAPI 1, protocols were reserved because the list was fixed. This changed in v2 and we made a free-form list, i.e., anyone can put there whatever they want. As long as tooling understands it, that's fine. It would be great to have a middle way between v1 and v2 and add some names to a reserved list of names 👍 Should we create a new issue maybe?
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.
@rcoppen would you mind adding a pointer to this document in the main spec? Otherwise, it will not be considered as part of the spec yet. You have to add links here:
- https://github.com/asyncapi/asyncapi/blob/master/versions/2.0.0/asyncapi.md#serverBindingsObject
- https://github.com/asyncapi/asyncapi/blob/master/versions/2.0.0/asyncapi.md#channel-bindings-object
- https://github.com/asyncapi/asyncapi/blob/master/versions/2.0.0/asyncapi.md#operation-bindings-object
- https://github.com/asyncapi/asyncapi/blob/master/versions/2.0.0/asyncapi.md#message-bindings-object
Just follow the same convention we used for the rest of the protocols.
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.
@fmvilas sure, no problem. Is there a convention to the ordering of the protocol list?
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.
Actually, it used to be sorted alphabetically but mercure
got there at the end by mistake. If you can fix it at the same time that would be awesome.
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.
Great work, @rcoppen 👏 I think the structure of your document should serve as a guide for the other protocol bindings defined in this repo.
Added protocol constant that was defined in asyncapi/bindings#52 Contributes to: asyncapi/bindings#48 Signed-off-by: Dale Lane <dale.lane@uk.ibm.com>
Added protocol constant that was defined in asyncapi/bindings#52 Contributes to: asyncapi/bindings#48 Signed-off-by: Dale Lane <dale.lane@uk.ibm.com>
Description
Related issue(s)
Resolves #48