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

API v2 #57

Merged
merged 31 commits into from
May 1, 2019
Merged

API v2 #57

merged 31 commits into from
May 1, 2019

Conversation

f-f
Copy link
Member

@f-f f-f commented Mar 30, 2019

I'm sorry if this is a huge diff, but we're basically swapping the contents of the repo 😄

This PR ports the conversion script from Python to Haskell (fix #7).
The main reasons for this are that:

  • the Python script was really hard to maintain for reasons like "converting from Swagger to Dhall is interleaved with string formatting"
  • in Haskell we can use the dhall library to generate always syntactically correct Dhall AST. It's also much easier to keep an eye on correctness, because types and pattern matching. It also forces us to deal with things like cyclic imports from the get go

Things happening here:

  • move defaults from default to defaults folder, as it is in dhall-nethack
  • add types.dhall, defaults.dhall, so that one can now easily "pin a version" by just importing these two records at a specific commit/tag. They also make it really easy to access objects, e.g. let types = https://raw.githubusercontent... sha256:... in types.Deployment
  • also add typesUnion.dhall (fix Generate top-level union type for all apis #54), so one is able to send to Kubernetes different objects in the same stream
  • defaults are resolved recursively (fix [RFC]: Resolve defaults recursively #46): if there's an import of a "nullable" record, then it's not marked as Optional, making merging objects much easier
  • switch to the new Optional syntax (fix Transition to new Optional syntax #49)
  • default objects are not lambdas anymore, and instead they just don't include the required fields (that is, the ones that are not nullable records), as suggested in Less verbose records with optional values dhall-lang#382 (comment)
  • for objects that are simple types we used to generate a simple lambda \(a : Text) -> a as a default, now we just don't generate a default (e.g. see io.k8s.apimachinery.pkg.apis.meta.v1.Time)
  • autoformat all generated Dhall code

Things left to do before we can merge:

  • Thoroughly comment the code
  • Split code in different modules, as this is a huge module that mixes "business logic" with "json conversion"
  • Remove package.yaml and add version bounds
  • Remove HTTP fetching code, and take arguments with the location of the file instead
  • Think about how we are going to build this
  • Remove the convert.py script
  • Remove the api folder
  • Port the examples to the new API
  • Adjust the readme to remove the distinction between the "raw API" and the "nice API", now it's all "new API"
  • Add to the readme a FAQ section, as we need to document the pattern for Generate top-level union type for all apis #54
  • Reintroduce the JSONSchemaProps object, fixing the cyclic imports (fix JSONSchemaProps.dhall references itself (Cyclic import) #47)
  • Release the current version as 1.0

@f-f f-f requested a review from arianvp March 30, 2019 10:41
@f-f
Copy link
Member Author

f-f commented Mar 30, 2019

@arianvp I'll now take care of the first two items in the TODO list (adding comments and splitting code in modules), but otherwise all the items are up for grabs and any help would be amazing 😄

About the build, we have two possibilities here:

  1. we build the Haskell project here, run it, and see if it generates the same files as the ones that are versioned. If not we error out (a bit like we are doing for the readme and the examples)
  2. or we move the Haskell project into the megarepo (as proposed by Gabriel in Port to Haskell? #7 (comment)), and build it there. I'd like to keep the generated files here, so this would mean separating the "lifetimes" of the generator script and the generated files

Option 2 is tempting so we keep up with all the changes in dhall-haskell, but on the other hand it would be really useful to build and generate on CI runs (to avoid breakage), so I think I'm leaning towards option 1

Copy link
Member

@arianvp arianvp left a comment

Choose a reason for hiding this comment

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

Just some remarks. not much so far.

dhall-kubernetes.cabal Outdated Show resolved Hide resolved
package.yaml Outdated Show resolved Hide resolved
stack.yaml Outdated Show resolved Hide resolved
src/Main.hs Outdated Show resolved Hide resolved
src/Main.hs Outdated Show resolved Hide resolved
@arianvp
Copy link
Member

arianvp commented Mar 30, 2019

Question @f-f and @ari-becker

What format do we want defaults.dhall and types.dhall to have?
The current approach seems 'wrong' to me (https://github.com/dhall-lang/dhall-kubernetes/blob/e0a654ad3836cdaa1f063589734b342f2031a57f/types.dhall)
as k8s can have same object names in different namespaces, so we should definitely encode
the namespace in types.dhall and defaults.dhall. (i.e. there is v1beta1/Deployment and v1/Deployment) which are actually different

Something that resembles https://github.com/coralogix/dhall-prometheus-operator/blob/master/ImportTypes.dhall ? It does seem like a nice paradigm.

I was thinking, perhaps we can do this nested ala:

.
├── types
│   ├── io
│   │   ├── k8s
│   │   │   ├── api
│   │   │   │   ├── apps
│   │   │   │   │   ├── v1
│   │   │   │   │   │   ├── Deployment.dhall
│   │   │   │   │   │   └── DeploymentSpec.dhall
│   │   │   │   │   └── v1.dhall
│   │   │   │   ├── apps.dhall
│   │   │   │   ├── rbac
│   │   │   │   ├── rbac.dhall
│   │   │   │   ├── storage
│   │   │   │   └── storage.dhall
│   │   │   └── api.dhall
│   │   └── k8s.dhall
│   └── io.dhall
└── types.dhall
# types.dhall
{ io = ./io.dhall }
# k8s.dhall
{ api = ./api.dhall }
# api.dhall
{ apps = ./apps.dhall
, rbac = ./rbac.dhall
, storage = ./storage.dhall
...
 }
# apps.dhall
{ Deployment = ./Deployment.dhall
, DeploymentSpec = ./DeploymentSpec.dhall
}

You can then do something like:

let api = ./types.dhall
in  {... blah ... } : api.k8s.apps.v1.Deployment

Similar to how that's currently done in dhall-prometheus-operator

I'm not sure if the nested directory structure is neccssery per se. Pro is that it allows people to just use
a small part of the API. We could also just opt for one big top-level types.dhall
that looks like:

io = { k8s = { apps = { v1 = {
   Deployment = ./io.k8s.apps.v1.Deployment.dhall
  , DeploymentSpec = ./io.k8s.apps.v1.DeploymentSpecc.dhall } } } } }

Not sure what is preferable. Thoughts?

Note that this problem is also present in typesUnion.dhall. Perhaps we should have one union per API namespace?

src/Main.hs Outdated Show resolved Hide resolved
@f-f
Copy link
Member Author

f-f commented Mar 30, 2019

@arianvp the approach I took here comes from the fact that I consider the types.dhall and defaults.dhall records part of "UX".
That is, they are not strictly necessary (after all we managed without them just fine for a long time), but they help when doing things by reducing clutter: you could import all the objects by URL, but instead you import only one thing.
So introducing the full namespace would go in the other direction, as every time you have to reference an object you'd have to type the whole namespace

However, one might need objects that are not in these records (because they are on a different API version), so I propose we keep the current approach but also introduce a typesNamespaced.dhall and defaultsNamespaced.dhall (and a typesUnionNamespaced.dhall)

Some other notes:

  • I wouldn't like a nested directory structure, as it's very hard to quickly navigate and it would be inconsistent anyways (see next point)
  • splitting objects by namespace doesn't quite work, and the reason is that the objects naming is very inconsistent. For example, let's take the two objects io.k8s.api.extensions.v1beta1.DeploymentCondition and io.k8s.api.apps.v1beta1.DeploymentCondition. They are the same object, on the same API version, but in two different namespaces! Which one is the right one? Turns out the correct one is not the io.k8s.api.extensions, because that was the naming in development, and the "right" object (i.e. the one you'll find referenced by other objects) is the one in the io.k8s.api.apps namespace.
    There are tens of such weird cases - I spent several hours yesterday going through all the objects before coming up with the current approach

@arianvp
Copy link
Member

arianvp commented Mar 30, 2019

I don't fully understand the second point. But I'm fine with not nesting the directories so let's do as you suggest

@f-f
Copy link
Member Author

f-f commented Mar 30, 2019

@arianvp sorry I think I misunderstood your previous comment.
Updated take: let's keep the types.dhall as is, and have the nested records as you propose in typesFull.dhall (or typesNamespaced.dhall).
Performance might be bad compared to a flat record though. Also I'd like not to include this change in this PR, as this is already huge and it can be added later

@ari-becker
Copy link
Collaborator

@arianvp the approach I took here comes from the fact that I consider the types.dhall and defaults.dhall records part of "UX".
That is, they are not strictly necessary (after all we managed without them just fine for a long time), but they help when doing things by reducing clutter: you could import all the objects by URL, but instead you import only one thing.
So introducing the full namespace would go in the other direction, as every time you have to reference an object you'd have to type the whole namespace

Agree with this reasoning. Typing out the whole namespace for each record and type gets old fast. Would probably end in import style let-expressions (e.g. let Deployment = (./path/to/Imports.dhall).Kubernetes.io.k8s.api.apps.v1.Deployment) which feels wrong. In dhall-prometheus-operator I settled on that paradigm because, like you said @arianvp it's a nice paradigm, and it most closely matches the Kubernetes YAML objects (i.e. apiVersion: apps/v1 kind: Deployment translates to Kubernetes.apps.v1.Deployment) but it's also hand-coded so maybe it doesn't fit here, where you're trying to build this through a script and as such it's going to pick up everything in the OpenAPI document.

* splitting objects by namespace doesn't quite work, and the reason is that the objects naming is very inconsistent. For example, let's take the two objects `io.k8s.api.extensions.v1beta1.DeploymentCondition` and `io.k8s.api.apps.v1beta1.DeploymentCondition`. They are the same object, on the same API version, but in two different namespaces! Which one is the right one? Turns out the correct one is not the `io.k8s.api.extensions`, because that was the naming in development, and the "right" object (i.e. the one you'll find referenced by other objects) is the one in the `io.k8s.api.apps` namespace.

Question : because the users of this library are presumably Kubernetes end-users, does the extensions.v1beta1 namespace have to be included? Certainly it would reduce clutter (and probably improve performance, since the records would be smaller) to only provide the "real" types and records?

@arianvp
Copy link
Member

arianvp commented Mar 31, 2019

Think about how we are going to build this

Lets keep everything in this Repo. Dhall langauge is slowly starting to stabilize, so it doesn't matter if we
still use an old dhall version for a while. Allows us to choose ourselves when we migrate to a new version of dhall-haskell and keeps stuff simple

@f-f
Copy link
Member Author

f-f commented Apr 1, 2019

With this latest changes I'm pretty sure now #46 is properly implemented and working.

This is how a simple deployment looks like with the new schemas:

let types =
      ../types.dhall sha256:1ac84a230c0fdd003b4db05157b11329d273217cc6837083eb30c9288a67c87f

let defaults =
      ../defaults.dhall sha256:2e3d2b5d5fb52d08b078e803fa88ac1ed0f788c6a4d6342621a76eb77afe40f4

let deployment
    : types.Deployment
    =   defaults.Deployment
       { metadata =
            defaults.ObjectMeta  { name = "nginx" }
        , spec =
            Some
            (   defaults.DeploymentSpec
               { replicas =
                    Some 2
                , template =
                      defaults.PodTemplateSpec
                     { metadata =
                          defaults.ObjectMeta  { name = "nginx" }
                      , spec =
                          Some
                          (   defaults.PodSpec
                             { containers =
                                  [   defaults.Container
                                     { name =
                                          "nginx"
                                      , image =
                                          Some "nginx:1.15.3"
                                      , ports =
                                          [   defaults.ContainerPort
                                             { containerPort = 80 }
                                          ]
                                      }
                                  ]
                              }
                          )
                      }
                }
            )
        }

in  deployment

And the new omitEmpty flag is so nice:

$ dhall-to-yaml --omitEmpty <<< "./examples/deploymentSimple.dhall"
apiVersion: apps/v1
kind: Deployment
spec:
  template:
    spec:
      containers:
      - image: nginx:1.15.3
        name: nginx
        ports:
        - containerPort: 80
    metadata:
      name: nginx
  replicas: 2
metadata:
  name: nginx

@f-f
Copy link
Member Author

f-f commented Apr 1, 2019

@arianvp any idea why Hydra shows a passing CI? The CI should fail for this branch, as all the files in default disappeared

@arianvp
Copy link
Member

arianvp commented Apr 1, 2019

The CI generates the default folder as part of its build step. The result of the dhall-kubernetes derivation is a default and types folder by calling convert.py. The derivation actually ignores the default and types folder in the repo to ensure purity and always generates them from scratch

generate.sh builds the derivation and copies over the generated files to the root of the repo for convenience, if the derivation succeeds to build

So before the tests are run, the default folder is repopulated hence the tests succeed :p. See the Makefile

We should modify the Makefile to not call convert.py but instead call our Haskell script.

Since we can't build our haskell script with Nix yet, we could hack out the types generation for now, so you can manually test stuff

In the makefile https://github.com/dhall-lang/dhall-kubernetes/blob/master/Makefile

change the build rule to:

build:
    echo lol nothing

and in the derivation https://github.com/dhall-lang/dhall-kubernetes/blob/master/nix/dhall-kubernetes.nix
make it so that the derivation doesn't ignore the ../types and ../default folder.

that should cause those folders even if generated by hand to end up in the nix derivation and be available for the check-sources.sh script in the check phase of the Makefile until
we fix packaging of the haskell file

I'm not sure what it is; it for sure isn't commited
Tests currently fail; as expected
@arianvp
Copy link
Member

arianvp commented Apr 1, 2019

@f-f The build uses haskell now! 🎊 The tests now fail; as expected

@@ -18,7 +18,6 @@ executable dhall-kubernetes
Dhall.Kubernetes.Convert
Dhall.Kubernetes.Data
Dhall.Kubernetes.Types
Paths_dhall_kubernetes
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

@f-f f-f Apr 1, 2019

Choose a reason for hiding this comment

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

TL;DR: you want it to refer to static data without using Template Haskell (it includes other stuff too)

But you're right that we're not using it here 😄

@f-f
Copy link
Member Author

f-f commented Apr 2, 2019

@arianvp @ari-becker I rewrote the readme with the new examples, and documented the pattern for #54, could you proofread before we merge? 😊

@arianvp CI is failing because hash mismatches; I generated all the hashes locally with dhall 1.21.0, and we should be using that one in the nix build, but it looks like we're not? 🤔

@f-f
Copy link
Member Author

f-f commented Apr 2, 2019

First release done 🎉, we are ready to merge this as soon as we get CI green 😄

@arianvp
Copy link
Member

arianvp commented Apr 2, 2019

@f-f did you manually freeze the files?

We should adjust Makefile such that it freezes the dhall files...
because types and defaults should be generated by ./scripts/generate.sh and that currently produces a set of unfreezed files :P

remember that the tests and the build run in a sandbox that exclude the types and defaults folder on purpose... Such that ./scripts/generate.sh always puts out a 'clean' build that doesn't succeed 'accidentally'

@f-f
Copy link
Member Author

f-f commented Apr 2, 2019

@arianvp the types and defaults I did freeze by hand in the last commit (and I'll push a fix to the haskell implementation so that it freezes the files when generating them), but the failures come from the examples - which I also froze manually, but they are supposed to be human curated so we have to.

@arianvp
Copy link
Member

arianvp commented Apr 2, 2019

Looking at the diff on the examples:

apiVersion: apps/v1
kind: Deployment
spec:
  revisionHistoryLimit: 10
  selector:
    matchExpressions: []
    matchLabels:
      app: nginx
  strategy:
    rollingUpdate:
      maxSurge: 5
      maxUnavailable: 0
    type: RollingUpdate
  template:
    spec:
      initContainers: []
      tolerations: []
      containers:
      - image: nginx:1.15.3
        command: []
        args: []
        imagePullPolicy: Always
        env: []
        volumeMounts: []
        resources:
          limits:
            cpu: 500m
          requests:
            cpu: 10m
        name: nginx
        ports:
        - containerPort: 80
        envFrom: []
        volumeDevices: []
      readinessGates: []
      imagePullSecrets: []
      hostAliases: []
      volumes: []
    metadata:
      finalizers: []
      ownerReferences: []
      name: nginx
      labels:
        app: nginx
  replicas: 2
metadata:
  finalizers: []
  ownerReferences: []
  name: nginx

We forgot that not only we should omit None's but also empty lists... is that something we can fix in dhall-to-yaml ?

@f-f
Copy link
Member Author

f-f commented Apr 2, 2019

@arianvp I did fix it some days ago 🙂 dhall-lang/dhall-haskell#872

But now that I think of it CI will try to compare the committed yamls (which I generated with the patch above) with the newly generated ones - which might include empty lists - so it will likely fail. Should we pin dhall-haskell to the commit with the patch or should we wait for the next release?

@arianvp
Copy link
Member

arianvp commented Apr 2, 2019

~~Let's just pin to that commit for now. ~~

Edit: perhaps we should wait til the next dhall release. because people will probably install dhall from hackage and not from that pinned commit, and might be surprised that the output is different :P

@f-f f-f changed the title Haskell implementation API v2 Apr 8, 2019
Copy link
Collaborator

@ari-becker ari-becker left a comment

Choose a reason for hiding this comment

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

@arianvp @ari-becker I rewrote the readme with the new examples, and documented the pattern for #54, could you proofread before we merge? 😊

Jeez, sorry I missed this somehow... LGTM though.

If part of the documentation is to try and show cool things that can be done with dhall-kubernetes, I'd just bring up a pattern I've found: sorry it's a little messy and hard to read:

  λ(deployment : KubernetesTypes.apps.v1.Deployment)
     Kubernetes.core.v1.Service
      { metadata = deployment.metadata }
     { spec =
          Some
          (     Kubernetes.core.v1.ServiceSpec
               { ports =
                    Some
                    ( Prelude.`Optional`.fold
                      KubernetesTypes.apps.v1.DeploymentSpec
                      deployment.spec
                      (List KubernetesTypes.core.v1.ServicePort)
                      (   λ ( spec
                            : KubernetesTypes.apps.v1.DeploymentSpec
                            )
                         Prelude.`Optional`.fold
                          KubernetesTypes.core.v1.PodSpec
                          spec.template.spec
                          (List KubernetesTypes.core.v1.ServicePort)
                          (   λ ( spec
                                : KubernetesTypes.core.v1.PodSpec
                                )
                             Prelude.`List`.concatMap
                              KubernetesTypes.core.v1.Container
                              KubernetesTypes.core.v1.ServicePort
                              (   λ ( container
                                    : KubernetesTypes.core.v1.Container
                                    )
                                 Prelude.`Optional`.fold
                                  (List KubernetesTypes.core.v1.ContainerPort)
                                  container.ports
                                  (List KubernetesTypes.core.v1.ServicePort)
                                  (   λ ( portList
                                        : List
                                          KubernetesTypes.core.v1.ContainerPort
                                        )
                                     Prelude.`List`.map
                                      KubernetesTypes.core.v1.ContainerPort
                                      KubernetesTypes.core.v1.ServicePort
                                      (   λ ( containerPort
                                            : KubernetesTypes.core.v1.ContainerPort
                                            )
                                             Kubernetes.core.v1.ServicePort
                                              { port =
                                                  containerPort.containerPort
                                              }
                                             { name =
                                                  containerPort.name
                                              , protocol =
                                                  containerPort.protocol
                                              , targetPort =
                                                  let IntOrString =
                                                        < Int :
                                                            Natural
                                                        | String :
                                                            Text
                                                        >

                                                  in        if Prelude.`Optional`.null
                                                               Text
                                                               containerPort.name

                                                      then  Some
                                                            ( IntOrString.Int
                                                              containerPort.containerPort
                                                            )

                                                      else  Prelude.`Optional`.map
                                                            Text
                                                            IntOrString
                                                            (   λ ( name
                                                                  : Text
                                                                  )
                                                               IntOrString.String
                                                                name
                                                            )
                                                            containerPort.name
                                              }
                                          : KubernetesTypes.core.v1.ServicePort
                                      )
                                      portList
                                  )
                                  ( [] : List
                                         KubernetesTypes.core.v1.ServicePort
                                  )
                              )
                              spec.containers
                          )
                          ([] : List KubernetesTypes.core.v1.ServicePort)
                      )
                      ([] : List KubernetesTypes.core.v1.ServicePort)
                    )
                , selector =
                    deployment.metadata.labels
                , type =
                    Some "ClusterIP"
                }
            : KubernetesTypes.core.v1.ServiceSpec
          )
      }
  : KubernetesTypes.core.v1.Service

And what that does is basically generate a Service from a Deployment, mapping each of the ports of the Deployment to be exposed through a Service. If ports or added or changed in the Deployment, then those changes will be reflected in the Service.

It'd have to be updated for API v2 of course, and maybe it should be in some kind of function library for dhall-kubernetes, but yeah, I thought it was pretty cool when I finally got it to work.

@arianvp
Copy link
Member

arianvp commented May 1, 2019

LGTM!

@f-f
Copy link
Member Author

f-f commented May 1, 2019

@ari-becker looks great! I'd say more docs is better, so let's include useful patterns too. @arianvp was planning a documentation site after we merge this in, so this would be definitely welcome there

(small sidenote: we now provide the IntOrString type too in the types.dhall record, so no need to redeclare it inline)

@f-f f-f merged commit 805b432 into master May 1, 2019
@Gabriella439 Gabriella439 deleted the haskell-implementation branch January 29, 2020 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants