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

chore: consider switching JSON-schema dependency to github.com/santhosh-tekuri/jsonschema #5884

Open
thaJeztah opened this issue Mar 4, 2025 · 4 comments
Labels
area/dependencies help wanted kind/refactor PR's that refactor, or clean-up code

Comments

@thaJeztah
Copy link
Member

Description

The github.com/xeipuuv/gojsonschema module used for JSON-schema validation has not been maintained for a long time, and because of that, ended on the "avoid" list in kubernetes; kubernetes/kubernetes#128572 (comment)

The OCI image-spec has now transitioned to a new dependency (github.com/santhosh-tekuri/jsonschema) as replacement in v1.1.1, and we should look if we can follow so that we don't end up with two separate dependencies performing the same task;

@wilkice
Copy link

wilkice commented Mar 10, 2025

Hi, I am trying to implement this. But now has a problem. This new dependency: github.com/santhosh-tekuri/jsonschema, will fail current schema test. Because we are defining a dict type in test file

type dict map[string]any

The new schema validator take dict as an invalid type. Detail see santhosh-tekuri/jsonschema#215.

So I am wondering can we replace all dict in https://github.com/docker/cli/blob/master/cli/compose/schema/schema_test.go to map[string]any. This way we can transfer to new validator dependency.

Also we can wait until https://github.com/santhosh-tekuri/jsonschema fix this, but not sure how long it will takes.

If changing test is ok, I can make PR for this transfer to new dependency.

@thaJeztah
Copy link
Member Author

Thanks for looking at this! In all honesty, I haven't looked closely yet how easy the transition would be (and if the new one provides all functionality we need); I saw the move of other projects to this new module, and (as we tend to have common dependencies between those projects), wanted to explore if we could switch as well.

If you're interested, it may also be worth looking at compose-spec/compose-go#749 first; the compose-spec is a more modern codebase, and if we want to switch, we likely want to make sure the new dependency works for that project (if it does, we can switch docker/cli to match).

cc @ndeloof @glours ☝️ if you agree, that is (I'm not a core maintainer of the compose-spec, so let me know if you think that makes sense)

if changing test is ok, I can make PR for this transfer to new dependency.

I guess this depends a bit; does the new module provide an alternative to maintain existing behavior, and to still have a test covering what's tested? (If so, then I think updating the test should not be a problem).

@ndeloof
Copy link
Contributor

ndeloof commented Mar 10, 2025

👍

@wilkice
Copy link

wilkice commented Mar 10, 2025

Thanks for replying.

I guess this depends a bit; does the new module provide an alternative to maintain existing behavior, and to still have a test covering what's tested? (If so, then I think updating the test should not be a problem).

the new module need to modify it's behavior. In many case, provided json data is ok but validator fail. I am trying a PR to fix this. As the switch is still on discussing, So I think currently the change of test code is not needed.

I saw the move of other projects to this new module, and (as we tend to have common dependencies between those projects), wanted to explore if we could switch as well.

got it. I will do some experiment to see if switch is ok on compose-spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies help wanted kind/refactor PR's that refactor, or clean-up code
Projects
None yet
Development

No branches or pull requests

3 participants