search

kubernetes / code-generator

Generators for kube-like API types
Apache License 2.0
1.7k stars 410 forks source link

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

Open mattkelly opened 6 years ago

mattkelly commented 6 years ago

Overview

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

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 commented 6 years ago

Related https://github.com/kubernetes/kubernetes/issues/56489.

jennybuckley commented 6 years ago

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

mattkelly commented 6 years ago

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

jennybuckley commented 6 years ago

/close https://github.com/kubernetes/kubernetes/pull/59674

neolit123 commented 5 years ago

/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

k8s-ci-robot commented 5 years ago

@neolit123: Reopened this issue.

In response to [this](https://github.com/kubernetes/code-generator/issues/30#issuecomment-485511925): >/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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
rosti commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

/remove-lifecycle stale

fejta-bot commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

/lifecycle frozen

thockin commented 5 months ago

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.