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: make usage of Schema Object consistent #143

Merged

Conversation

char0n
Copy link
Collaborator

@char0n char0n commented Aug 18, 2022

Description

Main specification document says this:

image

All the Schema Objects in main specification document has following type notation:

image

All binding objects specification documents contain following type notation:

image


From this, it is apparent that type notation for fields using Schema Object, is not consistent within the spec and probably needs to be clarified. There is no reason (at least none that I can see currently) why to constrain binding objects fields to using Schema Object and not allow Reference Object in it's place as well. The AsynAPI specification tells us following: Alternatively, any time a Schema Object can be used, a Reference Object can be used in its place. - this can also be interpreted that using Reference Object in binding objects fields is allowed, but if we check the schemas, they tell us Reference Object is not allowed. As we already stipulated before, the specification is the source of truth and not the schemas.

This PR open possibility to use Reference Object in binding objects fields and updates schemas to use latest versions of AsyncAPI schemas (2.4.0). This also enhanced re-usability aspect of the spec.

Related issue(s)

swagger-api/apidom#1832

kafka/README.md Outdated Show resolved Hide resolved
Co-authored-by: Fran Méndez <[email protected]>
@char0n char0n requested a review from fmvilas September 6, 2022 11:14
@char0n char0n mentioned this pull request Sep 6, 2022
61 tasks
fmvilas
fmvilas previously approved these changes Sep 6, 2022
Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

LGTM 👍

dalelane
dalelane previously approved these changes Sep 6, 2022
@char0n
Copy link
Collaborator Author

char0n commented Sep 9, 2022

@lbroudoux @GeraldLoeffler @derberg @KhudaDad414 would you be able to look at this PR please? THanks!

derberg
derberg previously approved these changes Sep 12, 2022
smoya
smoya previously approved these changes Sep 19, 2022
@char0n
Copy link
Collaborator Author

char0n commented Sep 20, 2022

@KhudaDad414 @lbroudoux @GeraldLoeffler pinging again on this one.

lbroudoux
lbroudoux previously approved these changes Sep 20, 2022
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.

Looks good to me and makes a lot of sense

KhudaDad414
KhudaDad414 previously approved these changes Sep 27, 2022
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!

GeraldLoeffler
GeraldLoeffler previously approved these changes Sep 28, 2022
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.

excellent - thank you!

@char0n
Copy link
Collaborator Author

char0n commented Sep 28, 2022

PR updated with changes from master.

@char0n char0n requested a review from smoya December 19, 2022 14:48
Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

@derberg @dalelane please approve again 🙏

@derberg
Copy link
Member

derberg commented Jan 16, 2023

as multiple bindings were modified we need below folks to approve again:

@char0n
Copy link
Collaborator Author

char0n commented Feb 5, 2023

Haha this PR proved to be pretty difficult to merge ;]

@derberg
Copy link
Member

derberg commented Feb 6, 2023

@smoya
@lbroudoux
@GeraldLoeffler

can you folks have a look 🙏🏼

@char0n
Copy link
Collaborator Author

char0n commented Feb 8, 2023

3 more needed ;]

@derberg
Copy link
Member

derberg commented Feb 8, 2023

@char0n just 2, unless you expect bot approval too 😄

@GeraldLoeffler
Copy link
Collaborator

1 to go ;)

Copy link
Member

derberg commented Mar 9, 2023

@lbroudoux we need ya man

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.

LGTM. Thanks!

@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
@char0n
Copy link
Collaborator Author

char0n commented Nov 2, 2023

/rtm

@asyncapi-bot asyncapi-bot merged commit b1e7736 into asyncapi:master Nov 2, 2023
11 checks passed
@char0n
Copy link
Collaborator Author

char0n commented Nov 2, 2023

Thank you everybody for your patience. Finally merged!

@Tenischev
Copy link
Member

@char0n @derberg @fmvilas
this looks strange
image

@smoya
Copy link
Member

smoya commented Nov 7, 2023

@char0n @derberg @fmvilas this looks strange image

Indeed got merged with the Git merge HEAD marks left. See https://github.com/asyncapi/bindings/pull/143/files#diff-ba2c28c470746625d05c982e58e3b6fdd081ddae606d2918856189bb787c8b4fR148
Good catch @Tenischev!

@char0n are you ok fixing this? Otherwise I'm happy to do it so we do not leave that broken for so much time.

@smoya
Copy link
Member

smoya commented Nov 7, 2023

@char0n are you ok fixing this? Otherwise I'm happy to do it so we do not leave that broken for so much time.

I ended up creating a PR with the fix. #220. Please @char0n review as well 🙏

Copy link
Member

derberg commented Nov 7, 2023

isn't it the same as #219 ?

@char0n
Copy link
Collaborator Author

char0n commented Nov 10, 2023

@derberg yes, #219 and #220 are the same. Author of #219 closed it as he probably saw #220.

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.