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

fix: add missed IBM MQ Operation Binding #840

Merged

Conversation

magicmatatjahu
Copy link
Member

I found that we have forgot to define (probably it's a bug) definition for IBM MQ Operation Binding. I know that some bindings don't need to have definition for particular kind of binding, but we should have reserved keywords.

I found it in review of #836 PR

cc @derberg @fmvilas and @dalelane you should be most interested :)

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@magicmatatjahu magicmatatjahu mentioned this pull request Sep 22, 2022
61 tasks
@char0n
Copy link
Collaborator

char0n commented Sep 22, 2022

@magicmatatjahu referencing also previous discussion about this in #605

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Sep 22, 2022

@char0n Hah, thanks! I didn't see your issue. However @derberg added comment #836 (comment) so I don't know what I should think about this inconsistency across bindings.

@fmvilas
Copy link
Member

fmvilas commented Sep 22, 2022

LGTM but I don't see an "Operation Binding Object" section on the ibmmq binding. We should also add it there and specify that it's just empty and reserved for future use (see example).

@derberg
Copy link
Member

derberg commented Sep 22, 2022

yeah, I think we should make sure ibm mq is there too, for consistency reasons

@magicmatatjahu
Copy link
Member Author

@fmvilas @derberg Pr on bindings repo asyncapi/bindings#149

@fmvilas
Copy link
Member

fmvilas commented Sep 22, 2022

Awesome, thanks @magicmatatjahu. Approved it but let's wait for @rcoppen and others to review it.

@magicmatatjahu
Copy link
Member Author

PR asyncapi/bindings#149 is merged. It can be merged too :)

cc @char0n

@char0n
Copy link
Collaborator

char0n commented Sep 23, 2022

@fmvilas @derberg can you please look into this PR? THanks!

@char0n
Copy link
Collaborator

char0n commented Sep 23, 2022

@magicmatatjahu is there any adaption required for parser-js after this PR is merged?

@magicmatatjahu
Copy link
Member Author

Nope, ParserJS treats bindings atm as Map<string, any> values.

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

oh nice, asyncapi/bindings#149 is merged

lets get it in

@derberg
Copy link
Member

derberg commented Sep 23, 2022

@char0n as Fran wrote Approved it but let's wait for @rcoppen and others to review it., IMHO you can merge as others did review and @rcoppen is aware as he had to approve the change in the binding to get it marget, as he is a codeowner of the IBMMQ binding

@fmvilas
Copy link
Member

fmvilas commented Sep 23, 2022

Yup, feel free to merge.

@char0n
Copy link
Collaborator

char0n commented Sep 24, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit 68d85d1 into asyncapi:next-spec Sep 24, 2022
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.5.0-next-spec.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants