Conversion should allow explicit declaration of conversion funcs, beyond just naming convention

[akutz]

This issue was filed based on this discussion in the #kubebuilder channel from Kubernetes Slack.

👋 I encountered an oddity with controller-gen where I have two types, Fu, from two packages, ./v1alpha2/subdir and ./v1alpha3/subdir, and the signature controller-gen expects for both of the functions is: Convert_subdir_Fu_To_subdir_Fu. Obviously two functions cannot share the same name in the same package, so this is an issue.

I have tried looking through the GitHub issues, and the closest I can find is #94 (hi @ncdc!), but that only applies to other types that are of type runtime.Object and are conversion hubs. In my case, Fu is a data structure shared by different packages in each schema version. For example, ./api/v1alpha2 and ./api/v1alpha2/hello might both import ./api/v1alpha2/subdir.

I have experimented with several different approaches, including defining the Convert functions for the parent type that includes Fu, but I would still receive compileOnError warnings in the generated output about the expected function. The only solution I found is the following:

|-- v1alpha2
    |-- conversion
        |-- v1alpha2
        |   |-- fu_conversion.go
        |-- v1alpha3
            |-- fu_conversion.go

In ./v1alpha2/conversion/v1alpha2/fu_conversion.go I have one Convert_subdir_Fu_To_subdir_Fu, and I have the same signature in the other file, with the arg types reversed. With the above, I add the two directories to the --input-dirs param, and it works.

Is the above solution really the only option?

cc @bryanv

[ncdc]

Hi @akutz! I think your solution is the only one that comes to mind for me.

[akutz]

Thanks @ncdc. That is unfortunate, as it really is a hacky hack. Still, it is better than not having a solution at all I suppose. I will leave this issue open for the time being in case anyone else wants to add their thoughts. If no one has spoken up by next week, I will close this. I may also file a PR against kubebuilder and update docs to reflect the above.

[akutz]

It looks like --input-dirs was dropped in the latest release?!

[akutz]

Apparently d697340 removed --input-dirs when k8s.io/gengo args were removed. cc @thockin -- you authored the referenced commit -- it broke the use of --input-dirs for the use case this issue describes. Thoughts?

[thockin]

gengo and all the tools which use it underwent a massive overhaul to make Go workspaces work, which necessarily included breaking CLI changes. If you are using a newer version of the tools, then you can drop the --input-dirs part and just pass the dirs directly as arguments, but you may need to make other changes to get the flags right. Looking at conversion-gen, it may have been one of the less impacted tools - others got hit harder.

As for this bug, I don't mean to make excuses, but subdirs are just not something that was used/tested in k/k, so it's not a surprise it breaks. The way we detect conversion functions is, IMO, awful (for reasons like this, among others) and I don't think there's a "clean" fix because the naming convention is baked pretty deep. I think the "real" fix would be to have explicit tags indictating "this function converts from X to Y". It sounds like a fun project, but isn't something I personally can tackle any time soon.

[akutz]

Thanks @thockin. I will try your suggestion about passing in the arguments directly.

[akutz]

Thanks @thockin, a variation on your suggestion worked like a charm, thanks again!

/Users/akutz/Projects/vmop/vmop/hack/tools/bin/darwin_arm64/conversion-gen \
	--output-file=zz_generated.conversion.go \
	--go-header-file=/Users/akutz/Projects/vmop/vmop/hack/boilerplate/boilerplate.generatego.txt \
	--extra-peer-dirs='./v1alpha2/sysprep/conversion/v1alpha2,./v1alpha2/sysprep/conversion/v1alpha3' \
	./v1alpha1 ./v1alpha2

[akutz]

@thockin I am reluctant to close this issue, however, as your response indicates my hack is just that, a hack. A proper solution would be to do what you said re: tags. Perhaps I will close this issue and open a new one for an enhancement request that can track your proposed solution. The new issue can point to this one for reference.

[thockin]

Let's just retitle this one. I don't have super-access to this repo but something like "conversion should allow explicit declaration of conversion funcs, beyond just naming convention"

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle rotten
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[thockin]

/remove-lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale