-
-
Notifications
You must be signed in to change notification settings - Fork 273
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
feat: allow re-usability of Server Variable Objects #776
Conversation
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this 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.
/dnm |
Do we really have to do |
No, it's not really necessary, I've just suggested it given the quick cadence of release process for the spec.
IMHO it does change the capabilities of the specification; strictly speaking it extends the capabilities, so
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 Commit history for the 3.1.0 spec: https://github.com/OAI/OpenAPI-Specification/commits/main/versions/3.1.0.md |
For me it looks also good. Thanks @char0n for extending the idea of reusability. |
There was a problem hiding this 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).
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. |
There was a problem hiding this 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?
All good as |
@char0n yup, I think you should be able to change the base branch |
Right, I'll do it right after #831 is merged. I guess it would require re-review of all code owners. |
Base branch changed for |
/ready-to-merge |
Changed title from |
/rtm |
🎉 This PR is included in version 2.5.0-next-spec.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Refs #775