🌱 Add missing comment tags

[manusa]

What this PR does / why we need it:

• fix: add json tags to match field annotation tag +patchStrategy

• fix: add missing doc.go for proper groupName inference
  Most go code generators such as kube-openapi rely on gengo [1] to infer the annotated tags.
  When dealing with package annotations, gengo extracts them from the doc.go file [2].

  The types defined in apis/metal3.io/v1alpha1 do have a package annotated with +groupName.
  However, the annotation is in the groupversion_info.go file instead.

  You can see comments that refer to this issue [3] [4]

  [1] https://github.com/kubernetes/gengo/
  [2] https://github.com/kubernetes/gengo/blob/2b36238f13e9b8aebe4c286c2ffb2fc71f4be3c5/types/types.go#L121-L126
  [3] applyconfiguration-gen does not respect the +groupName tag and uses the plain apiVersion kubernetes/code-generator#150 (comment)
  [4] groupName defined in types.go is not used for fake clients kubernetes/code-generator#135

[metal3-io-bot]

Hi @manusa. Thanks for your PR.

I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[tuminoid]

/ok-to-test

[tuminoid]

/cc @kashifest @zaneb @elfosardo

[kashifest]

/approve
I am ok with this change, but I would like @zaneb to have a look also .
/hold

[metal3-io-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kashifest

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [kashifest]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[zaneb]

TBH I have no idea what any of this does.

[zaneb]

These comments already exist here. Do we need 2 or should we delete those ones?

[manusa]

I left them there because I'm unsure if someone might be relying on those.
IMO they should be just in doc.go, but it might be better to defer the removal or leave them there after all.

I tried to find an example of a similar situation in a different project from the metal3-io org.
You can find something similar going on in the cluster-api-provider project.
The v1alpha5 types contain both doc.go and groupversion_info.go files with the duplicate comment tags.
The same happens for the more recent v1beta1:

• https://github.com/metal3-io/cluster-api-provider-metal3/blob/5dceef85d4794a647161e2bfac57b66554e573b2/api/v1beta1/doc.go#L22
• https://github.com/metal3-io/cluster-api-provider-metal3/blob/5dceef85d4794a647161e2bfac57b66554e573b2/api/v1beta1/groupversion_info.go#L22

TBH I have no idea what any of this does.

The comment tags are mainly used for code and spec generation.
Hence the breaking changes in the generated code, which is now actually considering the tag that was previously ignored since it was only in the groupversion_info.go file.

In my case I'm using them to generate an OpenAPI spec with kube-openapi and with that spec generate reliable Java types 🤯 (fabric8io/kubernetes-client#6417).

[lentzi90]

Looks fine to keep both.

[kashifest]

Suggested change

	// +kubebuilder:object:generate=true
	// +k8s:openapi-gen=true

Shouldn't this be k8s:openapi-gen=true then instead if this is meant for kube-openapi ?

[manusa]

Yes, definitely. In my case I don't need those because I'm using a customized filter function for kube-openapi.
But for the standard usecase it's most definitely required.

[lentzi90]

Please add

// +k8s:openapi-gen=true

[manusa]

It was already added

[lentzi90]

Oh, sorry! I missed it since it was below the comment 🤦

[kashifest]

@lentzi90 do you have any opinion on this? otherwise we can unhold

[lentzi90]

/lgtm
/hold cancel