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(Servers): allow to use union type of Server and Reference objects #706

Merged
merged 1 commit into from
Apr 11, 2022

Conversation

char0n
Copy link
Collaborator

@char0n char0n commented Feb 4, 2022

Refs #705

@sonarcloud
Copy link

sonarcloud bot commented Feb 4, 2022

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

@smoya
Copy link
Member

smoya commented Mar 21, 2022

I am not very sure if we should consider this a fix or rather a feature.
Considering we have examples already using this feature, I would say it can stay as bug. However, from the point of view of the spec, this is a new feature.

If it is a new feature, we could include it in 2.4 release (expected to be released in April 2022).

WDYT? cc @derberg @fmvilas

@smoya smoya mentioned this pull request Mar 21, 2022
55 tasks
@smoya
Copy link
Member

smoya commented Mar 24, 2022

If it is a new feature, we could include it in 2.4 release (expected to be released in April 2022).

The reality is that our parser supports this from the very beginning, so we could consider this change just as it is, a fix.
However, It could be considered even as a breaking change, since another tooling may expect only a Server Object but now it can be both Server Object or Reference Object.
This is another example for #688.

I'm up for adding this fix into 2.4 release, what do others think @dalelane @derberg @fmvilas ?

@fmvilas
Copy link
Member

fmvilas commented Mar 24, 2022

To me, this is a typo in the document and I'd not consider it a breaking change. I know being strict, it is but it was never intended to be like this so I'd leave it as a fix.

@derberg
Copy link
Member

derberg commented Mar 24, 2022

I see it as a fix, but fix to the specification should be released as a patch. Since 2.4 is pretty close, best to put it into 2.4 branch than now trigger 2.3.1 as this is not critical. How doesn't it look like in spec's JSON Schema, all good there?

@smoya
Copy link
Member

smoya commented Mar 24, 2022

I see it as a fix, but fix to the specification should be released as a patch. Since 2.4 is pretty close, best to put it into 2.4 branch than now trigger 2.3.1 as this is not critical. How doesn't it look like in spec's JSON Schema, all good there?

We are covered as Server object has the property $ref that is a ReferenceObject. See https://github.com/asyncapi/spec-json-schemas/blob/master/schemas/2.3.0.json#L173-L175.

Is that the best approach? It is consistent with the rest of the objects? This is a topic @jonaslagoni brought in asyncapi/spec-json-schemas#167., but out of the scope of this PR.

@smoya
Copy link
Member

smoya commented Mar 24, 2022

@char0n would you mind changing the target base branch to 2022-04-release so we can include this fix in the 2.4 release happening next month? Thanks!

@smoya
Copy link
Member

smoya commented Apr 6, 2022

@char0n would you mind changing the target base branch to 2022-04-release so we can include this fix in the 2.4 release happening next month? Thanks!

Maybe any code owner can do this? cc @fmvilas @derberg @dalelane 🙏

@char0n
Copy link
Collaborator Author

char0n commented Apr 6, 2022

@smoya sorry for delay - was a little sick. Doing it now.

@char0n char0n changed the base branch from master to 2022-04-release April 6, 2022 14:05
@char0n
Copy link
Collaborator Author

char0n commented Apr 6, 2022

base changed

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! 🚀 🌔

However, you will need approval from at least one code owner. cc @dalelane @derberg @fmvilas

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.

👍

@smoya
Copy link
Member

smoya commented Apr 11, 2022

/rtm

@asyncapi-bot
Copy link
Contributor

Hello, @smoya! 👋🏼
This PR is not up to date with the base branch and can't be merged.
Please update your branch manually with the latest version of the base branch.
PRO-TIP: Add a comment to your PR with the text: /au or /autoupdate and our bot will take care of updating the branch in the future. The only requirement for this to work is to enable Allow edits from maintainers option in your PR.
Thanks 😄

@asyncapi-bot asyncapi-bot merged commit dda545e into asyncapi:2022-04-release Apr 11, 2022
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.3.1-2022-04-release.1 🎉

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.

None yet

6 participants