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

feat: adapt anypointmq bindings to v3 #203

Merged
merged 10 commits into from
Nov 22, 2023

Conversation

jonaslagoni
Copy link
Member

@jonaslagoni jonaslagoni commented Jun 13, 2023

Description
This PR adapts the anypointmq bindings to v3. This first round of review is just for the general structure changes.

TODO right before final review:

  • Adapt examples, if any that contain v2 structure.
  • Figure out which version change this constitutes

TODO after merge:

  • Adapt schema files in spec-schema-repo

Related issue(s)
Related to #182

@GeraldLoeffler
Copy link
Collaborator

I'd like to try not reviewing this PR myself but instead request that my successors as code owners, namely @jestefaniaaulet and @mihaelmulesoft, review it

@GeraldLoeffler
Copy link
Collaborator

or maybe Mihael goes by @mboss37 these days

@mboss37
Copy link
Collaborator

mboss37 commented Jun 16, 2023

I can review it today.

Copy link
Collaborator

@mboss37 mboss37 left a comment

Choose a reason for hiding this comment

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

Dear @jonaslagoni,

I see that the field name has been updated from 'url' to 'path' and 'pathname', as per your recent changes. However, I've also observed that in the YAML example for the server configuration, 'url' is still being used instead of 'path' and 'pathname'.

Could you kindly clarify if there's a particular reason for maintaining 'url' in the YAML example, despite the changes in the field names?

Thank you!

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Jun 16, 2023

Hey @mboss37,

For the first round of review is to focus on:

This first round of review is just for the general structure changes.

After we agree on the correct change, we can move on to:

TODO right before final review:

  • Adapt examples, if any that contain v2 structure.
  • Figure out which version change this constitutes

I just didn't want to change everything at once if we didn't agree on the core changes 😄

But you are absolutely right, those should change 😄

Copy link
Collaborator

@mboss37 mboss37 left a comment

Choose a reason for hiding this comment

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

@jonaslagoni Ok I agree, we can do it this way.

@jonaslagoni jonaslagoni changed the base branch from master to next-major-spec June 22, 2023 10:22
@jonaslagoni jonaslagoni changed the title feat!: adapt anypointmq bindings to v3 feat: adapt anypointmq bindings to v3 Jun 22, 2023
@jonaslagoni jonaslagoni requested a review from mboss37 June 22, 2023 12:55
@jonaslagoni
Copy link
Member Author

jonaslagoni commented Jun 22, 2023

@mboss37 updated examples and docs where applicable.

Also updated the version number to just be a feature release 🙂

anypointmq/README.md Outdated Show resolved Hide resolved
anypointmq/README.md Outdated Show resolved Hide resolved
@derberg
Copy link
Member

derberg commented Jun 27, 2023

@mboss37 nice to see you here 👋🏼

once this PR is merged can you @GeraldLoeffler update https://github.com/asyncapi/bindings/pull/185/files ? probably Jose should be removed as till not he never contacted the community, at least I haven't seen anything

@jonaslagoni
Copy link
Member Author

ping @mboss37

@mboss37
Copy link
Collaborator

mboss37 commented Aug 24, 2023

@derberg I will start updating the files next week. Was on vacation the last couple of weeks

@smoya
Copy link
Member

smoya commented Sep 13, 2023

@mboss37 Any update on this? 🙏

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Oct 9, 2023

What do we do about this one? cc @derberg

@derberg
Copy link
Member

derberg commented Oct 11, 2023

@jonaslagoni sending an email for help

Copy link
Collaborator

@mboss37 mboss37 left a comment

Choose a reason for hiding this comment

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

Changes reviewed. I will check as next step if pathname needs to be extended as the new anypoint MQ version 4.x had a change in the host configuration.

Anypoint MQ v3 Host Example: https://mq-eu-west-2.anypoint.mulesoft.com/api/v1
Anypoint MQ v4 Host Example: https://mq-eu-west-2.anypoint.mulesoft.com/api/v1/organizations/92761c84-fca0-4643-b18d-a70327387b43/environments/766c627c-2c10-4432-b382-31aa5405a1d6

while the first Id after organizations represents the Mulesoft Org Id and the second Id after environments the MuleSoft specific environment of the destination.

See here: https://docs.mulesoft.com/anypoint-mq-connector/4.x/anypoint-mq-connector-upgrade-migrate

@derberg
Copy link
Member

derberg commented Oct 11, 2023

@mboss37 thanks a lot for the review

@jonaslagoni next-major-spec do not have latest CODEOWNERS from master thus approve from Mihael did not make the PR green yet. Do you plan to update next-major-spec anytime soon?

@jonaslagoni
Copy link
Member Author

@derberg nope, got other things with higher priority right now. I personally will probably not do an update until right before release of v3.

@jonaslagoni
Copy link
Member Author

As @mboss37 already accepted, @derberg do you mind just approving so we can move this along.

#225 is updating the bindings with latest master changes.

@derberg
Copy link
Member

derberg commented Nov 20, 2023

my approval does not matter here too much

@jonaslagoni
Copy link
Member Author

@mboss37 we need your approval again 🙂

@derberg
Copy link
Member

derberg commented Nov 20, 2023

@jonaslagoni

@jonaslagoni next-major-spec do not have latest CODEOWNERS from master thus approve from Mihael did not make the PR green yet. Do you plan to update next-major-spec anytime soon?

so #225 needs to be merged to merge this one

@derberg
Copy link
Member

derberg commented Nov 21, 2023

in theory all is there and we should be ready to merge but @mboss37 did not accept maintainer invite so his approval is still not respected by GH. Sent an email to @mboss37

@mboss37
Copy link
Collaborator

mboss37 commented Nov 21, 2023 via email

@mboss37
Copy link
Collaborator

mboss37 commented Nov 21, 2023

Approval from my side is already there. Approval from @GeraldLoeffler would unblock merging

@derberg
Copy link
Member

derberg commented Nov 21, 2023

@mboss37 you're approval is ok, but GH do not respect it as you did not accept invite to become maintainer in this repo. Email from GitHub probably ended up in spam folder. Just go to https://github.com/asyncapi/bindings/invitations and you should see some button to accept invite or something

@mboss37
Copy link
Collaborator

mboss37 commented Nov 22, 2023 via email

@derberg
Copy link
Member

derberg commented Nov 22, 2023

@jonaslagoni now all green, merge whenever you want

@jonaslagoni
Copy link
Member Author

/rtm

@asyncapi-bot asyncapi-bot merged commit d4b208b into asyncapi:next-major-spec Nov 22, 2023
6 checks passed
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