Skip to content

feat: Add support for (custom) types #498

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

Merged
merged 8 commits into from
May 13, 2025
Merged

Conversation

gfrey
Copy link
Contributor

@gfrey gfrey commented Apr 9, 2025

This is the first implementation that was discussed in the slack channel. The other idea of having the types field in the spec part of the schema, seems to be more complicated to implement.

Fixes #144.

Gereon Frey added 4 commits April 8, 2025 14:12
This fragment is repeated in the downstream functions. I guess it is a
left-over from refactorings.
This adds the `Types` field to the ResourceGraphDefinition CRD. It
contains a map of names to type definitions, where the definitions
follow the simple schema.

The defined types are then loaded into the transformer as predefined
types. Appearances in the spec are then properly resolved.

An example for usage would be:

``` yaml
apiVersion: kro.run/v1alpha1
kind: ResourceGraphDefinition
metadata:
  name: service-entry-dns
spec:
  schema:
    apiVersion: v1alpha1
    kind: ServiceEntryDNS
    spec:
      ports: "[]port | required=true"
    types:
      port:
         name: string
         protocol: string
         number: integer | required
```

Known issues: Currently there is no support for references of custom
types within the definition of a custom type!

Fixes kro-run#144
If a simple type was used with the required marker:

``` yaml
types:
  myType: string | required=true description="my description"
spec:
  myValue: myType
```
the required marker was lost, while the description was set properly.
This happened as in the applyMarkers method of the transformer the
required information is set to the parent.

While loading the predefined types this parent is just a shell object,
that is later read to fill the list of predefined types and then
discarded.

Now the information on required types is captured in the
`predefinedType` type and then later used to set the parent object
required fields when handling predefined types.
Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

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

Thanks @gfrey! left two tiny comments

Comment on lines +25 to +29
// A predefined type is a type that is predefined in the schema.
// It is used to resolve references in the schema, while capturing the fact
// whether the type has the required marker set (this information would
// otherwise be lost in the parsing process).
type predefinedType struct {
Copy link
Member

Choose a reason for hiding this comment

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

Good call! initially i wasn't sure about this, but i just remembered that required fields always live parent-type-schema-definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just noticed that adding this broke the test for the predefined types. So, I fixed that.

if err != nil {
t.Fatalf("Failed to load pre-defined types: %v", err)
}
func floatPtr(f float64) *float64 {
Copy link
Member

Choose a reason for hiding this comment

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

looks like this function isn't used, shall we remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely!

flake.lock Outdated
Copy link
Member

Choose a reason for hiding this comment

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

can you please exclude nix files from this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have force pushed the commits that contained them. Sorry for that!

Copy link
Member

Choose a reason for hiding this comment

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

I would love to see the nix files setup for kro. Would you share it in a gist please ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gereon Frey added 3 commits April 22, 2025 09:16
This fragment is repeated in the downstream functions. I guess it is a
left-over from refactorings.
This is reported when `gofmt` is run with the `-s` option.
@gfrey gfrey requested a review from a-hilaly April 29, 2025 07:27
Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

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

Neat, thank you @gfrey !!

@barney-s
Copy link
Member

barney-s commented May 8, 2025

Have a top level Q.
Since this is schema level changes how is spec and status different from custom types ?

Instead of this:

schema:
  types:
    Person:
      name: string
      age: integer
  spec:
    people: '[]Person | required=true`

How does this look like ?

schema:
   Person:
     name: string
     age: integer
  spec:
    people: '[]Person | required=true`

??

@barney-s
Copy link
Member

barney-s commented May 13, 2025

Another alternative is:

schema:
  spec:
    people:
    - _properties: "array | required=true"
      name: string
      age: integer

for maps:

schema:
  spec:
    people: `[]string | required=true`
    peeps:
    - _properties: "array | required=true"
      name: string
      age: integer

@barney-s barney-s merged commit 095e3c0 into kro-run:main May 13, 2025
5 checks passed
barney-s pushed a commit to barney-s/kro that referenced this pull request May 21, 2025
* Add support for (custom) types

This adds the `Types` field to the ResourceGraphDefinition CRD. It
contains a map of names to type definitions, where the definitions
follow the simple schema.

The defined types are then loaded into the transformer as predefined
types. Appearances in the spec are then properly resolved.

An example for usage would be:

``` yaml
apiVersion: kro.run/v1alpha1
kind: ResourceGraphDefinition
metadata:
  name: service-entry-dns
spec:
  schema:
    apiVersion: v1alpha1
    kind: ServiceEntryDNS
    spec:
      ports: "[]port | required=true"
    types:
      port:
         name: string
         protocol: string
         number: integer | required
```

Known issues: Currently there is no support for references of custom
types within the definition of a custom type!

Fixes kro-run#144

* Add documentation for (custom) types
* Fix (custom) simple types with required marker

If a simple type was used with the required marker:

``` yaml
types:
  myType: string | required=true description="my description"
spec:
  myValue: myType
```
the required marker was lost, while the description was set properly.
This happened as in the applyMarkers method of the transformer the
required information is set to the parent.

While loading the predefined types this parent is just a shell object,
that is later read to fill the list of predefined types and then
discarded.

Now the information on required types is captured in the
`predefinedType` type and then later used to set the parent object
required fields when handling predefined types.

* Fix typo in documentation
* Remove unused code
* Fix and improve test for predefined types
* Simplify code
a-hilaly pushed a commit to a-hilaly/kro that referenced this pull request May 22, 2025
* Add support for (custom) types

This adds the `Types` field to the ResourceGraphDefinition CRD. It
contains a map of names to type definitions, where the definitions
follow the simple schema.

The defined types are then loaded into the transformer as predefined
types. Appearances in the spec are then properly resolved.

An example for usage would be:

``` yaml
apiVersion: kro.run/v1alpha1
kind: ResourceGraphDefinition
metadata:
  name: service-entry-dns
spec:
  schema:
    apiVersion: v1alpha1
    kind: ServiceEntryDNS
    spec:
      ports: "[]port | required=true"
    types:
      port:
         name: string
         protocol: string
         number: integer | required
```

Known issues: Currently there is no support for references of custom
types within the definition of a custom type!

Fixes kro-run#144

* Add documentation for (custom) types
* Fix (custom) simple types with required marker

If a simple type was used with the required marker:

``` yaml
types:
  myType: string | required=true description="my description"
spec:
  myValue: myType
```
the required marker was lost, while the description was set properly.
This happened as in the applyMarkers method of the transformer the
required information is set to the parent.

While loading the predefined types this parent is just a shell object,
that is later read to fill the list of predefined types and then
discarded.

Now the information on required types is captured in the
`predefinedType` type and then later used to set the parent object
required fields when handling predefined types.

* Fix typo in documentation
* Remove unused code
* Fix and improve test for predefined types
* Simplify code
a-hilaly pushed a commit to a-hilaly/kro that referenced this pull request May 22, 2025
* Add support for (custom) types

This adds the `Types` field to the ResourceGraphDefinition CRD. It
contains a map of names to type definitions, where the definitions
follow the simple schema.

The defined types are then loaded into the transformer as predefined
types. Appearances in the spec are then properly resolved.

An example for usage would be:

``` yaml
apiVersion: kro.run/v1alpha1
kind: ResourceGraphDefinition
metadata:
  name: service-entry-dns
spec:
  schema:
    apiVersion: v1alpha1
    kind: ServiceEntryDNS
    spec:
      ports: "[]port | required=true"
    types:
      port:
         name: string
         protocol: string
         number: integer | required
```

Known issues: Currently there is no support for references of custom
types within the definition of a custom type!

Fixes kro-run#144

* Add documentation for (custom) types
* Fix (custom) simple types with required marker

If a simple type was used with the required marker:

``` yaml
types:
  myType: string | required=true description="my description"
spec:
  myValue: myType
```
the required marker was lost, while the description was set properly.
This happened as in the applyMarkers method of the transformer the
required information is set to the parent.

While loading the predefined types this parent is just a shell object,
that is later read to fill the list of predefined types and then
discarded.

Now the information on required types is captured in the
`predefinedType` type and then later used to set the parent object
required fields when handling predefined types.

* Fix typo in documentation
* Remove unused code
* Fix and improve test for predefined types
* Simplify code
a-hilaly pushed a commit to a-hilaly/kro that referenced this pull request May 22, 2025
* Add support for (custom) types

This adds the `Types` field to the ResourceGraphDefinition CRD. It
contains a map of names to type definitions, where the definitions
follow the simple schema.

The defined types are then loaded into the transformer as predefined
types. Appearances in the spec are then properly resolved.

An example for usage would be:

``` yaml
apiVersion: kro.run/v1alpha1
kind: ResourceGraphDefinition
metadata:
  name: service-entry-dns
spec:
  schema:
    apiVersion: v1alpha1
    kind: ServiceEntryDNS
    spec:
      ports: "[]port | required=true"
    types:
      port:
         name: string
         protocol: string
         number: integer | required
```

Known issues: Currently there is no support for references of custom
types within the definition of a custom type!

Fixes kro-run#144

* Add documentation for (custom) types
* Fix (custom) simple types with required marker

If a simple type was used with the required marker:

``` yaml
types:
  myType: string | required=true description="my description"
spec:
  myValue: myType
```
the required marker was lost, while the description was set properly.
This happened as in the applyMarkers method of the transformer the
required information is set to the parent.

While loading the predefined types this parent is just a shell object,
that is later read to fill the list of predefined types and then
discarded.

Now the information on required types is captured in the
`predefinedType` type and then later used to set the parent object
required fields when handling predefined types.

* Fix typo in documentation
* Remove unused code
* Fix and improve test for predefined types
* Simplify code
a-hilaly pushed a commit to a-hilaly/kro that referenced this pull request May 22, 2025
* Add support for (custom) types

This adds the `Types` field to the ResourceGraphDefinition CRD. It
contains a map of names to type definitions, where the definitions
follow the simple schema.

The defined types are then loaded into the transformer as predefined
types. Appearances in the spec are then properly resolved.

An example for usage would be:

``` yaml
apiVersion: kro.run/v1alpha1
kind: ResourceGraphDefinition
metadata:
  name: service-entry-dns
spec:
  schema:
    apiVersion: v1alpha1
    kind: ServiceEntryDNS
    spec:
      ports: "[]port | required=true"
    types:
      port:
         name: string
         protocol: string
         number: integer | required
```

Known issues: Currently there is no support for references of custom
types within the definition of a custom type!

Fixes kro-run#144

* Add documentation for (custom) types
* Fix (custom) simple types with required marker

If a simple type was used with the required marker:

``` yaml
types:
  myType: string | required=true description="my description"
spec:
  myValue: myType
```
the required marker was lost, while the description was set properly.
This happened as in the applyMarkers method of the transformer the
required information is set to the parent.

While loading the predefined types this parent is just a shell object,
that is later read to fill the list of predefined types and then
discarded.

Now the information on required types is captured in the
`predefinedType` type and then later used to set the parent object
required fields when handling predefined types.

* Fix typo in documentation
* Remove unused code
* Fix and improve test for predefined types
* Simplify code
a-hilaly pushed a commit to a-hilaly/kro that referenced this pull request May 22, 2025
* Add support for (custom) types

This adds the `Types` field to the ResourceGraphDefinition CRD. It
contains a map of names to type definitions, where the definitions
follow the simple schema.

The defined types are then loaded into the transformer as predefined
types. Appearances in the spec are then properly resolved.

An example for usage would be:

``` yaml
apiVersion: kro.run/v1alpha1
kind: ResourceGraphDefinition
metadata:
  name: service-entry-dns
spec:
  schema:
    apiVersion: v1alpha1
    kind: ServiceEntryDNS
    spec:
      ports: "[]port | required=true"
    types:
      port:
         name: string
         protocol: string
         number: integer | required
```

Known issues: Currently there is no support for references of custom
types within the definition of a custom type!

Fixes kro-run#144

* Add documentation for (custom) types
* Fix (custom) simple types with required marker

If a simple type was used with the required marker:

``` yaml
types:
  myType: string | required=true description="my description"
spec:
  myValue: myType
```
the required marker was lost, while the description was set properly.
This happened as in the applyMarkers method of the transformer the
required information is set to the parent.

While loading the predefined types this parent is just a shell object,
that is later read to fill the list of predefined types and then
discarded.

Now the information on required types is captured in the
`predefinedType` type and then later used to set the parent object
required fields when handling predefined types.

* Fix typo in documentation
* Remove unused code
* Fix and improve test for predefined types
* Simplify code
@gfrey gfrey deleted the custom_types branch May 23, 2025 12:53
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.

Add support for complex data types
4 participants