Add dhall-kubernetes support for "schemas"
#84
Conversation
For lack of a better term, I'm calling the `{ Type = …, default = … }`
record that the record completion operator expects a "schema". This
change adds `dhall-kubernetes` support for auto-generating these schemas
for ease of use with the new `::` operator.
See the included example for how this would be used
|
Great stuff Gabriel! |
| } | ||
| , template = kubernetes.PodTemplateSpec::{ | ||
| , metadata = kubernetes.ObjectMeta::{ | ||
| , name = name |
There was a problem hiding this comment.
This is one of the places where I think you can ommit the metadata.name attribute
This is a manifestation of #8 (comment) I think the ground rule is that only toplevel objects need name, any template objects can (and must?) omit it instead.
I think if you explicitly set metadata.name here, really weird things might happen. Every pod will have the same name, e.g. there will never be more than 1 pod created eventhough the daemonset wants to create more than 1.
Also I think you must set template.selector otherwise the pods that the daemonset will create will not associate with the daemonset (See https://kubernetes.io/docs/concepts/workloads/controllers/daemonset/)
It used to be the case that if you left out selector that it would be implicitly created, but I think that behaviour was removed recently. At least for Deployment; maybe not for daemonset.
I'm poking @akshaymankar here just to be sure.
There was a problem hiding this comment.
This is a bug that we should fix. name should not be a required attribute of ObjectMeta. The bug issue is #8
There was a problem hiding this comment.
Yeah, I only added the name field because the type required it
For reference, this is based off of: https://github.com/helm/charts/blob/550096fcda27d7637a7a066240c61a4c6cb61f21/stable/aws-iam-authenticator/templates/daemonset.yaml
There was a problem hiding this comment.
Ah yeh that chart uses an older version of the DaemonSet API. Ours requires the selector: https://github.com/dhall-lang/dhall-kubernetes/blob/master/types/io.k8s.api.apps.v1.DaemonSetSpec.dhall so your example will hopefully give you a nice type error! (Woohoo dhall types!)
There was a problem hiding this comment.
@arianvp: The example type-checks because the default record for DaemonSetSpec defines an empty selector field
There was a problem hiding this comment.
Ahh yes. I remember now. We have a bug open for this as well :) #78
There was a problem hiding this comment.
I think if you explicitly set metadata.name here, really weird things might happen.
I checked with a daemonset, nothing weird happened, the controller just ignored the name field while creating pods. But it might confuse somebody reading a config.
Also I think you must set
template.selector
This is true, if you don't set it, it is a validation error, but only for daemonsets. Not sure if this should be a type error given the OpenAPI spec doesn't say anything about it.
It is only required for top-level objects . (E.g. `Deployment`, `Pod`, `DaemonSet`) but it is Optional when it is part of a compond object (e.g. `PodTemplateSpec` inside a `Deployment` or `DaemonSet`) See #8 (comment) and #84 (comment)
|
Hi :) This PR would be great to use for us but I was waiting for it to be merged to master before starting using it in https://github.com/EarnestResearch/dhall-packages . Do you think this is the final state this will be done in master? If yes, I could risk starting using this branch as the syntax is much better with schemas :) |
|
At this point I think we should just merge this and if there are any additional changes they should be made as additional pull requests because I don't see a concrete action item from reviewers. If I don't hear otherwise I'm going to merge tomorrow |
For lack of a better term, I'm calling the
{ Type = …, default = … }record that the record completion operator expects a "schema". This
change adds
dhall-kubernetessupport for auto-generating these schemasfor ease of use with the new
::operator.See the included example for how this would be used