Add support for explicit splitting of definitions #2124
Conversation
| This is considered a guess as it does not work with all types. Currently it uses the first word from the description | ||
| appended to the largest prefix before the last @.@ of the parent. | ||
| -} | ||
| guessModelNameForSplit :: ModelHierarchy -> Definition -> Maybe ModelName |
There was a problem hiding this comment.
I'm a bit concerned about guessing the model name from the first word of the description, since I'm still new to this domain and I don't know how much we can rely on users following this convention. The main thing I'm trying to avoid is having to help users with debugging their CRD if things go wrong. The other reason I bring this up is because it seems like we don't need to worry about the splits being particularly ergonomic since they only need to be specified once.
One intermediate alternative is perhaps to allow the user to omit the prefix from the model name, which would default to using the parent's prefix. In other words, the user could specify:
com.example.v1.Certificate~spec=CertificateSpec
There was a problem hiding this comment.
Here is my sample of CRDs that led me to think this was a good idea:
| CRD | Follows Convention |
|---|---|
| kubevirt | ✓ |
| cert-manager | ✗ |
| calico | ✓ |
| contour | ✓ |
| rook | ✗ |
| in house CRDs | ✓ |
I would have to think on making the parents prefix optional. The one use case I wanted to support in the future is:
(com.example.ResourceSpec).podSelector=io.k8s.apimachinery.pkg.apis.meta.v1.LabelSelector
(Right now the only thing preventing that is LabelSelector has imports that a naiive == on Dhall.Expr cant see through.)
There was a problem hiding this comment.
Alright, then we can keep it the way it is
muff1nman
force-pushed
the
ref-override
branch
2 times, most recently
from
7b44d67
to
26daf39
Compare
| This is considered a guess as it does not work with all types. Currently it uses the first word from the description | ||
| appended to the largest prefix before the last @.@ of the parent. | ||
| -} | ||
| guessModelNameForSplit :: ModelHierarchy -> Definition -> Maybe ModelName |
There was a problem hiding this comment.
Alright, then we can keep it the way it is
|
Feel free to merge after resolving the merge conflicts |
The motivation behind this is better user experience when dealing with Kubernetes CRDs. CRDs in an openapi definition are expressed as one massive definition rather than the Kubernetes builtin types which rarely nest and instead make use of references within openapi. These references are desirable as it makes it more manageable to sparsely define resources utilizing the nested defaults as you go down the structure. Unfortunately it looks like support for references wrt CRDs is unlikely to be supported in the near future: kubernetes/kubernetes#62872 As a workaround, this adds a new option `--splitPaths` that takes a path and an optional new model name. The path roughly emulates the idea behind `kubectl explain` but () must be used around some model names to disambiguate with a `.` character. During type conversion any matches of path will cause a Dhall reference to be injected with the nested definition being pushed back on the stack of definitions to convert. The top level model name can either be specified on the command line via `=com.some.ModelName` or be guessed in the case the CRD author follows the best practice of using the field name as the first word in the description. Some future work that might need exploring related to this are: 1. Allow specifying `splitPaths` from a file 2. Clean up which types get accumulated into the `typesUnion`. I noticed while doing this that all? top level definitions are being dumped into this union where really it should only be top level definitions that have an `apiVersion` and `kind` 3. See if the `mergeNoConflicts` function needs to be improved such that it takes semantic equality (traversing imports) into account.
muff1nman
force-pushed
the
ref-override
branch
from
26daf39
to
8bb2b82
Compare
The motivation behind this is better user experience when dealing
with Kubernetes CRDs. CRDs in an openapi definition are expressed
as one massive definition rather than the Kubernetes builtin types
which rarely nest and instead make use of references within openapi.
These references are desirable as it makes it more manageable to
sparsely define resources utilizing the nested defaults as you go down
the structure.
Unfortunately it looks like support for references wrt CRDs is unlikely
to be supported in the near future:
kubernetes/kubernetes#62872
As a workaround, this adds a new option
--splitPathsthat takes a pathand an optional new model name. The path roughly emulates the idea
behind
kubectl explainbut I ended up using the~character insteadof
.such that the full parent model name could still be used withoutambiguities. During type conversion any matches of path will cause a
Dhall reference to be injected with the nested definition being pushed
back on the stack of definitions to convert. The top level model name
can either be specified on the command line via
=com.some.ModelNameorbe guessed in the case the CRD author follows the best practice of using
the field name as the first word in the description.
Some future work that might need exploring related to this are:
splitPathsfrom a filetypesUnion. I noticedwhile doing this that all? top level definitions are being dumped
into this union where really it should only be top level definitions
that have an
apiVersionandkindmergeNoConflictsfunction needs to be improved such thatit takes semantic equality (traversing imports) into account.