Generated files should follow golang standard for machine-generated files to avoid golint issues

[mattkelly]

Overview

Files generated by code-generator currently have a lot of issues picked up by golint. There are a few options to fix this:

• Fix the actual lint issues in the generated code
  • This would require fixing the existing generated code and putting a process in place for the future to prevent lint issues in generated code from being merged
• Ignore the generated files in some other way
  • This requires all users to implement their own solution to this problem
• Follow the golang convention for machine-generated files so golint automatically skips them
  • I think this is the most appropriate solution because the golang community has standardized on it, it's trivial to implement, and it avoids adding more process

Proposed Solution

I propose that we add the following comment line just below the boilerplate header:

// Code generated by <generator-name>. DO NOT EDIT.

There are already lines similar to this generated but they do not match the desired regex, so we can just modify them. For example: https://github.com/kubernetes/kubernetes/search?utf8=✓&q=%22Do+not+edit+it+manually%22&type=Code.

After some quick poking around, this will require modifying not only code-generator but also kubernetes/gengo for example.

I've tested manually editing an auto-generated file to add this line and it does result in golint ignoring the file as expected.

I'm not very familiar with the process for auto-generated code in Kubernetes itself - would we want to re-generate all of the generated code as part of the PR for this issue?

I'm happy to take this on this weekend if we feel that it's a good solution.

[nikhita]

Related kubernetes/kubernetes#56489.

[jennybuckley]

@mattkelly Does "<generator-name>" mean "deepcopy-gen", "conversion-gen", etc?
something like this?

[mattkelly]

@jennybuckley yup, that's what I was picturing. That looks good to me!

[jennybuckley]

/close
kubernetes/kubernetes#59674

[neolit123]

/reopen

the defaulter still has some linter problems.

it generates function names with underscores _.
which makes it difficult to pass the modern 1.12+ golint on packages that include generated API methods.

e.g.
https://github.com/kubernetes/kubernetes/pull/76710/files#diff-9a2f5c3d2d4df5c049e0951f45d50133R69

see the discussion here:
kubernetes/kubernetes#76710 (comment)

[k8s-ci-robot]

@neolit123: Reopened this issue.

In response to this:

/reopen

the defaulter still has some linter problems.

it generates function names with underscores _.
which makes it difficult to pass the modern 1.12+ golint on packages that include generated API methods.

e.g.
https://github.com/kubernetes/kubernetes/pull/76710/files#diff-9a2f5c3d2d4df5c049e0951f45d50133R69

see the discussion here:
kubernetes/kubernetes#76710 (comment)

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/test-infra repository.

[rosti]

Currently zz_generated.* files are ignored. The problem is that the defaulter generates code in those files, that call SetDefault_TypeName funcs. Those are meant to be written by the API implementers and don't reside in zz_generated.* files.
It would be superb if we provide an option for the code generators to call in (and even generate actually) code that is linter proof.

[neolit123]

also we have to note that fixing the underscores will result in code that no longer passes /hack verification.
which means that the PR that fixes the existing files will be pretty big.

[rosti]

That's why I propose an option to the generators, that can enable this behavior (so that we don't have to fix everything).

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[neolit123]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[neolit123]

/lifecycle frozen

[thockin]

Revisiting long-lost issues.

It's true that conversion-gen expects manually-written conversions to be named like Convert_v1_Secret_To_core_Secret and that defaulter-gen expects manually-written functions to be named like SetDefaults_Service. It's kind of horrible. I'd welcome it if someone wants to make those explicit, e.g.

Instead of Convert_core_PodSpec_To_v1_PodSpec() and Convert_v1_PodSpec_To_core_PodSpec() being "found":

// PodSpec is a description of a pod.
// +k8s:conversion-gen:manual=to:k8s.io/kubernetes/pkg/apis/core.PodSpec=k8s.io/kubernetes/pkg/apis/v1.ConvertCorePodSpecToV1PodSpec    
// +k8s:conversion-gen:manual=from:k8s.io/kubernetes/pkg/apis/core.PodSpec=k8s.io/kubernetes/pkg/apis/v1.ConvertV1PodSpecToCorePodSpec                                                                                                                                                      
type PodSpec struct {             

It's not exactly obvious, because there are 3 packages in play, but I think it could be done. Defaulting would be even easier.