Skip to content

make golangci-lint run locally with correct version #5266

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.

Sign up for GitHub

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

Open
grosser wants to merge 1 commit into
base: master
Choose a base branch
from
Open

make golangci-lint run locally with correct version #5266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,11 @@

# Output of the go coverage tool, specifically when used with LiteIDE
*.out

# Folders used by make
_tmp/
_output/
bin/
Copy link
Contributor

@zhzhuang-zju zhzhuang-zju Jul 29, 2024

Choose a reason for hiding this comment

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

Suggested change
bin/
_tool

How about mimicking the _output directory and naming it _tool or _bin? This directory can be specifically used to place tools for maintaining Karmada.

Copy link
Contributor Author

@grosser grosser Jul 29, 2024

Choose a reason for hiding this comment

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

the beauty of bin/ is that if users have PATH=./bin then things "just work" :)

not against _tool either to be consistent with the other _ folders
never really saw a project use _ folders 🤷
everything could be in tmp/ (tmp/output tmp/bin/ etc)

but either way is fine for me

Copy link
Contributor

@zhzhuang-zju zhzhuang-zju Jul 29, 2024

Choose a reason for hiding this comment

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

the beauty of bin/ is that if users have PATH=./bin then things "just work" :)

agree


# Dependency directories (remove the comment below to include it)
# vendor/
Expand Down
21 changes: 10 additions & 11 deletions hack/verify-staticcheck.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,32 +13,31 @@
# See the License for the specific language governing permissions and
# limitations under the License.

# - download golangci-lint if needed (replace existing if incorrect version)
# - run golangci-lint

set -o errexit
set -o nounset
set -o pipefail

REPO_ROOT=$(dirname "${BASH_SOURCE[0]}")/..
GOLANGCI_LINT_VER="v1.59.0"
GOLANGCI_LINT_VER="1.59.0"
GOLANGCI_LINT="./bin/golangci-lint"
Copy link
Contributor

@liangyuanpeng liangyuanpeng Jul 29, 2024

Choose a reason for hiding this comment

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

seems like you want to download the tools to the project folder and not GOPATH, I think this makes sense, but currently karmada downloads tools into GOPATH, which might be worth discussing.

Copy link
Contributor Author

@grosser grosser Jul 29, 2024

Choose a reason for hiding this comment

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

yeah, other tools would need to follow that pattern, so better get consensus
idk of any reason not to do it 🤷

Copy link
Contributor

@liangyuanpeng liangyuanpeng Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't have much background here, I think it would be possible to add a parameter to have the tool download to the specifically directory, defaulting to gopath, what do you think?

Maybe:

GO_PATH=$(go env GOPATH)
TOOLS_PATH=${TOOLS_PATH:-$GO_PATH}

Copy link
Contributor

@liangyuanpeng liangyuanpeng Jul 29, 2024

Choose a reason for hiding this comment

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

cc @zhzhuang-zju @XiShanYongYe-Chang @RainbowMango

Copy link
Contributor Author

@grosser grosser Jul 29, 2024

Choose a reason for hiding this comment

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

that would fix the error

if we also want the correct version it would need to switch and replace the binaries in gopath depending on what version the current project needs, should mostly be fine just weird when using different projects with different version requirements

Copy link
Contributor

@zhzhuang-zju zhzhuang-zju Jul 29, 2024

Choose a reason for hiding this comment

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

I think we can provide the capability to install to a specific path, but considering that minimizing the impact on users who may have already installed their own golang-lint, the default path could be set to ./bin/


cd "${REPO_ROOT}"
source "hack/util.sh"
mkdir -p bin

if util::cmd_exist golangci-lint ; then
echo "Using golangci-lint version:"
golangci-lint version
else
echo "Installing golangci-lint ${GOLANGCI_LINT_VER}"
# https://golangci-lint.run/usage/install/#other-ci
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin ${GOLANGCI_LINT_VER}
if ! (test -f "${GOLANGCI_LINT}" && ${GOLANGCI_LINT} --version | grep " ${GOLANGCI_LINT_VER} " >/dev/null); then
rm -f ${GOLANGCI_LINT} && echo "Installing ${GOLANGCI_LINT} ${GOLANGCI_LINT_VER}"
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b bin -d "v${GOLANGCI_LINT_VER}"
fi

if golangci-lint run; then
if ${GOLANGCI_LINT} run; then
Copy link
Member

@chaosi-zju chaosi-zju Jul 29, 2024

Choose a reason for hiding this comment

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

Suggested change
if ${GOLANGCI_LINT} run; then
if $(go env GOPATH)/bin/golangci-lint run; then

maybe this is enough to solve your problem.

As for where to put the downloaded files, we can continue to discuss in the above comment.

echo 'Congratulations! All Go source files have passed staticcheck.'
else
echo # print one empty line, separate from warning messages.
echo 'Please review the above warnings.'
echo 'Tips: The golangci-lint might help you fix some issues, try with the command "golangci-lint run --fix".'
echo "Tips: The ${GOLANGCI_LINT} might help you fix some issues, try with the command "${GOLANGCI_LINT} run --fix"."
echo 'If the above warnings do not make sense, feel free to file an issue.'
exit 1
fi