Skip to content

Conversation

ralfhandl
Copy link
Contributor

@ralfhandl ralfhandl commented Oct 15, 2024

The schema-test workflow was using the JSON metaschemas, whereas the README.md files claim that the YAML files are the source of truth.

  • Use YAML metaschemas in schema tests
  • Use YAML metaschemas in validate.mjs
  • Remove out-of-sync JSON files
  • Update README.md files

@ralfhandl ralfhandl requested review from a team as code owners October 15, 2024 15:25
@ralfhandl ralfhandl changed the title Convert YAML metaschemas to JSON, test the JSON metaschemas, then check in changes Convert YAML metaschemas to JSON Oct 15, 2024
mikekistler
mikekistler previously approved these changes Oct 15, 2024
Copy link
Contributor

@mikekistler mikekistler 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. 👍

One minor comment.

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'd still prefer to not have the JSON on main at all. I think the problem of "these schemas look published but they are not" is the bigger problem here, and I don't think we should invest in the current arrangement.

Per #3677, I think we should focus on setting up schema development correctly on the new dev branch (and release-line-specific branches off of it), which is where all work-in-progress will live.

If there's an argument that this is a necessary step, I'm open to it, but this feels like going backwards to me by trying to bless main as an alternate publication location.

@handrews handrews added Schema changes related to the schema(s) script Pull requests that update Bash or JavaScript code labels Oct 15, 2024
@ralfhandl ralfhandl marked this pull request as draft October 15, 2024 19:49
@ralfhandl
Copy link
Contributor Author

ralfhandl commented Oct 16, 2024

I'd still prefer to not have the JSON on main at all

The JSON is used by the schema tests on main. We can

  1. Generate the JSON on the fly the YAML before running the tests, or
  2. Change the tests to read the YAML instead

In addition we can remove the JSON from main.

I think we should focus on setting up schema development correctly on the new dev branch

Fine with me, but the schema tests being based on the JSON contradicts the README's claim that the YAML are is leading, and fixing that now makes it easier to move the sources & scripts somewhere else.

@ralfhandl ralfhandl changed the title Convert YAML metaschemas to JSON Run tests with YAML metaschemas Oct 16, 2024
@ralfhandl ralfhandl marked this pull request as ready for review October 16, 2024 08:32
@ralfhandl ralfhandl requested review from a team, handrews and mikekistler October 16, 2024 08:32
@handrews handrews merged commit e9dc186 into OAI:main Oct 18, 2024
2 checks passed
@ralfhandl ralfhandl deleted the schemas-yaml-to-json-test-check-in branch October 21, 2024 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Housekeeping Schema changes related to the schema(s) script Pull requests that update Bash or JavaScript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants