You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2022/08/01 02:39:24 UTC
[GitHub] [yunikorn-k8shim] wilfred-s commented on a diff in pull request #443: [YUNIKORN-1265] fix shellcheck issues in our shell scripts
wilfred-s commented on code in PR #443:
URL: https://github.com/apache/yunikorn-k8shim/pull/443#discussion_r934091847
##########
Makefile:
##########
@@ -358,3 +358,37 @@ arch:
e2e_test:
@echo "running e2e tests"
cd ./test/e2e && ginkgo -r -v -timeout=2h -- -yk-namespace "yunikorn" -kube-config $(KUBECONFIG)
+
+.PHONY: install_shellcheck
+SHELLCHECK_PATH := $(BASE_DIR)scripts/shellcheck
+SHELLCHECK_VERSION := "v0.8.0"
+SHELLCHECK_ARCHIVE := "shellcheck-$(SHELLCHECK_VERSION).$(OS).$(HOST_ARCH).tar.xz"
+install_shellcheck: $(eval SHELL:=/bin/bash)
+ @if [[ ! -f $(SHELLCHECK_PATH) && "$(shell which shellcheck)" == "" ]]; then \
+ echo "shellcheck is not installed"; \
+ if [[ $(HOST_ARCH) == "arm64" ]]; then \
Review Comment:
`which` could be a builtin or an external command. Behaviour differs between shells zsh always says "not found" and breaks this detection. `command -v exe-name` is portable
##########
Makefile:
##########
@@ -358,3 +358,37 @@ arch:
e2e_test:
@echo "running e2e tests"
cd ./test/e2e && ginkgo -r -v -timeout=2h -- -yk-namespace "yunikorn" -kube-config $(KUBECONFIG)
+
+.PHONY: install_shellcheck
+SHELLCHECK_PATH := $(BASE_DIR)scripts/shellcheck
+SHELLCHECK_VERSION := "v0.8.0"
+SHELLCHECK_ARCHIVE := "shellcheck-$(SHELLCHECK_VERSION).$(OS).$(HOST_ARCH).tar.xz"
+install_shellcheck: $(eval SHELL:=/bin/bash)
+ @if [[ ! -f $(SHELLCHECK_PATH) && "$(shell which shellcheck)" == "" ]]; then \
+ echo "shellcheck is not installed"; \
+ if [[ $(HOST_ARCH) == "arm64" ]]; then \
+ echo "Can't find shellcheck for arm64 from https://github.com/koalaman/shellcheck/releases/$(SHELLCHECK_VERSION)"; \
+ exit 1; \
+ else \
+ echo "Installing shellcheck $(SHELLCHECK_VERSION) ..." \
+ && set -x \
+ && wget https://github.com/koalaman/shellcheck/releases/download/$(SHELLCHECK_VERSION)/$(SHELLCHECK_ARCHIVE) \
+ && tar -xf $(SHELLCHECK_ARCHIVE) shellcheck-$(SHELLCHECK_VERSION)/shellcheck \
+ && rm $(SHELLCHECK_ARCHIVE) \
+ && mv shellcheck-$(SHELLCHECK_VERSION)/shellcheck $(SHELLCHECK_PATH) \
+ && chmod +x $(SHELLCHECK_PATH) \
+ && rmdir shellcheck-$(SHELLCHECK_VERSION) \
+ && set +x \
+ && echo "Shellcheck $(SHELLCHECK_VERSION) has been installed at $(SHELLCHECK_PATH)"; \
+ fi \
+ else \
+ echo "shellcheck has been installed"; \
+ fi
+
+# Check scripts
+.PHONY: check_scripts
+ALLSCRIPTS := $(shell find . -name '*.sh')
+check_scripts: install_shellcheck
+ @echo "running shellcheck"
+ @if ! $(shell which shellcheck) $(ALLSCRIPTS); then exit 1; fi
Review Comment:
This fails if the local machine does not have shellcheck installed and it was added via the make target and the $(BASE_DIR)scripts is not my the path.
Also see comment above about which and zsh
##########
Makefile:
##########
@@ -358,3 +358,37 @@ arch:
e2e_test:
@echo "running e2e tests"
cd ./test/e2e && ginkgo -r -v -timeout=2h -- -yk-namespace "yunikorn" -kube-config $(KUBECONFIG)
+
+.PHONY: install_shellcheck
+SHELLCHECK_PATH := $(BASE_DIR)scripts/shellcheck
+SHELLCHECK_VERSION := "v0.8.0"
+SHELLCHECK_ARCHIVE := "shellcheck-$(SHELLCHECK_VERSION).$(OS).$(HOST_ARCH).tar.xz"
+install_shellcheck: $(eval SHELL:=/bin/bash)
+ @if [[ ! -f $(SHELLCHECK_PATH) && "$(shell which shellcheck)" == "" ]]; then \
+ echo "shellcheck is not installed"; \
+ if [[ $(HOST_ARCH) == "arm64" ]]; then \
+ echo "Can't find shellcheck for arm64 from https://github.com/koalaman/shellcheck/releases/$(SHELLCHECK_VERSION)"; \
+ exit 1; \
+ else \
+ echo "Installing shellcheck $(SHELLCHECK_VERSION) ..." \
+ && set -x \
Review Comment:
Keep it clean no need to show details of the process
##########
Makefile:
##########
@@ -358,3 +358,37 @@ arch:
e2e_test:
@echo "running e2e tests"
cd ./test/e2e && ginkgo -r -v -timeout=2h -- -yk-namespace "yunikorn" -kube-config $(KUBECONFIG)
+
+.PHONY: install_shellcheck
+SHELLCHECK_PATH := $(BASE_DIR)scripts/shellcheck
+SHELLCHECK_VERSION := "v0.8.0"
+SHELLCHECK_ARCHIVE := "shellcheck-$(SHELLCHECK_VERSION).$(OS).$(HOST_ARCH).tar.xz"
+install_shellcheck: $(eval SHELL:=/bin/bash)
+ @if [[ ! -f $(SHELLCHECK_PATH) && "$(shell which shellcheck)" == "" ]]; then \
+ echo "shellcheck is not installed"; \
+ if [[ $(HOST_ARCH) == "arm64" ]]; then \
+ echo "Can't find shellcheck for arm64 from https://github.com/koalaman/shellcheck/releases/$(SHELLCHECK_VERSION)"; \
+ exit 1; \
+ else \
+ echo "Installing shellcheck $(SHELLCHECK_VERSION) ..." \
+ && set -x \
+ && wget https://github.com/koalaman/shellcheck/releases/download/$(SHELLCHECK_VERSION)/$(SHELLCHECK_ARCHIVE) \
+ && tar -xf $(SHELLCHECK_ARCHIVE) shellcheck-$(SHELLCHECK_VERSION)/shellcheck \
+ && rm $(SHELLCHECK_ARCHIVE) \
Review Comment:
wget is not installed on a mac by default we should not use it, target failed for me due to that. Other installs use curl:
```
curl -sSfL https://github.com/koalaman/shellcheck/releases/download/v0.8.0/shellcheck-v0.8.0.linux.x86_64.tar.xz| tar -x --strip-components=1 shellcheck-v0.8.0/shellcheck
```
That drops the `shellcheck` binary at the same level as from where the make target has been started and does not save the archive, it should maintain the x bit (no need for chmod), removes the need for mv and rmdir also.
##########
Makefile:
##########
@@ -358,3 +358,37 @@ arch:
e2e_test:
@echo "running e2e tests"
cd ./test/e2e && ginkgo -r -v -timeout=2h -- -yk-namespace "yunikorn" -kube-config $(KUBECONFIG)
+
+.PHONY: install_shellcheck
+SHELLCHECK_PATH := $(BASE_DIR)scripts/shellcheck
+SHELLCHECK_VERSION := "v0.8.0"
+SHELLCHECK_ARCHIVE := "shellcheck-$(SHELLCHECK_VERSION).$(OS).$(HOST_ARCH).tar.xz"
+install_shellcheck: $(eval SHELL:=/bin/bash)
+ @if [[ ! -f $(SHELLCHECK_PATH) && "$(shell which shellcheck)" == "" ]]; then \
+ echo "shellcheck is not installed"; \
+ if [[ $(HOST_ARCH) == "arm64" ]]; then \
+ echo "Can't find shellcheck for arm64 from https://github.com/koalaman/shellcheck/releases/$(SHELLCHECK_VERSION)"; \
+ exit 1; \
+ else \
+ echo "Installing shellcheck $(SHELLCHECK_VERSION) ..." \
+ && set -x \
+ && wget https://github.com/koalaman/shellcheck/releases/download/$(SHELLCHECK_VERSION)/$(SHELLCHECK_ARCHIVE) \
+ && tar -xf $(SHELLCHECK_ARCHIVE) shellcheck-$(SHELLCHECK_VERSION)/shellcheck \
+ && rm $(SHELLCHECK_ARCHIVE) \
+ && mv shellcheck-$(SHELLCHECK_VERSION)/shellcheck $(SHELLCHECK_PATH) \
+ && chmod +x $(SHELLCHECK_PATH) \
+ && rmdir shellcheck-$(SHELLCHECK_VERSION) \
+ && set +x \
+ && echo "Shellcheck $(SHELLCHECK_VERSION) has been installed at $(SHELLCHECK_PATH)"; \
+ fi \
+ else \
+ echo "shellcheck has been installed"; \
+ fi
+
+# Check scripts
+.PHONY: check_scripts
+ALLSCRIPTS := $(shell find . -name '*.sh')
+check_scripts: install_shellcheck
+ @echo "running shellcheck"
+ @if ! $(shell which shellcheck) $(ALLSCRIPTS); then exit 1; fi
+ @if [ -e $(SHELLCHECK_PATH) ]; then rm $(SHELLCHECK_PATH) ; fi
Review Comment:
Leave the binary and add it to `.gitignore`
##########
Makefile:
##########
@@ -358,3 +358,37 @@ arch:
e2e_test:
@echo "running e2e tests"
cd ./test/e2e && ginkgo -r -v -timeout=2h -- -yk-namespace "yunikorn" -kube-config $(KUBECONFIG)
+
+.PHONY: install_shellcheck
+SHELLCHECK_PATH := $(BASE_DIR)scripts/shellcheck
Review Comment:
Preference is to keep it at the top level and add `shellcheck` binary to `.gitignore`, similar as the coverage.txt file generated during the build.
##########
Makefile:
##########
@@ -358,3 +358,37 @@ arch:
e2e_test:
@echo "running e2e tests"
cd ./test/e2e && ginkgo -r -v -timeout=2h -- -yk-namespace "yunikorn" -kube-config $(KUBECONFIG)
+
+.PHONY: install_shellcheck
+SHELLCHECK_PATH := $(BASE_DIR)scripts/shellcheck
+SHELLCHECK_VERSION := "v0.8.0"
+SHELLCHECK_ARCHIVE := "shellcheck-$(SHELLCHECK_VERSION).$(OS).$(HOST_ARCH).tar.xz"
+install_shellcheck: $(eval SHELL:=/bin/bash)
+ @if [[ ! -f $(SHELLCHECK_PATH) && "$(shell which shellcheck)" == "" ]]; then \
+ echo "shellcheck is not installed"; \
+ if [[ $(HOST_ARCH) == "arm64" ]]; then \
+ echo "Can't find shellcheck for arm64 from https://github.com/koalaman/shellcheck/releases/$(SHELLCHECK_VERSION)"; \
Review Comment:
Simple failure message is enough: arm is not supported
##########
Makefile:
##########
@@ -358,3 +358,37 @@ arch:
e2e_test:
@echo "running e2e tests"
cd ./test/e2e && ginkgo -r -v -timeout=2h -- -yk-namespace "yunikorn" -kube-config $(KUBECONFIG)
+
+.PHONY: install_shellcheck
+SHELLCHECK_PATH := $(BASE_DIR)scripts/shellcheck
+SHELLCHECK_VERSION := "v0.8.0"
+SHELLCHECK_ARCHIVE := "shellcheck-$(SHELLCHECK_VERSION).$(OS).$(HOST_ARCH).tar.xz"
+install_shellcheck: $(eval SHELL:=/bin/bash)
Review Comment:
Bash on the mac is replaced with zsh. We should use a portable make script not rely on bash
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org