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/07/30 09:44:55 UTC
[GitHub] [yunikorn-k8shim] lowc1012 opened a new pull request, #443: [YUNIKORN-1265] fix shellcheck issues in our shell scripts
lowc1012 opened a new pull request, #443:
URL: https://github.com/apache/yunikorn-k8shim/pull/443
### What is this PR for?
Fix issues and add shellcheck into ci
### What type of PR is it?
* [x] - Bug Fix
* [ ] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Fix issues and add shellcheck into CI for other repos
### What is the Jira issue?
[YUNIKORN-1265](https://issues.apache.org/jira/projects/YUNIKORN/issues/YUNIKORN-1265)
### How should this be tested?
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
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
[GitHub] [yunikorn-k8shim] wilfred-s closed pull request #443: [YUNIKORN-1265] fix shellcheck issues in our shell scripts
Posted by GitBox <gi...@apache.org>.
wilfred-s closed pull request #443: [YUNIKORN-1265] fix shellcheck issues in our shell scripts
URL: https://github.com/apache/yunikorn-k8shim/pull/443
--
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
[GitHub] [yunikorn-k8shim] wilfred-s commented on a diff in pull request #443: [YUNIKORN-1265] fix shellcheck issues in our shell scripts
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [yunikorn-k8shim] codecov[bot] commented on pull request #443: [YUNIKORN-1265] fix shellcheck issues in our shell scripts
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #443:
URL: https://github.com/apache/yunikorn-k8shim/pull/443#issuecomment-1200127090
# [Codecov](https://codecov.io/gh/apache/yunikorn-k8shim/pull/443?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#443](https://codecov.io/gh/apache/yunikorn-k8shim/pull/443?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9df70e6) into [master](https://codecov.io/gh/apache/yunikorn-k8shim/commit/f676c05d3fbfb6e25e7fff2dc4766434a76456e0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f676c05) will **decrease** coverage by `0.02%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #443 +/- ##
==========================================
- Coverage 67.34% 67.31% -0.03%
==========================================
Files 41 41
Lines 6744 6744
==========================================
- Hits 4542 4540 -2
- Misses 2032 2034 +2
Partials 170 170
```
| [Impacted Files](https://codecov.io/gh/apache/yunikorn-k8shim/pull/443?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [pkg/dispatcher/dispatcher.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/443/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2Rpc3BhdGNoZXIvZGlzcGF0Y2hlci5nbw==) | `74.82% <0.00%> (-1.40%)` | :arrow_down: |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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