Fixes Golint Errors: staging/src/k8s.io/kube-aggregator #73369

eloyekunle · 2019-01-27T13:19:33Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug

/kind cleanup

/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
Fixes lint issues in Go files under: staging/src/k8s.io/kube-aggregator.
Removed staging/src/k8s.io/kube-aggregator from hack/.golint_failures.

Special notes for your reviewer:
See #68026 for more information.

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2019-01-27T13:19:41Z

Hi @eloyekunle. Thanks for your PR.

I'm waiting for a kubernetes 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/test-infra repository.

eloyekunle · 2019-01-27T13:21:43Z

/assign @lavalamp

neolit123

@eloyekunle thank you for the PR.
please squash the commits into one.

/priority backlog
/kind cleanup

eloyekunle · 2019-01-27T18:06:49Z

Hi @neolit123
I've squashed the commits.

neolit123 · 2019-01-27T18:10:24Z

/ok-to-test

fejta · 2019-01-28T08:03:38Z

please pass tests

eloyekunle · 2019-01-28T13:57:32Z

@fejta I don't know why the pull-kubernetes-node-e2e test is still failing. From the logs, it seems something is wrong with GCE.

fejta · 2019-01-28T23:37:49Z

/retest

eloyekunle · 2019-01-30T13:44:45Z

@fejta @neolit123
Thanks, the tests pass now.

sttts · 2019-02-07T12:27:25Z

staging/src/k8s.io/apiserver/pkg/registry/rest/create-update.go

+	// unconditionally (irrespective of the latest resource version), when
+	// there is no resource version specified in the object.
+	AllowUnconditionalUpdate() bool
+}


can one test that it is complete?

var _ RESTUpdateStrategy = nil.(RESTCreateUpdateStrategy)

Is that valid code?

Yes, one can test with:

var _ RESTCreateStrategy = (RESTCreateUpdateStrategy)(nil)

I've added the tests.

sttts · 2019-02-07T12:27:57Z

staging/src/k8s.io/apiserver/pkg/registry/rest/create-update.go

@@ -0,0 +1,46 @@
+/*
+Copyright 2014 The Kubernetes Authors.


sttts · 2019-02-07T12:28:34Z

Some last nit, then lgtm.

eloyekunle · 2019-02-07T14:09:47Z

@sttts
I've addressed the comments.

rajathagasthya · 2019-02-07T17:18:39Z

staging/src/k8s.io/apiserver/pkg/registry/rest/create-update.go

@@ -0,0 +1,52 @@
+/*


nit: could you name this file create_update.go (underscore) instead of the hyphen?

Hi @rajathagasthya
I've updated the file name.

liggitt · 2019-02-08T21:58:31Z

staging/src/k8s.io/kube-aggregator/pkg/controllers/autoregister/autoregister_controller.go

 // for cases where you want to auto-register APIs like TPRs or groups from the core kube-apiserver
-type autoRegisterController struct {
+type Controller struct {


we shouldn't export this type just to make lint happy

Across the codebase, most controllers similar to this are exported.

For example:

APIServiceRegistrationController.

CertificateController

AutoRegisterController

AvailableConditionController

CloudNodeController

and many others are exported, and they all have 'getters', similar to this one (i.e. NewControllerName) returning the exported controller.

Is there another way to satisfy Golint, or will I have to leave this package in .golint_failures?

@liggitt I agree with @eloyekunle. Let's not block this by making Controller public. We need a better golint exception mechanism.

I agree with @eloyekunle. Let's not block this by making Controller public

not sure if you are suggesting keeping this package in .golint_failures or making Controller public. I'd prefer not to export just to make golint happy, but I don't feel that strongly

Jordan, why is it important for this to stay private?

Automated systems like this and gofmt don't always produce the ideal decision at the micro level (maybe you hate tabs, or disagree with lint's exported type directives), but at the macro level the value of providing consistent expectations makes it easier for large teams to collaborate on large code bases.

Removing this package from .golint_failures is useful for two reasons:

It ensures no other, potentially more useful failures, get checked into this package.

A shorter .golint_failure exception lint makes it more likely other packages will fix their lint errors rather than being one of the few special-cased packages.

IMO it is generally an unproductive use of time to try and fight the linting system (but maybe this is an exceptional circumstance?)

why is it important for this to stay private?

this is a package in an exported repo, so making the controller class public implies it is a suitable API for other projects to depend on. I don't think its structure has been considered in that light, so simply making it public to pass a linting check seems subpar

Jordan, is this the only controversial part about the PR?

Elijah, if so how would you feel about removing this part from this PR. This will expedite removing most of the lint errors.

If you're up for it, I'd suggest creating a second PR with just the autoRegisterController change (and removing the lint exception). We can continue the discussion there.

yes, everything else looked fine to me

Sounds good, I've removed this part from the PR. The controller is now unexported.

liggitt · 2019-02-08T21:59:59Z

one comment on exported type, lgtm otherwise

sttts · 2019-02-11T10:06:19Z

one comment on exported type, lgtm otherwise

We had this discussion before, even with the golint people. I still consider export of private structs as a good pattern, golint doesn't. Introducing interfaces is ugly.

liggitt · 2019-02-11T22:00:27Z

/lgtm
/approve

k8s-ci-robot · 2019-02-11T22:00:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eloyekunle, fejta, liggitt

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:

~~api/OWNERS~~ [liggitt]
~~hack/OWNERS~~ [fejta,liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [liggitt]
~~staging/src/k8s.io/kube-aggregator/pkg/apis/OWNERS~~ [liggitt]

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

eloyekunle · 2019-02-11T22:33:47Z

Feels so good to have my first contribution to Kubernetes! 😄
Thanks everyone.

s-chand · 2019-02-12T08:34:02Z

Nice work man!

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 27, 2019

k8s-ci-robot requested review from fejta and mwielgus January 27, 2019 13:19

k8s-ci-robot assigned lavalamp Jan 27, 2019

neolit123 reviewed Jan 27, 2019

View reviewed changes

k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 27, 2019

eloyekunle force-pushed the lint/staging/src/k8s.io/kube-aggregator branch from 07978fb to f3c092b Compare January 27, 2019 18:05

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 27, 2019

eloyekunle force-pushed the lint/staging/src/k8s.io/kube-aggregator branch 2 times, most recently from 5ad37e2 to 000ceb6 Compare January 28, 2019 07:54

eloyekunle force-pushed the lint/staging/src/k8s.io/kube-aggregator branch 2 times, most recently from e64fd8e to b92dba9 Compare January 28, 2019 11:14

eloyekunle force-pushed the lint/staging/src/k8s.io/kube-aggregator branch from f655570 to 342dfc3 Compare February 7, 2019 10:57

sttts reviewed Feb 7, 2019

View reviewed changes

eloyekunle force-pushed the lint/staging/src/k8s.io/kube-aggregator branch from 342dfc3 to 833a300 Compare February 7, 2019 12:58

rajathagasthya reviewed Feb 7, 2019

View reviewed changes

eloyekunle force-pushed the lint/staging/src/k8s.io/kube-aggregator branch 2 times, most recently from 7cddc77 to c174300 Compare February 8, 2019 09:53

liggitt reviewed Feb 8, 2019

View reviewed changes

sttts mentioned this pull request Feb 11, 2019

semantic-deep-equal: handle unexported symbols via pointer funcs kubernetes/utils#81

Closed

fixed golint errors in staging/src/k8s.io/kube-aggregator

6e9fc74

eloyekunle force-pushed the lint/staging/src/k8s.io/kube-aggregator branch from c174300 to 6e9fc74 Compare February 11, 2019 20:15

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 11, 2019

eloyekunle added 2 commits February 11, 2019 21:18

update openapi

d8dcc1a

unexported autoRegisterController

f83474d

liggitt added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 11, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2019

k8s-ci-robot merged commit 805a9e7 into kubernetes:master Feb 11, 2019

eloyekunle deleted the lint/staging/src/k8s.io/kube-aggregator branch February 11, 2019 22:31

eloyekunle mentioned this pull request Sep 4, 2019

REQUEST: New membership for eloyekunle kubernetes/org#1157

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes Golint Errors: staging/src/k8s.io/kube-aggregator #73369

Fixes Golint Errors: staging/src/k8s.io/kube-aggregator #73369

eloyekunle commented Jan 27, 2019

k8s-ci-robot commented Jan 27, 2019

eloyekunle commented Jan 27, 2019

neolit123 left a comment

eloyekunle commented Jan 27, 2019

neolit123 commented Jan 27, 2019

fejta commented Jan 28, 2019

eloyekunle commented Jan 28, 2019

fejta commented Jan 28, 2019

eloyekunle commented Jan 30, 2019

sttts Feb 7, 2019

eloyekunle Feb 7, 2019 •

edited

sttts Feb 7, 2019

sttts commented Feb 7, 2019

eloyekunle commented Feb 7, 2019 •

edited

rajathagasthya Feb 7, 2019

eloyekunle Feb 8, 2019

liggitt Feb 8, 2019

eloyekunle Feb 9, 2019 •

edited

sttts Feb 11, 2019

liggitt Feb 11, 2019

fejta Feb 11, 2019

liggitt Feb 11, 2019

fejta Feb 11, 2019

liggitt Feb 11, 2019

eloyekunle Feb 11, 2019

liggitt commented Feb 8, 2019

sttts commented Feb 11, 2019

liggitt commented Feb 11, 2019

k8s-ci-robot commented Feb 11, 2019

eloyekunle commented Feb 11, 2019

s-chand commented Feb 12, 2019

Fixes Golint Errors: staging/src/k8s.io/kube-aggregator #73369

Fixes Golint Errors: staging/src/k8s.io/kube-aggregator #73369

Conversation

eloyekunle commented Jan 27, 2019

k8s-ci-robot commented Jan 27, 2019

eloyekunle commented Jan 27, 2019

neolit123 left a comment

Choose a reason for hiding this comment

eloyekunle commented Jan 27, 2019

neolit123 commented Jan 27, 2019

fejta commented Jan 28, 2019

eloyekunle commented Jan 28, 2019

fejta commented Jan 28, 2019

eloyekunle commented Jan 30, 2019

Choose a reason for hiding this comment

eloyekunle Feb 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Feb 7, 2019

eloyekunle commented Feb 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eloyekunle Feb 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Feb 8, 2019

sttts commented Feb 11, 2019

liggitt commented Feb 11, 2019

k8s-ci-robot commented Feb 11, 2019

eloyekunle commented Feb 11, 2019

s-chand commented Feb 12, 2019

eloyekunle Feb 7, 2019 •

edited

eloyekunle commented Feb 7, 2019 •

edited

eloyekunle Feb 9, 2019 •

edited