code-generator

code-generator copied to clipboard

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

:wave: 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 https://github.com/kubernetes/code-generator/issues/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

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

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.

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

Apparently https://github.com/kubernetes/code-generator/commit/d697340b658daf8072223c1dc0bd9c628c0ecc69 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?

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.

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

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

@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.

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"

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

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

/remove-lifecycle rotten