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: enable extensions in bindings #179

Closed
wants to merge 3 commits into from

Conversation

derberg
Copy link
Member

@derberg derberg commented Jan 17, 2023

Related asyncapi/spec#893

This PR once and for all makes it clear that bindings are also extendable like other parts of AsyncAPI spec with specification extensions (x-whatever)

  • this PR updates all bindings with info about specification extension
  • this PR bumps versions of bindings and update their schemas

One open questions, something maybe controversial

In this PR I also suggest that text:

This object MUST NOT contain any properties. Its name is reserved for future use.

is replaced everywhere with:

This object does not contain any fixed fields. Its name is reserved for future use.

This object MAY be extended with [Specification Extensions](https://github.com/asyncapi/spec/blob/master/spec/asyncapi.md#specification-extensions).

The fact that we do not have any fixed fields for some binding should not block users from experimenting with extensions. So above change is needed if we want to truly enable extensions in all the bindings.
This change is visible in this commit. I did not provide schemas for these bindings yet, as first I want to be sure it is widely accepted.

Copy link
Collaborator

@VisualBean VisualBean left a comment

Choose a reason for hiding this comment

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

Why are the versions changing for the Json schemas, but nothing actually changed in them?
I believe most, if not all, already had the extension regex.

Copy link
Member Author

derberg commented Jan 18, 2023

@VisualBean yes, you are right, we had extensions enabled on JSON Schema level. But readme text was saying something different. JSON Schema is considered to be a "tool" helpful for validation and other stuff. Individual binding readme is what is the "truth", so if we make some changes there that change behaviour, we need to bump the version of the binding. And yeah, texts in readmes were saying that basically "no props" should be added (which also includes extension).

Does that make sense?

@VisualBean
Copy link
Collaborator

@VisualBean yes, you are right, we had extensions enabled on JSON Schema level. But readme text was saying something different. JSON Schema is considered to be a "tool" helpful for validation and other stuff. Individual binding readme is what is the "truth", so if we make some changes there that change behaviour, we need to bump the version of the binding. And yeah, texts in readmes were saying that basically "no props" should be added (which also includes extension).

Does that make sense?

I'm surprised that the "text" side of the documentation is what is considered the "truth" - with that in mind, the change makes sense.

Copy link
Member Author

derberg commented Jan 18, 2023

@VisualBean yes, mainly because not all the things can be described with JSON Schema.

It is the same with main spec.

this is why also binding's JSON Schema files will be also moved to spec-json-schemas repo -> #113


Thanks so much for the approval and quick reaction 🙇

@VisualBean
Copy link
Collaborator

@VisualBean yes, mainly because not all the things can be described with JSON Schema.

It is the same with main spec.

this is why also binding's JSON Schema files will be also moved to spec-json-schemas repo -> #113


Thanks so much for the approval and quick reaction 🙇

Any time. I think my reason for thinking it was the schema that was the truth, is that it is machine verifiable, so to speak, where the "text" isn't.

Anyway, you are very welcome.

Copy link
Member

@KhudaDad414 KhudaDad414 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@CameronRushton CameronRushton left a comment

Choose a reason for hiding this comment

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

Solace stuff looks good to me. Thanks!

Copy link
Collaborator

@GeraldLoeffler GeraldLoeffler left a comment

Choose a reason for hiding this comment

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

reviewed and approved anypointmq binding

Copy link
Collaborator

@dalelane dalelane left a comment

Choose a reason for hiding this comment

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

makes sense to me 👍

Copy link
Collaborator

@char0n char0n left a comment

Choose a reason for hiding this comment

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

The fact that we do not have any fixed fields for some binding should not block users from experimenting with extensions. So above change is needed if we want to truly enable extensions in all the bindings.

Amen ;]

LGTM and nice work! 🎉

The sentence This object MAY be extended with [Specification Extensions](https://github.com/asyncapi/spec/blob/master/spec/asyncapi.md#specification-extensions) is always in right place, right under the list of fixed fields (if they exists).

@derberg
Copy link
Member Author

derberg commented Feb 6, 2023

only few of you left to accept 😄

@whitlockjc @fmvilas @smoya @lbroudoux @damaru-inc

Copy link
Collaborator

@whitlockjc whitlockjc left a comment

Choose a reason for hiding this comment

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

👍 for googlepubsub.

@derberg
Copy link
Member Author

derberg commented Mar 30, 2023

@lbroudoux @fmvilas @smoya need your approval folks

Copy link
Member

@smoya smoya left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀🌔

Copy link
Collaborator

@lbroudoux lbroudoux left a comment

Choose a reason for hiding this comment

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

Kafka part looks good 🚀

@fmvilas
Copy link
Member

fmvilas commented May 30, 2023

@derberg I think we can merge this now, right?

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

This pull request has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Oct 5, 2023
@github-actions github-actions bot closed this Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.