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

[K8s Contrib CRD] Duplicate class definitions in generated module #40

Open
mruoss opened this issue Mar 11, 2024 · 8 comments · May be fixed by #77
Open

[K8s Contrib CRD] Duplicate class definitions in generated module #40

mruoss opened this issue Mar 11, 2024 · 8 comments · May be fixed by #77

Comments

@mruoss
Copy link

mruoss commented Mar 11, 2024

The example CRD below secifies .first.some_class and .second.some_class. The CRD generator successfully generates a module. However, the module defines the class SomeClass twice which leads to an error upon evaluation.

The CRD

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: foos.example.com
spec:
  group: example.com
  names:
    kind: Foo
    plural: foos
  scope: Namespaced
  versions:
    - name: v1
      schema:
        openAPIV3Schema:
          properties:
            first:
              type: object
              properties:
                some_class: 
                  type: object
                  properties:
                    foo: 
                      type: string
            second:
              type: object
              properties:
                some_class: 
                  type: object
                  properties:
                    bar: 
                      type: string
          type: object
      served: true
      storage: true

Resulting PKL

/// This module was generated from the CustomResourceDefinition at
/// <file:///Users/mruoss/src/community/pkl/playground/example.yaml>.
module com.example.v1.Foo

extends "package://pkg.pkl-lang.org/pkl-k8s/[email protected]#/K8sResource.pkl"

import "package://pkg.pkl-lang.org/pkl-k8s/[email protected]#/apimachinery/pkg/apis/meta/v1/ObjectMeta.pkl"

fixed apiVersion: "example.com/v1"

fixed kind: "Foo"

/// Standard object's metadata.
///
/// More info: <https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata>.
metadata: ObjectMeta?

first: First?

second: Second?

class First {
  some_class: SomeClass?
}

class SomeClass {
  foo: String?
}

class Second {
  some_class: SomeClass?
}

class SomeClass { // <-- second definition of SomeClass
  bar: String?
}
@FredrikAugust
Copy link
Contributor

FredrikAugust commented Mar 19, 2024

Hi! This happens to me as well. Would it be possible to prefix the class names with the parent(s) to avoid name collisions?

E.g.

spec.template.spec

would become

class Spec

class SpecTemplate

class SpecTemplateSpec

@holzensp
Copy link
Contributor

I don't know the problem space well enough. Are you sure these are always separate classes? Can they be the same class, but with fields that can be omitted? A more Pkly thing could be to have separate modules for First and Second, in which case you already have First.SomeClass "externally," without adding repetitive verbosity to internal use.

@jstrachan
Copy link

did this fix it?
#20

@lduvnjak
Copy link

I ran into this issue today with trying to convert Elastic Cloud on Kubernetes (ECK) CRDs to PKL.
Here is the command I ran:

pkl eval package://pkg.pkl-lang.org/pkl-pantry/[email protected]#/generate.pkl \
   -m . \
   -p source="https://download.elastic.co/downloads/eck/2.12.1/crds.yaml"

Converting the pkl file back to yaml gives the following error:

# pkl eval Elasticsearch.pkl
–– Pkl Error ––
Duplicate definition of member `Spec`.

142 | class Spec {
      ^^^^^^^^^^
at co.elastic.k8s.elasticsearch.v1.Elasticsearch (file:///root/venv/eck/pkl/test/Elasticsearch.pkl, line 142)

@Avarei
Copy link

Avarei commented Jun 14, 2024

I have the same problem:

$ pkl eval "package://pkg.pkl-lang.org/pkl-pantry/[email protected]#/generate.pkl" -m . -p source="https://raw.githubusercontent.com/crossplane/crossplane/master/cluster/crds/apiextensions.crossplane.io_compositions.yaml"

$ pkl eval Composition.pkl

–– Pkl Error ––
Duplicate definition of member `String`.

370 | class String {
      ^^^^^^^^^^^^

also the generated class name "String" is not ideal

@Avarei
Copy link

Avarei commented Jun 20, 2024

I don't know the problem space well enough. Are you sure these are always separate classes? Can they be the same class, but with fields that can be omitted? A more Pkly thing could be to have separate modules for First and Second, in which case you already have First.SomeClass "externally," without adding repetitive verbosity to internal use.

I believe this would be a great solution. Though I think the decision on when and where to create a seperate module should not be made automatically.

Maybe similarly to converters we could decide for which paths a seperate module should be created?

createModules {
  ["foos.v1.example.com"] {
    new List("first")
    new List("second")
  }
}

or a more complex Example the Kubernetes Deployment if it were a CRD:

createModules {
  ["deployments.apps.api.k8s"] {
    [new List("selector")] = "LabelSelector"
    [new List("spec", "template")] = "PodTemplateSpec"
    [new List("spec", "template", "spec")] = "PodSpec"
    [new List("spec", "template", "spec", "imagePullSecrets")] = "LocalObjectReference"
    [new List("spec", "template", "spec", "securityContext", "seLinuxOptions")] = "SELinuxOptions"
    // and so on.
  }
}

So a CRD of a Deployment would contain these Modules:

@Avarei
Copy link

Avarei commented Jul 3, 2024

In case it helps someone else:
I just implemented @FredrikAugust's suggestion as a workaround in my local environment.

I replaced

let (typeName = determineTypeName(path, path.lastOrNull?.capitalize() ?? "Item", accumulator.values.toSet(), 0))

With

let (typeName = determineTypeName(path, path.fold(List(), (res: List<String>, el: String) -> res.add(el.capitalize())).join(""), accumulator.values.toSet(), 0))

which generates beatiful class names like SpecEnvironmentEnvironmentConfigsEnvironmentConfigSelectorMatchLabelsMatchLabel

@ichekrygin
Copy link

@Avarei, have you considered submitting a PR with that ^ fix?

ichekrygin pushed a commit to ichekrygin/pkl-pantry that referenced this issue Oct 7, 2024
…ng properties (objects, list). This change will result in: "spec.foo.bar":

- current: classes ["Spec", "Foo", "Bar"]
- change: classes ["Spec", "SpecFoo", "SpecFooBar"

This change is suggested by https://github.com/Avarei in apple#40 (comment)
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 a pull request may close this issue.

7 participants