Fix linter errors 2 by JimmyCYJ · Pull Request #1841 · istio/istio

JimmyCYJ · 2017-11-23T01:03:57Z

What this PR does / why we need it: Fix linter errors.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #1446

Special notes for your reviewer:
The following linter errors are reported by linters.sh, and are fixed in this PR.
security/integration/main.go:151:13:warning: "Succesfully" is a misspelling of "Successfully" (misspell)
security/integration/main.go:368::error: unreachable code (vet)
security/cmd/istio_ca/main.go:171::warning: declaration of "ca" shadows declaration at security/cmd/istio_ca/main.go:32 (vetshadow)
security/cmd/istio_ca/main.go:191::warning: declaration of "ca" shadows declaration at security/cmd/istio_ca/main.go:32 (vetshadow)
security/integration/main.go:155::warning: declaration of "err" shadows declaration at security/integration/main.go:146 (vetshadow)
security/integration/main.go:167::warning: declaration of "err" shadows declaration at security/integration/main.go:146 (vetshadow)
security/integration/main.go:174::warning: declaration of "err" shadows declaration at security/integration/main.go:146 (vetshadow)
security/integration/utils/kubernetes.go:111::warning: declaration of "uuid" shadows declaration at security/integration/utils/kubernetes.go:26 (vetshadow)
security/integration/utils/kubernetes.go:146:2:warning: ineffectual assignment to pod (ineffassign)
security/pkg/platform/aws_test.go:168:20:warning: ineffectual assignment to err (ineffassign)
security/cmd/node_agent/na/nafactory.go:35:26:warning: error strings should not be capitalized or end with punctuation or a newline (golint)
security/cmd/node_agent/na/nodeagent.go:44:26:warning: error strings should not be capitalized or end with punctuation or a newline (golint)
security/cmd/node_agent/na/nodeagent.go:52:26:warning: error strings should not be capitalized or end with punctuation or a newline (golint)
security/integration/main.go:66:2:warning: don't use underscores in Go names; struct field org_root_cert should be orgRootCert (golint)
security/integration/main.go:67:2:warning: don't use underscores in Go names; struct field org_cert_chain should be orgCertChain (golint)
security/integration/main.go:146:2:warning: don't use underscores in Go names; var self_signed_ca_pod should be selfSignedCaPod (golint)
security/integration/main.go:158:9:warning: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (golint)
security/integration/main.go:169:9:warning: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (golint)
security/integration/main.go:177:9:warning: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (golint)
security/integration/main.go:192:2:warning: don't use underscores in Go names; var ca_pod should be caPod (golint)
security/integration/main.go:200:2:warning: don't use underscores in Go names; var ca_service should be caService (golint)
security/integration/main.go:207:2:warning: don't use underscores in Go names; var na_pod should be naPod (golint)
security/integration/main.go:215:2:warning: don't use underscores in Go names; var na_service should be naService (golint)
security/integration/main.go:231:9:warning: if block ends with a return statement, so drop this else and outdent its block (golint)
security/integration/main.go:269:21:warning: error strings should not be capitalized or end with punctuation or a newline (golint)
security/integration/main.go:288:21:warning: error strings should not be capitalized or end with punctuation or a newline (golint)
security/integration/main.go:302:6:warning: func readUri should be readURI (golint)
security/integration/main.go:318:2:warning: don't use underscores in Go names; var max_retry should be maxRetry (golint)
security/integration/main.go:321:2:warning: don't use underscores in Go names; var org_root_cert should be orgRootCert (golint)
security/integration/main.go:323:21:warning: error strings should not be capitalized or end with punctuation or a newline (golint)
security/integration/main.go:326:2:warning: don't use underscores in Go names; var org_cert_chain should be orgCertChain (golint)
security/integration/main.go:328:21:warning: error strings should not be capitalized or end with punctuation or a newline (golint)
security/integration/main.go:351:22:warning: error strings should not be capitalized or end with punctuation or a newline (golint)
security/integration/main.go:387:20:warning: error strings should not be capitalized or end with punctuation or a newline (golint)
security/integration/utils/kubernetes.go:32:1:warning: exported function CreateClientset should have comment or be unexported (golint)
security/integration/utils/kubernetes.go:47:1:warning: exported function CreateTestNamespace should have comment or be unexported (golint)
security/integration/utils/kubernetes.go:63:1:warning: exported function DeleteTestNamespace should have comment or be unexported (golint)
security/integration/utils/kubernetes.go:71:1:warning: exported function CreateService should have comment or be unexported (golint)
security/integration/utils/kubernetes.go:106:1:warning: exported function DeleteService should have comment or be unexported (golint)
security/integration/utils/kubernetes.go:110:1:warning: exported function CreatePod should have comment or be unexported (golint)
security/integration/utils/kubernetes.go:158:1:warning: exported function DeletePod should have comment or be unexported (golint)
security/integration/utils/kubernetes.go:162:1:warning: exported function CreateRole should have comment or be unexported (golint)
security/integration/utils/kubernetes.go:190:1:warning: exported function CreateRoleBinding should have comment or be unexported (golint)
security/integration/utils/kubernetes.go:275:1:warning: exported function WaitForSecretExist should have comment or be unexported (golint)
security/pkg/credential/token.go:45:1:warning: comment on exported method GcpTokenFetcher.FetchServiceAccount should be of the form "FetchServiceAccount ..." (golint)
security/pkg/server/grpc/authorizer.go:27:6:warning: type sameIdAuthorizer should be sameIDAuthorizer (golint)
security/pkg/server/grpc/authorizer.go:44:22:warning: error strings should not be capitalized or end with punctuation or a newline (golint)

Release note:

istio-merge-robot · 2017-11-23T01:04:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approvers: sebastienvas, wenchenglu

Assign the PR to them by writing /assign @sebastienvas @wenchenglu in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

bin/OWNERS
security/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

codecov · 2017-11-23T01:17:09Z

Codecov Report

Merging #1841 into master will increase coverage by 0.21%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1841      +/-   ##
==========================================
+ Coverage   81.68%   81.89%   +0.21%     
==========================================
  Files         135      189      +54     
  Lines       11537    18721    +7184     
==========================================
+ Hits         9424    15332    +5908     
- Misses       1920     2961    +1041     
- Partials      193      428     +235

Flag	Coverage Δ
#broker	`45.51% <ø> (ø)`	⬆️
#mixer	`82.49% <ø> (ø)`	⬆️
#pilot	`82.23% <ø> (?)`
#security	`90.39% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
security/cmd/node_agent/na/nodeagent.go	`88.67% <100%> (ø)`	⬆️
security/cmd/node_agent/na/nafactory.go	`88% <100%> (ø)`	⬆️
pilot/proxy/envoy/resources.go	`84.61% <0%> (ø)`
pilot/adapter/config/crd/types.go	`62.26% <0%> (ø)`
pilot/adapter/config/ingress/controller.go	`91.5% <0%> (ø)`
pilot/proxy/envoy/discovery.go	`75.51% <0%> (ø)`
pilot/adapter/config/file/monitor.go	`78.46% <0%> (ø)`
pilot/adapter/config/ingress/conversion.go	`100% <0%> (ø)`
pilot/platform/consul/conversion.go	`87.03% <0%> (ø)`
...ot/adapter/serviceregistry/aggregate/controller.go	`75% <0%> (ø)`
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1903f78...6818cd7. Read the comment docs.

myidpt

Thanks, just some nit comments.

myidpt · 2017-11-24T22:11:57Z

security/integration/utils/kubernetes.go

 	return clientset.CoreV1().Services(namespace).Get(name, metav1.GetOptions{})
 }

+// DeleteService takes identity information of a service and deletes the service.


Remove "takes identity information of a service"?

Done. Thanks!

myidpt · 2017-11-24T22:13:16Z

security/integration/utils/kubernetes.go

 	return clientset.CoreV1().Pods(namespace).Get(name, metav1.GetOptions{})
 }

+// DeletePod takes ideneity information of a pod and deletes the pod.


Done. Thanks!

myidpt · 2017-11-24T22:14:41Z

security/integration/utils/kubernetes.go

 }

+// CreateRole creates a role object named "istio-ca-role".
 func CreateRole(clientset kubernetes.Interface, namespace string) error {


Rename this function to "CreateIstioCARole" to be precise?

Done. Thanks!

myidpt · 2017-11-24T22:15:49Z

security/integration/utils/kubernetes.go

 }

+// CreateRoleBinding binds role "istio-ca-role" to default service account.
 func CreateRoleBinding(clientset kubernetes.Interface, namespace string) error {


createIstioCARoleBinding?

Done. Thanks!

jmuk · 2017-11-27T18:17:28Z

Ah, sorry not noticing you, but can you also update lintconfig_base.json and lintconfig.json in the top directory? The excludes section has "^security" line, which suppresses all of lint warnings from security directory.

Once that's done, linters (and presubmit) will now fail when new lint warnings come in, so we can keep the code health.

Thanks for the comments. These two "^security" lines are removed.

wattli · 2017-11-27T22:23:31Z

security/integration/main.go

-	flags.StringVar(&opts.org_root_cert, "root-cert", "", "Path to the original root ceritificate")
-	flags.StringVar(&opts.org_cert_chain, "cert-chain", "", "Path to the original certificate chain")
+	flags.StringVar(&opts.orgRootCert, "root-cert", "", "Path to the original root ceritificate")
+	flags.StringVar(&opts.orgCertChain, "cert-chain", "", "Path to the original certificate chain")


Since you are already here, can you change the description to something like 'Path to the original workload certificate chain'

Sure. Done.

wattli · 2017-11-27T22:25:32Z

security/integration/main.go

-
-		expectedID := fmt.Sprintf("spiffe://cluster.local/ns/%s/sa/default", s.GetNamespace())
-		examineSecret(s, expectedID)
+	secret, error := utils.WaitForSecretExist(opts.clientset, opts.namespace, "istio.default",


I think 'err' is widely used since 'error' can be confused with some lib name. At least please be consistent within this file (there are some 'err' and some 'error').

I replaced err with errors in this file.

JimmyCYJ · 2017-11-28T02:01:18Z

I didn't clone the istio code from my forked repository correctly. I am going to close this pull request and create a new one.

istio-testing added the release-note label Nov 23, 2017

googlebot added the cla: yes label Nov 23, 2017

yutongz added do-not-merge/post-submit labels Nov 23, 2017

JimmyCYJ requested review from myidpt, sebastienvas and wattli November 23, 2017 05:46

yutongz added do-not-merge/post-submit and removed do-not-merge/post-submit labels Nov 23, 2017

myidpt reviewed Nov 24, 2017

View reviewed changes

JimmyCYJ added 4 commits November 27, 2017 11:12

Fixed linter errors

7d897d4

Fixed linter errors

3e29feb

Fixed linter errors

192d80d

Addressed some review comments.

6818cd7

JimmyCYJ force-pushed the fix-linter-errors-2 branch from 27179d4 to 6818cd7 Compare November 27, 2017 20:15

wattli reviewed Nov 27, 2017

View reviewed changes

JimmyCYJ closed this Nov 28, 2017

JimmyCYJ deleted the fix-linter-errors-2 branch November 28, 2017 02:11

JimmyCYJ mentioned this pull request Nov 28, 2017

Fix linter errors. #1883

Merged

Conversation

JimmyCYJ commented Nov 23, 2017

Uh oh!

istio-merge-robot commented Nov 23, 2017

Uh oh!

codecov bot commented Nov 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

myidpt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmuk commented Nov 27, 2017 • edited by JimmyCYJ Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JimmyCYJ commented Nov 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecov bot commented Nov 23, 2017 •

edited

Loading

jmuk commented Nov 27, 2017 •

edited by JimmyCYJ

Loading