enable golint lll check #1305

camilamacedo86 · 2020-01-12T13:44:26Z

Description
Enable the lint check lll
(It will ensure the size of lines since is not a good practice be required to use the scroll to check the code.)

NOTE: issues fix as always that is possible or ignored.

camilamacedo86 · 2020-01-12T13:45:04Z

/assign @pmorie

camilamacedo86 · 2020-01-12T14:09:39Z

/assign @mengqiy

Adirio

I leave 30 comments but don't worry, most of them are duplicates, we can keep the discussion in the first appearance of each unless there is something specific to one of them.

cmd/init_project.go

cmd/main.go

cmd/init_project.go

cmd/main.go

test/e2e/utils/test_context.go

test/e2e/v1/e2e_suite.go

test/e2e/v2/e2e_suite.go

pkg/scaffold/util/util.go

estroz · 2020-01-13T19:33:25Z

pkg/scaffold/v1/webhook/webhook_test.go

@@ -39,39 +39,44 @@ var _ = Describe("Webhook", func() {
 	serverName := "default"
 	inputs := []*webhookTestcase{
 		{
-			Resource: resource.Resource{Group: "crew", Version: "v1", Kind: "FirstMate", Namespaced: true, CreateExampleReconcileBody: true},
+			Resource: resource.Resource{Group: "crew", Version: "v1", Kind: "FirstMate",


Better to put all Resource fields on their own line for consistency.

if we put all in one line it is long and will no pass in the lint.
It is harder understanding if we need to use the scroll.

I'd do this which IMO read better. I guess that's also what @estroz means.

Resource: resource.Resource{ Group: "crew", Version: "v1", Kind: "FirstMate", Namespaced: true, CreateExampleReconcileBody: true },

It makes sense. I did not understand that. Tks.

cmd/webhook_v1.go

mengqiy · 2020-01-16T21:53:48Z

cmd/webhook_v2.go

@@ -40,7 +40,8 @@ func newWebhookV2Cmd() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "webhook",
 		Short: "Scaffold a webhook for an API resource.",
-		Long:  `Scaffold a webhook for an API resource. You can choose to scaffold defaulting, validating and (or) conversion webhooks.`,
+		Long: `Scaffold a webhook for an API resource. You can choose to scaffold defaulting,
+		validating and (or) conversion webhooks.`,


This is not identical to what we used to have, since backquotes "`" are being used. You are introducing a newline here.

mengqiy · 2020-01-16T22:02:22Z

Need rebase

mengqiy · 2020-01-17T17:23:07Z

@camilamacedo86 Please rebase

estroz

/lgtm

mengqiy · 2020-01-17T18:02:47Z

pkg/scaffold/util/util.go

@@ -26,6 +26,7 @@ import (
 	"sigs.k8s.io/kubebuilder/pkg/scaffold/resource"
 )

+// nolint:lll


This can be fixed by either split the arguments across multiple lines or split the return values across multiple lines.

mengqiy · 2020-01-17T18:08:08Z

pkg/scaffold/v1/webhook/webhook_test.go

+				Version:    "v1",
+				Kind:       "FirstMate",
+				Namespaced: true,
+				CreateExampleReconcileBody: true},


IMO it's better to move the closed bracket } to its own line.

And add a comma at the end of CreateExampleReconcileBody: true,

pkg/scaffold/v2/metricsauth/auth_proxy_patch.go

Adirio · 2020-01-17T18:47:39Z

pkg/scaffold/util/util.go

@@ -26,6 +26,7 @@ import (
 	"sigs.k8s.io/kubebuilder/pkg/scaffold/resource"
 )

+// nolint:lll
 func GetResourceInfo(r *resource.Resource, repo, domain string, isMultiGroup bool) (resourcePackage, groupDomain string) {


Suggested change

func GetResourceInfo(r *resource.Resource, repo, domain string, isMultiGroup bool) (resourcePackage, groupDomain string) {

func GetResourceInfo(

r *resource.Resource,

repo,

domain string,

isMultiGroup bool,

) (resourcePackage, groupDomain string) {

I one of the 100 comments was requested to keep it. I agree with you 👍

I guess these 2 lines are causing the problem.

repo, domain string,

I guess you can change it to:

repo string, domain string,

Adirio · 2020-01-17T18:51:32Z

cmd/webhook_v2.go

-		Long:  `Scaffold a webhook for an API resource. You can choose to scaffold defaulting, validating and (or) conversion webhooks.`,
+		Long: `Scaffold a webhook for an API resource. You can choose to scaffold defaulting,` +
+			`validating and (or) conversion webhooks.`,


I think you are missing an space to indent this properly.

Also, I would use double quotes instead of back quotes if no special functionality given by backquote is needed.

And I also think you are missing an space after the comma

I think you are missing a space to indent this properly.

The gofmt check indentation issues.

Also, I would use double quotes instead of back quotes if no special functionality given by backquote is needed.

The long ones require ``

And I also think you are missing an space after the comma

👍 yes, this we can address :-)

Adirio · 2020-01-17T18:52:58Z

pkg/scaffold/api.go

+		api.Resource,
+		api.project.Repo,
+		api.project.Domain,
+		api.project.MultiGroup)


Suggested change

api.project.MultiGroup)

api.project.MultiGroup,

)

Adirio · 2020-01-17T18:54:15Z

test/e2e/v1/e2e_suite.go

@@ -108,7 +108,8 @@ var _ = Describe("kubebuilder", func() {

 			// NOTE: If you want to run the test against a GKE cluster, you will need to grant yourself permission.
 			// Otherwise, you may see "... is forbidden: attempt to grant extra privileges"
-			// $ kubectl create clusterrolebinding myname-cluster-admin-binding --clusterrole=cluster-admin --user=myname@mycompany.com
+			// $ kubectl create clusterrolebinding myname-cluster-admin-binding \
+			// --clusterrole=cluster-admin --user=myname@mycompany.com


Suggested change

// --clusterrole=cluster-admin --user=myname@mycompany.com

// --clusterrole=cluster-admin --user=myname@mycompany.com

Visual indent

1 of 100 comments requests to add \ and remove the spaces. :-)
I prefer this other suggestion either if you do not mind.

Did I request to remove the spaces? Well it seems I changed my mind sorry for that. The / is needed but thats already there.

Adirio · 2020-01-17T18:54:37Z

test/e2e/v2/e2e_suite.go

@@ -159,7 +159,8 @@ var _ = Describe("kubebuilder", func() {

 			// NOTE: If you want to run the test against a GKE cluster, you will need to grant yourself permission.
 			// Otherwise, you may see "... is forbidden: attempt to grant extra privileges"
-			// $ kubectl create clusterrolebinding myname-cluster-admin-binding --clusterrole=cluster-admin --user=myname@mycompany.com
+			// $ kubectl create clusterrolebinding myname-cluster-admin-binding \
+			// --clusterrole=cluster-admin --user=myname@mycompany.com


Suggested change

// --clusterrole=cluster-admin --user=myname@mycompany.com

// --clusterrole=cluster-admin --user=myname@mycompany.com

1 of 100 comments requests to add \ and remove the spaces. :-)
I prefer this other suggestion either, if you do not mind.

Again, sorry.

Now worry. Thank you a lot for your contributions btw.

Adirio · 2020-01-17T18:55:38Z

testdata/project-v2-multigroup/config/default/manager_auth_proxy_patch.yaml

-# This patch inject a sidecar container which is a HTTP proxy for the controller manager,
-# it performs RBAC authorization against the Kubernetes API using SubjectAccessReviews.
+# This patch inject a sidecar container which is a HTTP proxy for the 
+# controller manager, it performs RBAC authorization against the Kubernetes API using SubjectAccessReviews.


How can this pass the lll test if it the second line is longer after the change than the first line before the change?

This line is in the correct size.
The lint issue was in the line where the const is defined.

This sounds like a bug in lll linter...
I guess we can keep what we have now in the PR for now.

It is not. The GitHub diff is making harder you folks get what was a change here.
The change/fix is regarding the above line where the const is defined.

Adirio · 2020-01-17T18:55:53Z

testdata/project-v2/config/default/manager_auth_proxy_patch.yaml

-# This patch inject a sidecar container which is a HTTP proxy for the controller manager,
-# it performs RBAC authorization against the Kubernetes API using SubjectAccessReviews.
+# This patch inject a sidecar container which is a HTTP proxy for the 
+# controller manager, it performs RBAC authorization against the Kubernetes API using SubjectAccessReviews.


Same as above

This line is in the correct size.
The lint issue was in the line where the const is defined.

camilamacedo86 · 2020-01-17T22:55:31Z

Hi @mengqiy now I think we can move forward with this one 👍

mengqiy · 2020-01-18T00:13:35Z

/lgtm
/approve

k8s-ci-robot · 2020-01-18T00:13:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, mengqiy

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:

~~OWNERS~~ [mengqiy]

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 12, 2020

k8s-ci-robot requested review from DirectXMan12 and estroz January 12, 2020 13:44

k8s-ci-robot assigned pmorie Jan 12, 2020

k8s-ci-robot assigned mengqiy Jan 12, 2020

Adirio suggested changes Jan 13, 2020

View reviewed changes

camilamacedo86 force-pushed the lint-lll branch 4 times, most recently from 8fd555f to a850c22 Compare January 13, 2020 16:52

estroz reviewed Jan 13, 2020

View reviewed changes

pkg/scaffold/util/util.go Outdated Show resolved Hide resolved

estroz reviewed Jan 13, 2020

View reviewed changes

Adirio reviewed Jan 13, 2020

View reviewed changes

cmd/webhook_v1.go Outdated Show resolved Hide resolved

camilamacedo86 force-pushed the lint-lll branch from a850c22 to 5a80e39 Compare January 13, 2020 22:08

This comment has been minimized.

Sign in to view

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2020

Adirio approved these changes Jan 13, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

camilamacedo86 force-pushed the lint-lll branch from 5a80e39 to 4b78b0c Compare January 14, 2020 11:08

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2020

Adirio mentioned this pull request Jan 14, 2020

[Umbrella Issue] GolangCI lint #1309

Closed

41 tasks

camilamacedo86 requested a review from estroz January 15, 2020 11:41

mengqiy reviewed Jan 16, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2020

camilamacedo86 force-pushed the lint-lll branch from 4b78b0c to 3e52fa9 Compare January 16, 2020 22:53

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2020

Adirio mentioned this pull request Jan 17, 2020

Order linters #1323

Merged

camilamacedo86 force-pushed the lint-lll branch from badebf1 to 5f4c516 Compare January 17, 2020 08:57

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 17, 2020

camilamacedo86 force-pushed the lint-lll branch from 5f4c516 to 9032028 Compare January 17, 2020 17:35

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 17, 2020

estroz approved these changes Jan 17, 2020

View reviewed changes

k8s-ci-robot assigned estroz Jan 17, 2020

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

mengqiy reviewed Jan 17, 2020

View reviewed changes

pkg/scaffold/v2/metricsauth/auth_proxy_patch.go Show resolved Hide resolved

Adirio reviewed Jan 17, 2020

View reviewed changes

camilamacedo86 force-pushed the lint-lll branch from 9032028 to 4fa7843 Compare January 17, 2020 22:49

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 17, 2020

enable golint lll check

c52ba01

camilamacedo86 force-pushed the lint-lll branch from 4fa7843 to c52ba01 Compare January 17, 2020 22:54

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2020

k8s-ci-robot merged commit 8f8905a into kubernetes-sigs:master Jan 18, 2020

camilamacedo86 deleted the lint-lll branch January 18, 2020 00:18

	// --clusterrole=cluster-admin --user=myname@mycompany.com
	// --clusterrole=cluster-admin --user=myname@mycompany.com

enable golint lll check #1305

enable golint lll check #1305

Conversation

camilamacedo86 commented Jan 12, 2020

camilamacedo86 commented Jan 12, 2020

camilamacedo86 commented Jan 12, 2020

Adirio left a 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

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengqiy commented Jan 16, 2020

mengqiy commented Jan 17, 2020

estroz left a 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

camilamacedo86 Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adirio Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

camilamacedo86 Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Jan 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Jan 17, 2020 • 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

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

camilamacedo86 commented Jan 17, 2020

mengqiy commented Jan 18, 2020

k8s-ci-robot commented Jan 18, 2020

camilamacedo86 Jan 17, 2020 •

edited

Loading

Adirio Jan 17, 2020 •

edited

Loading

camilamacedo86 Jan 17, 2020 •

edited

Loading

camilamacedo86 Jan 17, 2020 •

edited

Loading

camilamacedo86 Jan 17, 2020 •

edited

Loading