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: allow re-usability of Server Variable Objects #776

Merged

Conversation

char0n
Copy link
Collaborator

@char0n char0n commented May 3, 2022

Refs #775

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 3, 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

@fmvilas
Copy link
Member

fmvilas commented May 5, 2022

Answering your comment in the #775 issue:

I've issued a #776 that remedies the situation and would probably suggest to go for 2.4.1 as soon as possible.

Yeah, I think it's a pretty good use case for a patch version.

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.

Looks good to me. Let's wait for the other code owners to review it.

Would also love @smoya and @danielkocot to have a look since they've been involved in the last release.

@fmvilas
Copy link
Member

fmvilas commented May 5, 2022

/dnm

@magicmatatjahu
Copy link
Member

Do we really have to do 2.4.1? In my opinion, it's more of an oversight and that doesn't change the structure of the specification at all. When OpenAPI 3.1.0 came out, people were still making small improvements to the spec without releasing 3.1.1.

@char0n
Copy link
Collaborator Author

char0n commented May 5, 2022

Do we really have to do 2.4.1? In my opinion

No, it's not really necessary, I've just suggested it given the quick cadence of release process for the spec.

it's more of an oversight and that doesn't change the structure of the specification at all.

IMHO it does change the capabilities of the specification; strictly speaking it extends the capabilities, so feat would be more appropriate, but given the context and the fact that this is oversight I went with fix.

When OpenAPI 3.1.0 came out, people were still making small improvements to the spec without releasing 3.1.1.

Not sure this is correct observation (but I might be mistaken). OAS is considered immutable and the last commit to the 3.1.0 version is actually the release commit. No commits after that.

Commit history for the 3.1.0 spec: https://github.com/OAI/OpenAPI-Specification/commits/main/versions/3.1.0.md

@danielkocot
Copy link

For me it looks also good. Thanks @char0n for extending the idea of reusability.

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!

Even though I don't like the idea of releasing a patch, I agree @char0n it is appropriate in this case. The spec is defined in this repository, and not in the json schema definitions (those are supporting the spec, not defining them IMHO), meaning this should be released as what it is, a fix.
I would not release it as a minor since, even though this introduces a new feature technically speaking, the intention on 2.4.0 was to support this and we missed this change, so It can be considered a bug (ergo this fix).

@char0n
Copy link
Collaborator Author

char0n commented May 5, 2022

The spec is defined in this repository, and not in the json schema definitions (those are supporting the spec, not defining them IMHO)

This has come up multiple times already during different conversations. What is actually the source of truth? I've created an issue to track this unclarity and the PR that tries to remedy it. IMHO the source of truth is always the makrdown specification.

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

releasing a patch doesn't hurt 😄
we just merge and it magically happens 😄

all good on JSON Schema side?

@smoya
Copy link
Member

smoya commented May 9, 2022

releasing a patch doesn't hurt 😄 we just merge and it magically happens 😄

all good on JSON Schema side?

All good as $ref is allowed as additional property: https://github.com/asyncapi/spec-json-schemas/blob/master/schemas/2.4.0.json#L209-L213

@smoya
Copy link
Member

smoya commented Jul 7, 2022

@dalelane @fmvilas your review is needed here. Thanks!

@smoya smoya mentioned this pull request Jul 13, 2022
7 tasks
@char0n char0n mentioned this pull request Sep 13, 2022
61 tasks
@char0n
Copy link
Collaborator Author

char0n commented Sep 13, 2022

Given we've discontinued effort with 2.4.1 release and are going directly for 2.5.0, this PR needs to be send against next-spec and the title needs to changeto feat: allow re-usability of Server Variable Objects

@derberg
Copy link
Member

derberg commented Sep 13, 2022

@char0n yup, I think you should be able to change the base branch

@char0n
Copy link
Collaborator Author

char0n commented Sep 14, 2022

Right, I'll do it right after #831 is merged. I guess it would require re-review of all code owners.

@char0n char0n changed the base branch from master to next-spec September 15, 2022 07:12
@char0n
Copy link
Collaborator Author

char0n commented Sep 15, 2022

Base branch changed for next-spec.

@char0n
Copy link
Collaborator Author

char0n commented Sep 15, 2022

/ready-to-merge

@char0n char0n changed the title fix: allow reusability of Server Variable Objects feat: allow re-usability of Server Variable Objects Sep 15, 2022
@char0n
Copy link
Collaborator Author

char0n commented Sep 15, 2022

Changed title from fix: allow re-usability of Server Variable Objects to feat: allow re-usability of Server Variable Objects. Can I ask somebody to to remove do-not-merge label or merge this PR? Thanks!

@derberg
Copy link
Member

derberg commented Sep 15, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit 5deef1e into asyncapi:next-spec Sep 15, 2022
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.5.0-next-spec.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.

8 participants