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

Ensure golangci-lint for the correct OS #22

Merged
merged 3 commits into from
Sep 1, 2020

Conversation

arjunrn
Copy link
Contributor

@arjunrn arjunrn commented Sep 1, 2020

Attempts to fix #20

Currently the ensure.sh attempts to download the golangci-lint tool even when the tool is present on the path. The ensure.sh will now download the script only when the tool is not present in $PATH. Also other minor improvements:

  • Ensure that $GOPATH/bin is present in $PATH.
  • Allow user to specify the path for $GOLANGCI_LINT_CACHE

Signed-off-by: Arjun Naik <anaik@redhat.com>
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2020
@arjunrn
Copy link
Contributor Author

arjunrn commented Sep 1, 2020

/assign @dustman9000 @2uasimojo

fi
DOWNLOAD_URL="https://github.com/golangci/golangci-lint/releases/download/v${GOLANGCI_LINT_VERSION}/golangci-lint-${GOLANGCI_LINT_VERSION}-${GOOS}-amd64.tar.gz"
curl -sfL "${DOWNLOAD_URL}" | tar -C "${GOPATH}/bin" -zx --strip-components=1 "golangci-lint-${GOLANGCI_LINT_VERSION}-${GOOS}-amd64/golangci-lint"
Copy link
Member

@dustman9000 dustman9000 Sep 1, 2020

Choose a reason for hiding this comment

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

Ensure that GOOS is defined, default to go env GOOS if not.

Signed-off-by: Arjun Naik <anaik@redhat.com>
@dustman9000
Copy link
Member

dustman9000 commented Sep 1, 2020

I tested this by moving golangci-lint out of my $PATH and it's failing locally for me:

$ make
boilerplate/_lib/ensure.sh golangci-lint
# GOLANGCI_LINT_CACHE needs to be set to a directory which is writeable
# Relevant issue - https://github.com/golangci/golangci-lint/issues/734
GOLANGCI_LINT_CACHE=/tmp/golangci-cache golangci-lint run -c boilerplate/openshift/golang_osd_cluster_operator/golangci.yml ./...
bash: /Users/dustinrow/.gvm/pkgsets/go1.13.6/global/bin/golangci-lint: cannot execute binary file
make: *** [gocheck] Error 126
dustinrow@drow-mac:~/src/must-gather-operator$ which golangci-lint
/Users/dustinrow/.gvm/pkgsets/go1.13.6/global/bin/golangci-lint

@arjunrn
Copy link
Contributor Author

arjunrn commented Sep 1, 2020

I tested this by moving golangci-lint out of my $PATH and it's failing locally for me:

@dustman9000 You will have to set and pass the GOOS environment variable when calling make. Otherwise it defaults to linux as you suggested.

Signed-off-by: Arjun Naik <anaik@redhat.com>
@dustman9000
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 1, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arjunrn, dustman9000

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 [arjunrn,dustman9000]

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

@dustman9000
Copy link
Member

Tested locally with latest changes and it pulls down the proper golangci-lint regardless of cross-compile GOOS.

@dustman9000
Copy link
Member

/label tide/merge-method-squash

@openshift-ci-robot openshift-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 1, 2020
@openshift-merge-robot openshift-merge-robot merged commit f630ef8 into master Sep 1, 2020
Comment on lines +11 to +12
if which golangci-lint ; then
exit
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't ensure we're using $GOLANGCI_LINT_VERSION. As written, there are several ways that could go awry even when we're running the gocheck target in a container -- e.g. if the base container image we're using already has the binary (at the wrong version).

So IMO we really ought to include a version check here. Something like:

Suggested change
if which golangci-lint ; then
exit
if which golangci-lint && [[ $(golangci-lint --version) == *" v$GOLANGCI_LINT_VERSION "* ]]; then
exit

fi
DOWNLOAD_URL="https://github.com/golangci/golangci-lint/releases/download/v${GOLANGCI_LINT_VERSION}/golangci-lint-${GOLANGCI_LINT_VERSION}-${GOOS}-amd64.tar.gz"
curl -sfL "${DOWNLOAD_URL}" | tar -C "${GOPATH}/bin" -zx --strip-components=1 "golangci-lint-${GOLANGCI_LINT_VERSION}-${GOOS}-amd64/golangci-lint"
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the amd64 bit also break on Mac? (ppc or something?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No Macs have used intel x86 architecture for about 10 years now.

Copy link
Member

Choose a reason for hiding this comment

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

TIL.

We're still totally screwing all the people trying to develop on AIX though.

@arjunrn arjunrn deleted the fix-golangci-lint branch September 2, 2020 07:53
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. lgtm Indicates that a PR is ready to be merged. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependency Installation Broken For golangci-lint
5 participants