Skip to content

Fix linter errors.#1883

Merged
JimmyCYJ merged 8 commits intoistio:masterfrom
JimmyCYJ:fix-linter-errors-2
Nov 30, 2017
Merged

Fix linter errors.#1883
JimmyCYJ merged 8 commits intoistio:masterfrom
JimmyCYJ:fix-linter-errors-2

Conversation

@JimmyCYJ
Copy link
Copy Markdown
Member

What this PR does / why we need it:

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
Copy link
Copy Markdown

@JimmyCYJ PR needs rebase

@istio-merge-robot istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 28, 2017
@istio-merge-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: linsun

Assign the PR to them by writing /assign @linsun 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:

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

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 28, 2017

Codecov Report

Merging #1883 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1883      +/-   ##
==========================================
- Coverage   81.17%   81.17%   -0.01%     
==========================================
  Files         191      190       -1     
  Lines       19344    19319      -25     
==========================================
- Hits        15703    15682      -21     
+ Misses       3183     3179       -4     
  Partials      458      458
Flag Coverage Δ
#broker 45.51% <ø> (ø) ⬆️
#mixer 82.5% <ø> (ø) ⬆️
#pilot 80.35% <ø> (-0.03%) ⬇️
#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%> (ø) ⬆️
broker/pkg/model/config/mock_store.go 18.57% <0%> (ø) ⬆️
pilot/tools/version/version.go
mixer/adapter/kubernetes/cache.go 68.46% <0%> (+0.76%) ⬆️
pilot/platform/consul/monitor.go 79.76% <0%> (+3.57%) ⬆️

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 f738d85...ffe2211. Read the comment docs.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 28, 2017

Codecov Report

Merging #1883 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1883      +/-   ##
==========================================
- Coverage   81.19%   81.17%   -0.03%     
==========================================
  Files         195      186       -9     
  Lines       19633    18900     -733     
==========================================
- Hits        15941    15342     -599     
+ Misses       3217     3125      -92     
+ Partials      475      433      -42
Flag Coverage Δ
#broker 44.44% <ø> (-1.07%) ⬇️
#mixer 82.42% <ø> (-0.02%) ⬇️
#pilot 80.42% <ø> (-0.09%) ⬇️
#security 90.39% <100%> (+3.73%) ⬆️
Impacted Files Coverage Δ
security/cmd/node_agent/na/nafactory.go 88% <100%> (ø) ⬆️
security/cmd/node_agent/na/nodeagent.go 88.67% <100%> (ø) ⬆️
mixer/adapter/prometheus/server.go 89.83% <0%> (-5.09%) ⬇️
pilot/adapter/config/crd/controller.go 77.85% <0%> (-2.02%) ⬇️
security/pkg/platform/client.go
broker/pkg/version/version.go
security/pkg/platform/gcp.go
pilot/platform/kube/inject/inject.go
pilot/platform/kube/inject/configmap.go
pilot/platform/kube/inject/http.go
... and 5 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 4a615e5...a7abb98. Read the comment docs.

@JimmyCYJ
Copy link
Copy Markdown
Member Author

/retest

@istio-merge-robot istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 28, 2017
@JimmyCYJ
Copy link
Copy Markdown
Member Author

/test istio-presubmit

@JimmyCYJ
Copy link
Copy Markdown
Member Author

This PR addressed the same issue as #1841, and #1841 is closed due to incorrect set up of my github repository. Please take a look and apologize for the inconvenience.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's reason for renaming err to errors? I believe err is more widely used.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, errors is not accurate, it should be error, but since error can stand for a lib, usually people use err for golang.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comments. I have changed errors back to err.
Linter complains that at line 167 and line 173, declaration of "err" shadows declaration at security/integration/main.go:146 (vetshadow), so I rename these two err to delErr and creErr respectively.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because you are trying to declare an already existed var.

For declaration, you should use
err := ***

And after the err is declared, you should use

err = *** (no ':' any more)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this frustrate the linter since returned error is unchecked?

Copy link
Copy Markdown

@myidpt myidpt Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check this error? Done. Thanks.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out. Added a check for this error.

Copy link
Copy Markdown

@myidpt myidpt Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check this error? Done. Thanks.

@istio-merge-robot istio-merge-robot added do-not-merge/post-submit needs-rebase Indicates a PR needs to be rebased before being merged and removed do-not-merge/post-submit labels Nov 29, 2017
@istio-merge-robot
Copy link
Copy Markdown

@JimmyCYJ PR needs rebase

Copy link
Copy Markdown
Contributor

@wattli wattli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:= is used to declare and assign var, after it's declared, use '=' to avoid re-declaration.

Copy link
Copy Markdown
Contributor

@wattli wattli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to submit after you fix that, and thanks for working on it :)

@istio-merge-robot istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 29, 2017
@JimmyCYJ
Copy link
Copy Markdown
Member Author

/test istio-presubmit

@JimmyCYJ
Copy link
Copy Markdown
Member Author

/test istio-presubmit

@JimmyCYJ JimmyCYJ merged commit 15c79c9 into istio:master Nov 30, 2017
@JimmyCYJ JimmyCYJ deleted the fix-linter-errors-2 branch November 30, 2017 00:03
kyessenov pushed a commit to kyessenov/istio that referenced this pull request Aug 13, 2018
* Reduce log level for jwt filter (istio#1866)

* Update_Dependencies (istio#1873)

* Correctly clean up headers used for payload from JWT authentication (istio#1879)

* Correctly clean up headers used for payload from JWT authentication

* Clang

* Update_Dependencies (istio#1883)

* destination.principal derivation fix (istio#1884)

* fix attribute extraction

Signed-off-by: Kuat Yessenov <kuat@google.com>

* seed mock

Signed-off-by: Kuat Yessenov <kuat@google.com>

* merge 1.0 to master

* Update API SHA (istio#1891)

* add needed dependencies for circle ci
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Istio-security linter errors

8 participants