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

inline field description added for CRD generation #1133

Open
dongjiang1989 opened this issue Jan 22, 2025 · 8 comments
Open

inline field description added for CRD generation #1133

dongjiang1989 opened this issue Jan 22, 2025 · 8 comments

Comments

@dongjiang1989
Copy link
Contributor

dongjiang1989 commented Jan 22, 2025

The embedded types (CommonSpec) need to be annotated with json:",inline" with some description. so the fields description added in the generated CRD

e.g.

type MyAddonSpec struct {
    // this is CommonSpec description
    CommonSpec `json:",inline"`
}

// some other description
type CommonSpec struct {
     Name *string `json:"name,omitempty"`
}
@JoelSpeed
Copy link
Contributor

Where in the schema would this go? As far as I'm aware, when you have an inlined structure, the inlined field does not appear in the schema at all, so there wouldn't be an appropriate place to put the commonspec description. Did you have somewhere in mind?

@sbueringer
Copy link
Member

sbueringer commented Jan 22, 2025

As far as I remember there is basically one description field in the schema for this and we pick one of these godoc comments to set it (and not both)

@dongjiang1989
Copy link
Contributor Author

Thanks @JoelSpeed @sbueringer

In my opinion, is it possible to add this inline field description to the upper base description field and if the inline field has description then use it otherwise use inline structure description?

@JoelSpeed
Copy link
Contributor

Can you provide a concrete example? I'm not sure what you actually mean by "upper base description" and an example of where in the generated schema this would end up would probably be helpful

@dongjiang1989
Copy link
Contributor Author

dongjiang1989 commented Jan 23, 2025

Thanks @JoelSpeed

Demo case:

// This tests that embedded struct, which is an alias type, is handled correctly.
InlineAlias `json:",inline"`
}
type InlineAlias = EmbeddedStruct
// EmbeddedStruct is for testing that embedded struct is handled correctly when it is used through an alias type.
type EmbeddedStruct struct {
// FromEmbedded is a field from the embedded struct that was used through an alias type.
FromEmbedded string `json:"fromEmbedded,omitempty"`
}

Can add this description to:

description: CronJobSpec defines the desired state of CronJob
properties:
aliasFromPackage:
description: |-
This tests that alias imported from a package is handled correctly. The
corev1.IPFamilyPolicyType is just reused since it's available from
imported package. We can create our own in a separate package if needed.

Add to L42 or L45?

@JoelSpeed
Copy link
Contributor

You've linked the wrong part of the CRD schema, the actual part here is

fromEmbedded:
description: FromEmbedded is a field from the embedded struct that
was used through an alias type.
type: string

You can see the fromEmbedded field from the EmbeddedStruct is alongside things like ForbiddenInt which are siblings of InlineAlias. There is no element here that represents either the field InlineAlias or the struct EmbeddedStruct, and as such, I don't see anywhere to put these descriptions.

If you added them to the parent of fromEmbedded in the schema, that would imply merging them with the parent of InlineAlias in the go types, which would then affect all of the other fields within that parent.

If you were to merge with the description of fromEmbedded in the schema, then that implies copying these descriptions into all of the children of EmbeddedStruct which I also don't think is what you want.

@dongjiang1989
Copy link
Contributor Author

Whether to add new fields to populate it?

@JoelSpeed
Copy link
Contributor

I'm not quite certain what you mean by add a new field, I can interpret that in two ways.

Either you mean add a new field into this specific schema, which would then change the shape of the serialized resources, you'd go from

spec:
  fromEmbedded: ...

to

spec:
  inlineAlias:
    fromEmbedded: ...

Which is the same effect as changing the json tag to not be inline. Changing this behaviour would be majorly breaking.

Or you mean to add an extra description like field into the CRD OpenAPIV3Schema JSONSchemaProps, which would mean extending the openapi spec used within Kubernetes, in which case, I think this is definitely beyond the scope of controller-tools.


Moving aside from the mechanics here, why do you actually want the documentation from the inlined field to appear on your CRD schema? I'm not sure I get what the use case is here? You can add documentation to individual fields already, so what's missing that prevents you from achieving some goal for your project?

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

3 participants