code-generator
code-generator copied to clipboard
kubernetes
Generated files should follow golang standard for machine-generated files to avoid golint issues
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
golintautomatically 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.
/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: https://github.com/kubernetes/kubernetes/pull/76710#discussion_r277089993
@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: https://github.com/kubernetes/kubernetes/pull/76710#discussion_r277089993
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.
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.
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.
That's why I propose an option to the generators, that can enable this behavior (so that we don't have to fix everything).
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
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
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
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.
