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

Support yaml-to-dhall #83

Closed
akshaymankar opened this issue Oct 7, 2019 · 7 comments
Closed

Support yaml-to-dhall #83

akshaymankar opened this issue Oct 7, 2019 · 7 comments

Comments

@akshaymankar
Copy link

I tried running yaml-to-dhall on a configmap like this with dhall-kubernetes/types/io.k8s.api.core.v1.ConfigMap.dhall as SCHEMA:

apiVersion: v1
kind: ConfigMap
metadata:
  name: "eirini"
data:
  key: value

But it failed as it didn't match the schema. The type looks like this:

{ apiVersion :
    Text
, binaryData :
    List { mapKey : Text, mapValue : Text }
, data :
    List { mapKey : Text, mapValue : Text }
, kind :
    Text
, metadata :
    ./io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta.dhall
}

My understanding is that data and binaryData both are optional fields in K8s spec, but that is not true for the dhall type. Why is this so?

For context, I want to use yaml-to-dhall as a begining point to translate my helm chart into a sort of "dhall distribution".

@arianvp
Copy link
Member

arianvp commented Oct 8, 2019

Hmm this indeed seems to be a bug. The upstream swagger spec marks neither of the fields as required.

But I have a feeling this has to do with the fact that we treat the empty list [] as "not there" when running dhall-to-yaml --omit-empty.

Perhaps we should encode optional lists as Optional (List a) instead? that then makes dhall-to-yaml and yaml-to-dhall inverses of eachother, whilst the current behaviour is "lossy"

Only annoyance is that when defining a list you always have to do Some [1,2,3] instead of just [1,2,3]. And considering almost all lists are "optional" in kubernetes, this might add some noise to definitions.

Thoughts @f-f ?

@f-f
Copy link
Member

f-f commented Oct 8, 2019

@arianvp yeah what you described is exactly the reason for the current behaviour: we used to have Optional (List a) but changed it to List a, because it was a lot of noise and we could get rid of it with the --omit-empty

I'm not super keen on going back to the old behaviour, so how about adding a flag to yaml-to-dhall so it would "tolerate" this kind of schema mismatches for List?

@akshaymankar
Copy link
Author

I think emptiness toleration in yaml-to-dhall would be a decent feature and will definitely help in migrating from helm.
So, should I create an issue on the dhall-haskell repository?

And hopefully, there are no cases in K8s api where an empty list means something different from null :)

@akshaymankar
Copy link
Author

I modified yaml-to-dhall to support missing lists in dhall-lang/dhall-haskell#1414 and I am happy to report that the configmap I presented can be translated to dhall 🎉

But I bumped into couple of other problems when trying to read a deployment:

  • the name field in ObjectMeta, as mentioned in Figure out really required keys? #8 (comment), is indeed supposed to be optional. Not just in the case specified there, but also in spec.template.metadata of Deployment where the name for pod is to be generated by the deployment. Note that in this case even GenerateName is not required.
  • The PodSpec schema says nodeAffinity is required, but it is really not required. This code in Convert.hs, seems to be offending that. I am not sure if yaml-to-dhall should detect if a key is "transitively emptiable".

Here is the smallest example which is valid deployment but yaml-to-dhall cannot make it into a Dhall expression:

---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: something
spec:
  replicas: 1
  selector:
    matchLabels:
      name: something
  template:
    metadata:
      labels:
        name: something
    spec:
      containers:
      - name: nginx
        image: nginx

@arianvp
Copy link
Member

arianvp commented Aug 12, 2020

This example now works too. I think only the nodeAffinity issue is left, but I'm not sure if I understand @akshaymankar . Can you craft a minimal example for that too??

@akshaymankar
Copy link
Author

Looks that that case is also handled now. I see that affinity in PodSpec is an Optional now, so maybe that is also not an issue. I will probably not get much time to test the rest of the APIs anytime soon, so feel free to close this issue.

@Gabriella439
Copy link
Contributor

Alright, I will close the issue, then

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

No branches or pull requests

4 participants