-
Notifications
You must be signed in to change notification settings - Fork 500
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
Add checksum validation for the golangci-lint install script. #2378
Conversation
/cc @justaugustus @saschagrunert because they last touched this file. |
03fb7e6
to
8279ea0
Compare
128563c
to
67f6083
Compare
@cpanato please take another looks! Thanks! |
@cpanato - Please take another look. I am adding checksum for the installation script so that we can verify that it is what we intended to download. |
@cpanato - Could please take another look. I addressed all the comments. TIA! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for working on this i'm fine which those changes.
maybe just split the lint fixes in another commit to make it more clear
/assign @puerco @saschagrunert for thoughts as well
@puerco and @saschagrunert - Please take a look, thanks! |
Gentle ping! |
@saschagrunert - please take a look. I noticed that you recently updated this file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @vinayakankugoyal !
In the future, please try to scope your commits each for one change so that the PR is easier to review. The lint changes and the install script were mixed in one commit and it was difficult to understand at a glance.
Could you strip the lint warning changes (I think they are gone now), rebase and address the comments below?
URL_BASE=https://raw.githubusercontent.com/golangci/golangci-lint | ||
URL=$URL_BASE/$VERSION/install.sh | ||
INSTALL_CHECKSUM=294771225087ee48c8e0a45a99ac82ed8f9c6e9d384e692ab201986479c8594f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expanding a little on @cpanato's comment, could you add a comment so that anyone bumping the golangci-lint version can know how to obtain the install script sha?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
hack/verify-golangci-lint.sh
Outdated
$INSTALL_SCRIPT $VERSION | ||
PATH=$PATH:bin | ||
else | ||
echo 'ERROR: failed to install golangci-lint' >&2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's output here the actual reason of why the install failed:
echo 'ERROR: failed to install golangci-lint' >&2 | |
echo 'ERROR: install script sha256 checksum invalid' >&2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@vinayakankugoyal sorry for the late reply, can you please rebase this PR and address @puerco's comments? |
/test pull-release-integration-test |
@puerco - Thanks for taking a look. I address your comments and after rebase the lint changes were not needed because someone else already bumped the version to 1.43.0 and fixed all the linter callouts. PTAL. TIA! |
Co-authored-by: Sascha Grunert <sgrunert@redhat.com>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: saschagrunert, vinayakankugoyal 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 |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
We use curl to download the golangci-lint binary but don't verify the checksum of the installation script. I added checksum validation for the installation script.
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
Does this PR introduce a user-facing change?