Enable golinting for scheduler packages. #58437

tossmilestone · 2018-01-18T06:18:45Z

What this PR does / why we need it:
Enable golinting for scheduler packages

Which issue(s) this PR fixes:
Fixes #58234

Special notes for your reviewer:

pkg/scheduler/api and pkg/scheduler/api/v1 are not removed from hack/.golint_failures, because there are auto-generated go files by deepcopy-gen in the package, which have golint errors and are not suggested manually edited.
Please help to refine the comments if there are error or inaccurate descriptions. Thanks!

Release note:

Enable golint for `pkg/scheduler` and fix the golint errors in it.

resouer · 2018-01-18T07:46:53Z

/ok-to-test

resouer · 2018-01-18T07:52:00Z

@tossmilestone Could you rebase your commits into two?

Commit 1. Enable golint for pkg/scheduler
Commit 2. Fix golint errors in pkg/scheduler based on golint check

tossmilestone · 2018-01-18T08:21:37Z

@resouer updated, PTAL!
/cc @k82cn @ravisantoshgudimetla

ravisantoshgudimetla · 2018-01-20T02:12:41Z

pkg/kubelet/client/kubelet_client_test.go

@@ -53,9 +53,9 @@ func TestMakeTransportValid(t *testing.T) {
 		EnableHttps: true,
 		TLSClientConfig: restclient.TLSClientConfig{
 			CertFile: "../../client/testdata/mycertvalid.cer",
-			// TLS Configuration, only applies if EnableHttps is true.
+			// TLS Configuration, only applies if EnableHTTPs is true.


EnableHttps is the flag here at 2751ccf#diff-00db4807093c02bb99fd7c3c4d9f5bc5L53, so need for this change I think.

Yes, it should be not changed.

ravisantoshgudimetla · 2018-01-20T02:18:31Z

pkg/scheduler/algorithm/well_known_labels.go

-	// TaintNodeNotReady would be automatically added by node controller
-	// when node is not ready, and removed when node becomes ready.
+	// TaintNodeNotReady would be automatically added by node controller,
+	// when feature-gate for TaintBasedEvictions=true flag is enabled


Probably TaintBasedEvictions flag is enabled

ravisantoshgudimetla · 2018-01-20T02:20:46Z

pkg/scheduler/algorithm/well_known_labels.go

 	// TaintNodeUnreachable would be automatically added by node controller
-	// when node becomes unreachable (corresponding to NodeReady status ConditionUnknown)
+	// when feature-gate for TaintBasedEvictions=true flag is enabled and


I think for sake of consistency we can start with TaintNodeUnreachable will be added when node becomes..

ravisantoshgudimetla · 2018-01-20T02:25:29Z

pkg/scheduler/testing/fake_lister.go

@@ -25,11 +25,11 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	corelisters "k8s.io/client-go/listers/core/v1"
-	. "k8s.io/kubernetes/pkg/scheduler/algorithm"
+	algo "k8s.io/kubernetes/pkg/scheduler/algorithm"


nit: Probably algorithm may be a better name.

ravisantoshgudimetla · 2018-01-20T02:28:36Z

Thanks @tossmilestone for working on this!! Just some minor nits, rest look good to me. I will let @k82cn and @resouer to provide their feedback before merge.

tossmilestone · 2018-01-20T04:38:24Z

@ravisantoshgudimetla Thanks a lot for your review on so many changes! I will fix the errors soon.
Updated, PTAL!

tossmilestone · 2018-01-22T02:23:32Z

/test pull-kubernetes-e2e-kops-aws

k82cn · 2018-01-22T02:47:55Z

pkg/scheduler/core/generic_scheduler.go

@@ -451,7 +456,7 @@ func podFitsOnNode(
 		// TODO(bsalamat): consider using eCache and adding proper eCache invalidations
 		// when pods are nominated or their nominations change.
 		eCacheAvailable = eCacheAvailable && !podsAdded
-		for _, predicateKey := range predicates.PredicatesOrdering() {
+		for _, predicateKey := range predicates.Ordering() {


Why change this?

Because PredicatesOrdering caused the error:
pkg/scheduler/algorithm/predicates/predicates.go:152:6: func name will be used as predicates.PredicatesOrdering by other packages, and that stutters; consider calling this Ordering

Do we touch any code about PredicatesOrdering ? Not sure why it works before but failed in this PR :(.

Found the code about renaming.

ravisantoshgudimetla · 2018-01-22T23:30:53Z

/lgtm

/assign @k82cn

tossmilestone · 2018-02-03T08:06:41Z

/unassign @pwittrock

tossmilestone · 2018-02-03T08:07:23Z

/assign @liggitt

tossmilestone · 2018-02-08T02:55:24Z

@liggitt please help to review the PR if you are free, thanks! 😄

liggitt · 2018-02-08T04:45:15Z

looks like the only thing that still requires approval is the hack dir change enabling linting on those folders. that looks fine, but the PR needs a squash before merge (still has several merge commits present)

tossmilestone · 2018-02-08T07:01:24Z

@liggitt commits have been squashed, please take a look 😄

tossmilestone · 2018-02-08T08:14:51Z

/retest

liggitt · 2018-02-09T01:07:15Z

/approve

tossmilestone · 2018-02-09T03:12:26Z

@ravisantoshgudimetla needs your lgtm to merge the PR 😄

ravisantoshgudimetla · 2018-02-09T03:14:40Z

/lgtm

Thanks again @tossmilestone for working on this issue.

k8s-ci-robot · 2018-02-09T03:14:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: k82cn, liggitt, ravisantoshgudimetla, tossmilestone

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 OWNERS Files:

~~hack/OWNERS~~ [liggitt]
~~pkg/scheduler/OWNERS~~ [k82cn]
~~test/integration/scheduler/OWNERS~~ [k82cn,liggitt]

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

k8s-github-robot · 2018-02-09T04:24:27Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-09T05:02:24Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-github-robot assigned resouer and davidopp Jan 18, 2018

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 18, 2018

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

tossmilestone force-pushed the scheduler-golint branch from d8df122 to 1df1402 Compare January 18, 2018 08:58

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jan 18, 2018

tossmilestone force-pushed the scheduler-golint branch 2 times, most recently from ab7065f to 2751ccf Compare January 19, 2018 06:20

ravisantoshgudimetla reviewed Jan 20, 2018

View reviewed changes

k82cn reviewed Jan 22, 2018

View reviewed changes

k8s-ci-robot assigned k82cn and ravisantoshgudimetla and unassigned k82cn Jan 22, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 22, 2018

tossmilestone force-pushed the scheduler-golint branch 2 times, most recently from ffa8882 to 72e0be5 Compare January 26, 2018 09:44

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 27, 2018

tossmilestone force-pushed the scheduler-golint branch from 745d3aa to 3a1c9e3 Compare January 28, 2018 08:04

k8s-ci-robot unassigned pwittrock Feb 3, 2018

k8s-ci-robot assigned liggitt Feb 3, 2018

tossmilestone force-pushed the scheduler-golint branch 2 times, most recently from 00cf4e3 to 86effd7 Compare February 3, 2018 08:37

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 7, 2018

Enable golint for pkg/scheduler

a4e49d1

tossmilestone force-pushed the scheduler-golint branch from 050fdbf to 5ba0442 Compare February 8, 2018 06:46

Fix golint errors in pkg/scheduler based on golint check

3fdacfe

tossmilestone force-pushed the scheduler-golint branch from 5ba0442 to 3fdacfe Compare February 8, 2018 07:26

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

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

k8s-github-robot merged commit c179b8a into kubernetes:master Feb 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable golinting for scheduler packages. #58437

Enable golinting for scheduler packages. #58437

tossmilestone commented Jan 18, 2018 •

edited

Loading

resouer commented Jan 18, 2018

resouer commented Jan 18, 2018 •

edited

Loading

tossmilestone commented Jan 18, 2018 •

edited

Loading

ravisantoshgudimetla Jan 20, 2018

tossmilestone Jan 20, 2018 •

edited

Loading

ravisantoshgudimetla Jan 20, 2018

ravisantoshgudimetla Jan 20, 2018

ravisantoshgudimetla Jan 20, 2018

ravisantoshgudimetla commented Jan 20, 2018

tossmilestone commented Jan 20, 2018 •

edited

Loading

tossmilestone commented Jan 22, 2018 •

edited

Loading

k82cn Jan 22, 2018

tossmilestone Jan 22, 2018

k82cn Jan 23, 2018

k82cn Jan 23, 2018

ravisantoshgudimetla commented Jan 22, 2018

tossmilestone commented Feb 3, 2018

tossmilestone commented Feb 3, 2018

tossmilestone commented Feb 8, 2018

liggitt commented Feb 8, 2018

tossmilestone commented Feb 8, 2018

tossmilestone commented Feb 8, 2018

liggitt commented Feb 9, 2018

tossmilestone commented Feb 9, 2018

ravisantoshgudimetla commented Feb 9, 2018

k8s-ci-robot commented Feb 9, 2018

k8s-github-robot commented Feb 9, 2018

k8s-github-robot commented Feb 9, 2018

Enable golinting for scheduler packages. #58437

Enable golinting for scheduler packages. #58437

Conversation

tossmilestone commented Jan 18, 2018 • edited Loading

resouer commented Jan 18, 2018

resouer commented Jan 18, 2018 • edited Loading

tossmilestone commented Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

tossmilestone Jan 20, 2018 • edited Loading

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

ravisantoshgudimetla commented Jan 20, 2018

tossmilestone commented Jan 20, 2018 • edited Loading

tossmilestone commented Jan 22, 2018 • edited Loading

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

ravisantoshgudimetla commented Jan 22, 2018

tossmilestone commented Feb 3, 2018

tossmilestone commented Feb 3, 2018

tossmilestone commented Feb 8, 2018

liggitt commented Feb 8, 2018

tossmilestone commented Feb 8, 2018

tossmilestone commented Feb 8, 2018

liggitt commented Feb 9, 2018

tossmilestone commented Feb 9, 2018

ravisantoshgudimetla commented Feb 9, 2018

k8s-ci-robot commented Feb 9, 2018

k8s-github-robot commented Feb 9, 2018

k8s-github-robot commented Feb 9, 2018

tossmilestone commented Jan 18, 2018 •

edited

Loading

resouer commented Jan 18, 2018 •

edited

Loading

tossmilestone commented Jan 18, 2018 •

edited

Loading

tossmilestone Jan 20, 2018 •

edited

Loading

tossmilestone commented Jan 20, 2018 •

edited

Loading

tossmilestone commented Jan 22, 2018 •

edited

Loading