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

docs: adapt AsyncAPI version in Solace example #135

Merged

Conversation

Polo2
Copy link
Contributor

@Polo2 Polo2 commented May 20, 2022

Description

Solace Binding Protocol has been included with release 2.3.0 of AsyncAPI,
with:

Thus, specification example should indicate at least this version.

While here, let's favor last released version 2.4.0, as suggested in #135 (comment)

Note:

Following #135 (comment), this PR include a first commit to fix code owners orders.


Noticed by working on Solace Protocol support on https://bump.sh/asyncapi

Copy link

@github-actions github-actions bot left a 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.

@Polo2 Polo2 changed the title Adapt AsyncAPI version in Solace example docs: Adapt AsyncAPI version in Solace example May 20, 2022
@Polo2 Polo2 changed the title docs: Adapt AsyncAPI version in Solace example docs: adapt AsyncAPI version in Solace example May 20, 2022
@derberg
Copy link
Member

derberg commented May 23, 2022

@MichaelDavisSolace can you please have a look. The PR makes a lot of sense, but you are the correct codeowner that needs to see this

@Polo2 thanks so much, can I ask you a favour, and could you with this PR also fix https://github.com/asyncapi/bindings/blob/master/CODEOWNERS file by moving line 15 before line 9? Normally it should only be @damaru-inc @CameronRushton called out for the review, but the order in CODEOWNERS is broken

@fmvilas
Copy link
Member

fmvilas commented May 23, 2022

May I suggest that you use 2.4.0 for the examples? We released the new version recently.

Polo2 added a commit to Polo2/bindings that referenced this pull request May 24, 2022
Normally it should only be @damaru-inc @CameronRushton called
out for the review, but the order in CODEOWNERS is broken.

cf asyncapi#135 (comment)
@Polo2 Polo2 force-pushed the Adapt-AsyncAPI-version-in-Solace-example branch from 8faaf76 to ce3bdc3 Compare May 24, 2022 14:16
@Polo2
Copy link
Contributor Author

Polo2 commented May 24, 2022

@MichaelDavisSolace can you please have a look. The PR makes a lot of sense, but you are the correct codeowner that needs to see this

@Polo2 thanks so much, can I ask you a favour, and could you with this PR also fix https://github.com/asyncapi/bindings/blob/master/CODEOWNERS file by moving line 15 before line 9? Normally it should only be @damaru-inc @CameronRushton called out for the review, but the order in CODEOWNERS is broken

Thanks for your answer @derberg , I've added an initial commit with the requested fix.

May I suggest that you use 2.4.0 for the examples? We released the new version recently.

Sure, while here let's favor last version 👍

damaru-inc
damaru-inc previously approved these changes May 24, 2022
Polo2 added 2 commits June 1, 2022 14:19
Normally it should only be @damaru-inc @CameronRushton called
out for the review, but the order in CODEOWNERS is broken.

cf asyncapi#135 (comment)
Solace Binding Protocol has been included with release 2.3.0 of AsyncAPI,
with:
- asyncapi/spec@88abbe0
- asyncapi/spec#666

Thus, specification example should indicate at least this version.

While here, let's favor last released version 2.4.0, as suggested by @fmvilas

---

Noticed by working on Solace Protocol support on https://bump.sh/asyncapi
@Polo2 Polo2 force-pushed the Adapt-AsyncAPI-version-in-Solace-example branch from ce3bdc3 to 7cead60 Compare June 1, 2022 12:19
@Polo2
Copy link
Contributor Author

Polo2 commented Jun 1, 2022

Hi @damaru-inc , it looks like I've dismissed your first approval, sorry about that 🙇
I've only rebase the branch on master before push -f it, no change since your first review, may I ask again for your ✅ please ?

@Polo2 Polo2 requested a review from damaru-inc June 1, 2022 12:26
Copy link
Contributor

@damaru-inc damaru-inc left a comment

Choose a reason for hiding this comment

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

It's fine, thanks!

@Polo2
Copy link
Contributor Author

Polo2 commented Jun 2, 2022

cf #135 (comment)

Hi @CameronRushton 👋 ,

Following previous comment from @derberg, I understood something was wrong in code owners orders, and I guess this PR needs your ✅ before being merged.
May I ask you a review here 🙏

@Polo2
Copy link
Contributor Author

Polo2 commented Jun 13, 2022

Hello there,

Sorry if I miss something, but what should I do to allow this PR to be merged ?
Looks like 1 approving review is missing, and I don't know who will be this reviewer 🙇

@derberg
Copy link
Member

derberg commented Jun 13, 2022

@Polo2 normally @MichaelDavisSolace could merge as he is codeowner of the file that you modified. You could also merge as we have process automated and anyone can merge if all checks/approvals are there in place (just look at the last line of my comment - rtm stands for ready to merge)

but yeah, we had this error there in CODEOWNERS file and in this PR still me or @fmvilas had to approve, sorry about that.

/rtm

@asyncapi-bot asyncapi-bot merged commit db5307e into asyncapi:master Jun 13, 2022
@MichaelDavisSolace
Copy link
Contributor

Actually, I've got two github accounts. I use damaru-inc for my personal stuff and for AsyncAPI, and MichaelDavisSolace for Solace work. Sometimes I've made the mistake of committing to AsyncAPI stuff using the latter account. But here, damaru-inc is the codeowner, and I approved it already.

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