fix: fromCRD handles structural schema rule 1 exceptions#49
Conversation
1f5cf8a to
9f3df48
Compare
arnarg
left a comment
There was a problem hiding this comment.
Thanks for your pull request!
To not overcomplicate the if statements in jsonschema.nix we've been handling these kind of edge cases in crd2jsonschema.py instead. Could you move this logic there?
See:
nixidy/pkgs/generators/crd/crd2jsonschema.py
Line 134 in 95d463a
9f3df48 to
dffefef
Compare
|
done and added an unit test. TEST$nix run .#crd2jsonschemaTest
......
----------------------------------------------------------------------
Ran 6 tests in 0.003s
OK |
dffefef to
b72cec8
Compare
arnarg
left a comment
There was a problem hiding this comment.
Out of curiousity, are you using an LLM to write this?
Please don't creep in the type annotations and while unit tests are useful I want to first make this function more testable in the future.
To fix the issue at hand just edit the if statement I linked to like so:
elif "type" not in definition:
if definition.get("x-kubernetes-preserve-unknown-fields", False) == True:
# ...
elif definition.get("x-kubernetes-int-or-string", False) == True:
# ... fixes arnarg#48. There are a few exceptions that allow a node to not have "type" field [^1]. This PR addresses these exceptions in `fromCRD` function. ### TEST - Test CRD manifest ```yaml apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: name: bars.foo.baz spec: scope: Namespaced group: foo.baz names: kind: Bar plural: bars shortNames: - bar singular: bar versions: - name: v1beta3 schema: openAPIV3Schema: properties: spec: properties: node: description: a test node x-kubernetes-int-or-string: true required: - node type: object required: - spec type: object served: true storage: true ``` #### Failure replication ```bash $nix build --show-trace .#gen.bar # ... error: attribute 'type' missing at /nix/store/67bvmrzr4iin6fcvnz9qavr2fya9hnwm-source/pkgs/generators/crd/jsonschema.nix:154:29: 153| # if property has an array type 154| else if property.type == "array" | ^ 155| then # ... ``` - flake.lock ``` "nixidy": { "inputs": { "flake-utils": "flake-utils", "nix-kube-generators": "nix-kube-generators", "nixpkgs": "nixpkgs_2" }, "locked": { "lastModified": 1753281262, "narHash": "sha256-XZO8SNbjifLzUenfTClpzwBwiOi6OkViZ1DrECqUMHM=", "owner": "arnarg", "repo": "nixidy", "rev": "e7e7685825c6bd62a300bbe3eac09d7352629e92", "type": "github" }, "original": { "owner": "arnarg", "repo": "nixidy", "type": "github" } }, ... #### Run against this change ```bash $nix build --show-trace .#gen.bar ``` [^1]: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#specifying-a-structural-schema
b72cec8 to
6d321a2
Compare
|
I'm not using an LLM. I added types and a bit of refactoring according to my linter because there isn't a python linter setup in this repo afaik. I removed tests for now, but it is hard to contirubte relatively bigger changes (e.g. #51). Let me know how you'd like to handle this in the future. |
|
I didn't mean any offense with that question btw. Yeah this file started as a simple script to transform a CRD into something the existing code generator already understands. Since then handling for more and more edge cases has been added so it should probably be cleaned up a bit and tests added. |
arnarg
left a comment
There was a problem hiding this comment.
Looks good! Thank you for this contribution!
Fixes #48
TEST
Failure replication
Run against this change