-
Notifications
You must be signed in to change notification settings - Fork 9.1k
Make patch optional in openapi field. #4929
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
base: v3.3-dev
Are you sure you want to change the base?
Conversation
I am not in favour of introducing this change for 3.2 and delaying the release again. This is not a review of the change, but a logistical point. Please do not introduce changes for 3.2, and instead target this for 3.3 and we can review it there. |
@lornajane this is not a delay. There is a simple change that we had consensus on at one point. The amount of problems this is causing right now is worth the completely trivial amount of work to merge this PR. |
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.
I'm in favor of this change, though whether it should be in 3.2 or 3.3 MUST be resolved before this is merged.
PLEASE DON'T MERGE this until the discussion on the relevant issues has concluded. |
@karenetheridge this is still a problem regardless of what happens with schema |
Also remove what looked to be a part of the regex allowing for "-rcN" suffixes, which are not mentioned in the field definition at all.
Rebased and force-pushed because of the merge of PR #4927, but there are no actual changes to the commits/text here (both touched the |
@ralfhandl should be fixed now. |
@karenetheridge this PR still allows the point release to be present if people want to do something with it. Without opening another thread here on any other aspects, does that make any difference to you? Nothing you're doing now with point releases would be impossible after this for any OAD author who wants to specify the point release. This PR just allows authors who want to make it very clear that they are using 3.2 in its most clarified/least buggy form, whatever that is, to say so. |
We discussed this in the 9/11/25 TDC meeting and could not get consensus on whether this should go in 3.2 or 3.3. |
I have made my points here and elsewhere. I will be traveling and will not be able to make next week's TDC call, but I encourage folks to make a decision one way or the other and ship the release in my absence. I feel like folks have listened sufficiently to what I have to say on this topic, so I don't need to be present for any final call. Please feel free to edit the PR or replace it with another one if editing this one is inconvenient. I do not need to approve any edits. |
I vote for including this in v3.2. |
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.
I'm in favor of accepting this into 3.2, even though we have struggled to reach consensus. (If we need a formal vote on this issue, that's ok, too!)
The version section of the 3.1 spec states that:
The
major.minor
portion of the version string (for example3.1
) SHALL designate the OAS feature set.
So as a description document author, the patch should not matter to me (as it only clarifies the guidance in the features).
Furthermore:
The patch version SHOULD NOT be considered by tooling, making no distinction between 3.1.0 and 3.1.1 for example.
... since again the patch
only covered clarifications in the specification itself (outside the feature level). We should discourage tools from caring about the patch (as this PR does).
The last consideration I'm aware of is that by making the patch optional, we run the risk of the value being interpreted as a number. This is a problem mostly for hand-written documents. It doesn't seem to me like that's a compelling enough reason to overcome the benefits described above.
1025758
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.
Resolved conflict with editorial PR changing an adjacent line.
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.
Pull Request Overview
This PR makes the patch version optional in the OpenAPI openapi
field to reduce confusion while maintaining semantic version compatibility. The change addresses community feedback where requiring the patch version created unnecessary complexity.
- Updated the regex pattern to make patch version optional (
3.2
or3.2.x
) - Removed support for pre-release suffixes (e.g.,
-rcN
) from the openapi field - Updated documentation to clarify that patch versions should be ignored by tooling
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/schemas/validation/schema.yaml | Updated regex pattern to make patch version optional |
src/oas.md | Updated documentation to clarify openapi field behavior |
tests/schema/pass/*.yaml | Updated test files to use minimal version format "3.2" |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Discussion in the Moonwalk and TSC calls this week and we reached consensus that this should be retargeted to the 3.3 release.
The base branch was changed.
See discussion #4233 and issue #4147. Requiring the patch version is creating tremendous amounts of confusion. While we cannot seem to reach consensus on forbidding it, there has long been a consensus on not requiring it. Let's at least do that.
Also, the regex in the schema allowed for
-something
in theopenapi
field, which is not part of the OAS field definition at all so I removed it. We used to do-rcN
releases, but we have no plan to do so for 3.2 anyway, and we ought not further overload an already confusing mechanism. If we need to note a pre-release version in the future, let's add a field for it.