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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
