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

Fix kubernetes import version #54

Closed
wants to merge 0 commits into from

Conversation

arnaudbos
Copy link
Contributor

@arnaudbos arnaudbos commented Feb 14, 2020

Regarding #53, I have finally been able to get the following example (from the README) to work, with a few adjustments:

let packages = ./dhall-packages/package.dhall sha256:07634f215667de0270f7d8ce94210fb8195e81e384206bf349054b07d3f5e2fe

--let k8s = ./dhall-packages/kubernetes/k8s/1.17.dhall sha256:e9c55c7ff71f901314129e7ef100c3af5ec7a918dce25e06d83fa8c5472cb680
let k8s = packages.kubernetes.k8s.`1-17`

--let argocd = ./dhall-packages/kubernetes/argocd/package.dhall sha256:82aa39a0f29514d401ac8ec3035c91e7c20cf5ef08dbd84dcc5ff56c7d40ffdb
let argocd = packages.kubernetes.argocd

let application =
      argocd.Application::{
      , metadata = k8s.ObjectMeta::{ name = "hello-app" }
      , spec = argocd.ApplicationSpec::{
        , project = "hello-project"
        , source =
            argocd.SourceSpec.TypesUnion.Plugin
              argocd.PluginSourceSpec::{
              , repoURL =
                  "https://github.com/EarnestResearch/dhall-packages.git"
              , path = "kubernetes"
              , plugin = argocd.PluginSpec::{ name = "dhall-to-yaml" }
              }
        , destination = argocd.DestinationSpec::{
          , server = "kubernetes.svc.local"
          , namespace = "default"
          }
        }
      }

in  application

I'm not sure about the .`1.17` part above but .schema doesn't seem to exist anymore and .k8s alone didn't work.

The only differences are inside ./dhall-packages/kubernetes/argocd/Application/Type.dhall, where I've changed the dhall-kubernetes version + the hash, and ./dhall-packages/kubernetes/k8s/1.1.dhall where I've changed the kubernetes version and hash too.

I'm not sure this is the way to go.

  1. May break users of kubernetes 1.14?
  2. Doesn't fix other packages affected by this, such Argo for instance (as my initial target)

Anyways, this offers a support for discussion.
Happy to take feedback in order to improve!

@arnaudbos arnaudbos requested a review from amarrella February 14, 2020 22:54
Copy link
Collaborator

@amarrella amarrella left a comment

Choose a reason for hiding this comment

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

Hi @arnaudbos, thanks for opening the issue and this pull request!

I agree with the change of fixing the 1.17 in https://github.com/EarnestResearch/dhall-packages/pull/54/files#diff-1cdd891015865cf9e19fd2e1d1c20840L1 as the link was pointing to the wrong location (thanks for fixing it!).

Thanks also for fixing the example :)

With regards to replacing 1.14 with 1.17 in the argocd manifest i would be a bit more cautious as i see the following diff which would break current users:

❯ dhall diff "https://raw.githubusercontent.com/dhall-lang/dhall-kubernetes/6a47bd50c4d3984a13570ea62382a3ad4a9919a4/1.14/types/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta.dhall" "https://raw.githubusercontent.com/dhall-lang/dhall-kubernetes/6a47bd50c4d3984a13570ea62382a3ad4a9919a4/1.17/types/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta.dhall"
{ - initializers : …
,   managedFields : …
                    { - fields : …
                    , + fieldsType : …
                    , + fieldsV1 : …
                    , …
                    }
, …
}

I think though that we need a solution to support multiple versions of kubernetes to support more users. What we can do is:

  • create a custom "ObjectMeta" field with all the diffed values optional, and perhaps a conversion function from dhall-kubernetes' ObjectMeta to this specific type
  • create a union type of all the ObjectMetas
  • fork all the types within the kubernetes directory and instead have a directory for each k8s version this repo supports

The first and second solution would fix the problem with this specific package, while the third will address this problem not only for ObjectMeta, but also the types that we have e.g. in https://github.com/EarnestResearch/dhall-packages/tree/master/kubernetes/argo/types at the cost of a higher maintenance burden in the hand-crafted packages (perhaps we can mitigate it with a two-step generation of this files).

If you don't mind, could you please remove the ObjectMeta change from this PR (or open a different PR with just the fixes) so that we can discuss this in an issue? I want to get other users involved to come up with the best solution.

Thanks! :)

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

Successfully merging this pull request may close these issues.

2 participants