Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mattkelly opened this issue Dec 30, 2017 · 15 comments
Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.

Comments

@mattkelly
Copy link

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
Copy link
Member

nikhita commented Dec 30, 2017

Related kubernetes/kubernetes#56489.

@jennybuckley
Copy link

jennybuckley commented Jan 10, 2018

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

@mattkelly
Copy link
Author

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

@jennybuckley
Copy link

/close
kubernetes/kubernetes#59674

@neolit123
Copy link
Member

/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 k8s-ci-robot reopened this Apr 22, 2019
@k8s-ci-robot
Copy link

@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
Copy link

rosti commented Apr 23, 2019

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
Copy link
Member

neolit123 commented Apr 23, 2019

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
Copy link

rosti commented Apr 23, 2019

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
Copy link

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

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 22, 2019
@neolit123
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 22, 2019
@fejta-bot
Copy link

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

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 20, 2019
@jennybuckley jennybuckley removed their assignment Oct 29, 2019
@fejta-bot
Copy link

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

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 28, 2019
@neolit123
Copy link
Member

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Nov 28, 2019
@thockin
Copy link
Member

thockin commented Mar 12, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Development

No branches or pull requests

8 participants