Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build golangci-lint via go rather than curl | bash #396

Merged
merged 1 commit into from
Dec 22, 2021

Conversation

mattcary
Copy link
Contributor

What type of PR is this?

/kind cleanup

What this PR does / why we need it:
Remove use of curl'ing an arbitrary internet file through bash, which is now considered a security issue. Instead just use go build.

I created this by go mod download'ing the golangci, then go installing golangci-lint, which produced a lot of errors asking to go get certain dependencies. I did that by hand, go mod tidy'd, and everything seems to work.

Release note:

none

/assign @jingxu97
/assign @lizhuqi

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Dec 22, 2021
@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 22, 2021
if [[ -z "$(command -v golangci-lint)" ]]; then
echo "Cannot find golangci-lint. Installing golangci-lint..."
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.31.0
Copy link
Member

Choose a reason for hiding this comment

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

that's only for testing, is there still security issue here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our vulnerability scanners don't distinguish for what the script is used for, only that it's in the repo. Even if it's in test it's still running in CI jobs etc.

Copy link
Member

Choose a reason for hiding this comment

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

which vulnerability scanner are you referring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Internal google ones.

Copy link
Member

Choose a reason for hiding this comment

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

that's hard way not using curl in testing sometimes, I think it's better exclude test folder if that's configurable?
actually this hack/verify-golint.sh could be deleted since there is already a github action doing same check:

- name: Run linter
uses: golangci/golangci-lint-action@v2
with:
version: v1.29
args: -E=gofmt,golint,misspell --timeout=30m0s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleting this sounds even better to me! I'll update the PR.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 22, 2021
@mattcary
Copy link
Contributor Author

Had to update golang.org/x/text to v0.3.7

@mattcary
Copy link
Contributor Author

/retest

@andyzhangx
Copy link
Member

Had to update golang.org/x/text to v0.3.7

@mattcary could you approve this PR? #395

@mattcary
Copy link
Contributor Author

Had to update golang.org/x/text to v0.3.7

@mattcary could you approve this PR? #395

done

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1609514834

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 86.181%

Totals Coverage Status
Change from base Build 1609463185: 0.0%
Covered Lines: 817
Relevant Lines: 948

💛 - Coveralls

Copy link
Member

@andyzhangx andyzhangx left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 22, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, mattcary

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 22, 2021
@k8s-ci-robot k8s-ci-robot merged commit 92fdc10 into kubernetes-csi:master Dec 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants