crd-ref-docs icon indicating copy to clipboard operation
crd-ref-docs copied to clipboard

fix: load external types from imports

Open darkweaver87 opened this issue 1 year ago • 1 comments

This PRs makes it possible to render imported types in a CRD.

For instance:

package v1alpha1

import (
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	"github.com/starwars/jedi/pkg/weapons"
)

// +genclient
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
// +kubebuilder:storageversion

// Jedi is a Jedi
type Jedi struct {
	metav1.TypeMeta `json:",inline"`
	// Standard object's metadata.
	// More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata
	metav1.ObjectMeta `json:"metadata"`

	Spec JediSpec `json:"spec"`
}

// +k8s:deepcopy-gen=true

// JediSpec defines the desired state of a Jedi.
type JediSpec struct {
	Master *Jedi `json:"master,omitempty"`
	LightSaber *weapons.LightSaber `json:"lightSaber,omitempty"`
}

Without it, references are empty and https://github.com/elastic/crd-ref-docs/blob/master/renderer/markdown.go#L84 returns false.

Do you think this should be added as an opt-in or leaving it as default behavior is OK ?

darkweaver87 avatar May 07 '24 15:05 darkweaver87

💚 CLA has been signed

I'm not sure I follow what you do you expect. Could you update the test/ directory with a real example (update guestbook_types.go and the expected.[md|asciidoc] files)?

We get an _invalid type_ with your example as the type doesn't really exist. But it seems to me that external types are rendered because one reference is detected.

# after adding: fmt.Println("Type:", t, " ShouldRenderType:", result, " GVK:", t.GVK != nil, " Refs:", t.References) in MarkdownRenderer#ShouldRenderType
Type: github.com/elastic/crd-ref-docs/api/v1.Jedi  ShouldRenderType: true  GVK: false  Refs: [github.com/elastic/crd-ref-docs/api/v1.JediSpec]
Type: github.com/elastic/crd-ref-docs/api/v1.JediSpec  ShouldRenderType: true  GVK: false  Refs: [github.com/elastic/crd-ref-docs/api/v1.Jedi]

IIUC, with your PR we will inline part of the external type doc. I am not sure know who wants this. Isn't the current mechanism for declaring links for external types sufficient?

render:
  knownTypes:
    - name: LightSaber
      package: github.com/starwars/jedi/pkg/weapons
      link: https://starwars.com/guide/en/jedi/current/k8s-api-weapons-v1.html#k8s-api-github-com-starwars-blabla

thbkrkr avatar Jul 23 '24 15:07 thbkrkr

Well, to be honest, I'm not using this project anymore. Too much changes were needed to adapt it to suite my needs :-) but I will take the time to finish this PR if it makes sense for you / your project :-) The external types was not sufficient enough because I would expect from this kind of tool to at least detect all types from a given project. The example I provided is simplified but the source code I was running crd-ref-docs against provides a v1alpha1 CRD in which some types are defined but also import some types defined in other packages which are part of the same project. To be more accurate, I was trying to render CRD docs for this Middleware type. As you can see this type import some others types which are not part of v1alpha1 package like AddPrefix. This is the purpose of this PR. What do you think about this need ?

darkweaver87 avatar Jul 24 '24 07:07 darkweaver87

Ah ok, so "external types" are what I'll call "internal types" from another package. In this case I would expect all of these types to belong to your API and actually appear in the generated documentation. If they are declared correctly as any type with a group and version, you shouldn't have to do anything.

To give an example, look at the common types defined here: elastic/cloud-on-k8s/tree/main/pkg/apis/common/v1 and the generated doc with:

thbkrkr avatar Jul 24 '24 11:07 thbkrkr

Thanks for providing examples. I'll give another try and check why it's not working on my side :-)

darkweaver87 avatar Jul 24 '24 12:07 darkweaver87

OK so the issue was coming from my custom template actually :-) sorry You can close this PR :-)

darkweaver87 avatar Jul 24 '24 14:07 darkweaver87