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

v3.0.4 schema updates #4132

Open
wants to merge 7 commits into
base: v3.0.4-dev
Choose a base branch
from

Conversation

karenetheridge
Copy link
Member

@karenetheridge karenetheridge commented Oct 10, 2024

Brings the schemas up to date for the v3.0.4 release (see #4125), and adds some support scripts which are used to check for validation errors and inconsistencies between the yaml and json versions of the files.

$ json-schema-eval --validate-schema schemas/v3.0/schema.json
{
  "valid": true
}

$ scripts/compare schemas/v3.0/schema
schemas/v3.0/schema.yaml and schemas/v3.0/schema.json are identical.

@karenetheridge karenetheridge requested review from a team as code owners October 10, 2024 21:09
@karenetheridge karenetheridge changed the base branch from main to v3.0.4-dev October 10, 2024 21:09
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't "need" it. I wrote this script for my own use as I am not a javascript person. I simply thought it would be helpful to commit it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

If we are to accept

or build something similar, the JSON variant becomes a downstream artifact that is always regenerated from the YAML variant.

Copy link
Member Author

Choose a reason for hiding this comment

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

I edited the json files by hand in order to preserve the same property order as was in the yaml files. Autogenerating the json from the yaml would lose that, and cause a much larger diff.

Since the file was almost identical already (and just lacking a few of the recent changes to the yaml file) it was easier to add those by hand and the diff was smaller.

Copy link
Member

Choose a reason for hiding this comment

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

There are definitely order-preserving translators. I believe current versions of JavaScript maintain object property order. If the JSON is in a different order from the YAML, we should probably fix that.

Copy link
Member

@handrews handrews left a comment

Choose a reason for hiding this comment

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

I agree with @ralfhandl 's questions about why we should re-do the format synchronization scripts. Also, as much as I dislike JavaScript, we should have one tooling language if at all possible.

I'd be happy if we changed it, but let's either plan to change it all or plan to keep it, or at least have a clear division of labor (e.g. GitHub Pages requires Ruby to build so we can't get rid of it, but we don't use it otherwise).

On a related note, why do we even check in the JSON version here on main? It's confusing. Why don't we just generate it for publication in the gh-pages branch?

$schema: http://json-schema.org/draft-04/schema#
description: The description of OpenAPI v3.0.x documents, as defined by https://spec.openapis.org/oas/v3.0.3
Copy link
Member

Choose a reason for hiding this comment

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

@karenetheridge I realize you are not the person who put the "as defined by ...v3.0.N" here, but I'd really like to drop it. This is for 3.0. All of 3.0.

@@ -1,6 +1,6 @@
id: https://spec.openapis.org/oas/3.0/schema/2021-09-28
Copy link
Member

@handrews handrews Oct 11, 2024

Choose a reason for hiding this comment

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

I think part of the problem with our publishing process is has to do with this date. I'm fine with manually setting it when we push it out, but I think there was some expectation to set it automatically. Perhaps documented in #3715

Copy link
Member Author

Choose a reason for hiding this comment

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

The file in git should still have an id/$id of some kind though.. perhaps something with "in_progress" or "latest" or something else non-dateish to indicate that it's not reflecting an officially-published version. Otherwise, it's yet one more barrier to tool vendors (on top of converting yaml to json) to make use of the interim file for development and testing.

Copy link
Member

Choose a reason for hiding this comment

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

@karenetheridge I'd lean towards something like "in_progress" that discourages casual use.

There really shouldn't be interim files if our publication process is running smoothly. It might be different for pre-publication releases, but we should handle that separately as that's not a process we've defined at all yet.

In theory, every time we change a published schema, we just re-publish it immediately. If we ever have a reason not to do that, then we probably also don't want people to use the schemas out of main. We really only want the published ones to be usable, I think (again, excluding work-in-progress versions where we just haven't defined a process).

I do agree that if we do want people to use things directly from main, then they need $id/id values that are somehow reasonable, and not identical to any published version.

Copy link
Contributor

Choose a reason for hiding this comment

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

The widely used @apidevtools/openapi-schemas package (1.7 million weekly downloads) seems to pull the OpenAPI schemas directly from main:

This package contains the official JSON Schemas for every version of Swagger/OpenAPI Specification

And there may be other uses out there we don't know of.

Better have up-to-date YAML and JSON files with the most current identifier in the main branch.

Copy link
Member

Choose a reason for hiding this comment

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

@ralfhandl better to not put schemas where some random 3rd-party tool can think they are the correct ones when they are not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, then let's replace the date with "in-progress" and see what gets published to the NPM package registry 😎

Copy link
Member

Choose a reason for hiding this comment

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

@ralfhandl I wouldn't object to trying to coordinate with them a bit. This is part of my (myriad) frustration with the schemas- we don't exactly make it easy for people who want to do something like package and distribute to do the right thing. It was only a couple of months ago that I even realized that "published" meant "put on the GitHub pages site" since we didn't even link to them on the index page of that site. It's far easier to find the "wrong" files than the "right" ones.

@karenetheridge
Copy link
Member Author

Why don't we just generate it [the JSON file] for publication in the gh-pages branch?

I don't object to that if it wouldn't inconvenience contributors or tool vendors. It's not difficult to convert the yaml file in main (or a topic branch) to json on the fly, but the barrier is not zero.

@handrews
Copy link
Member

@karenetheridge we already have a YAML-to-JSON script that was run every time we published, as changes were (and are) _only_committed directly to the YAML version. So this isn't a new thing. The only baffling thing to me is why we ever checked the JSON in to main. We have, AFAICT, never used it for anything as it was regenerated on each publication. All it does is confuse people into thinking they need to keep it in sync on main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants