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

Add test for refs caching #30

Closed
wants to merge 1 commit into from

Conversation

Nitive
Copy link

@Nitive Nitive commented Jan 9, 2021

The problem with refs caching lays in gojsonschema code. There is a GET request which can't be cached on kubeconform side.

I think the best solution for caching refs would be to have http transport with supports caching and pass it to gojsonschema

  • Most code for caching can be deleted in favour of external module gregjones/httpcache
  • Easy to implement other caching method (just use one of the cache backend included in httpcache)
  • Standard interface means caching inside of gojsonschema code can be done as simple and passing http transport as argument (when and if gojsonschema supports it)

@test "Cache not only original schema but also refs to other schemas" {
run bin/kubeconform -schema-location './fixtures/registry/{{ .ResourceKind }}{{ .KindSuffix }}-with-ref.json' fixtures/test_crd.yaml
[ "$status" -eq 0 ]
[ "`ls cache/ | wc -l`" -eq 1 ]
Copy link
Author

Choose a reason for hiding this comment

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

Actual value is 0 because no refs got cached

Copy link
Author

@Nitive Nitive Jan 9, 2021

Choose a reason for hiding this comment

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

I think the better test would be

  1. Run kubeconform with internet access
  2. Disable network
  3. Run kubeconform again and check that status hasn't changed

But then we need to block network somehow. Maybe with DNS hack or somehow better

"metadata": {
"$ref": "https://kubernetesjsonschema.dev/v1.14.0/_definitions.json#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta",
"description": "Standard object metadata."
},
Copy link
Author

Choose a reason for hiding this comment

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

Almost the same file as trainingjob-sagemaker-v1.json with such change:

    "metadata": {
-     "type": "object",
+     "$ref": "https://kubernetesjsonschema.dev/v1.14.0/_definitions.json#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta",
+     "description": "Standard object metadata."
    },

@Nitive Nitive mentioned this pull request Jan 9, 2021
@yannh
Copy link
Owner

yannh commented Jan 9, 2021

yep that ll need patching gojsonschema.. might have a look, some day :)

@yannh
Copy link
Owner

yannh commented Feb 28, 2021

BTW thanks a lot for filing that bug xeipuuv/gojsonschema#322 👍

@yannh
Copy link
Owner

yannh commented Oct 16, 2022

Closing, it looks like gojsonschema is not very actively maintained... and this will be fairly difficult to complete :(

@yannh yannh closed this Oct 16, 2022
@grosser
Copy link

grosser commented Mar 30, 2023

not sure what the point of a cache is if it can't be used offline
I assume it saves some amount of requests 🤷

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

Successfully merging this pull request may close these issues.

None yet

3 participants