-
Notifications
You must be signed in to change notification settings - Fork 152
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
Make check fixes #1534
Make check fixes #1534
Conversation
To check for existence of Go rather an specific version
Thanks for submitting this pull request 🎉. The team will review it soon and get back to you. If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document. |
Makefile
Outdated
@@ -285,7 +285,7 @@ stop-kind: | |||
@$(MAKE) run CMD='-c "./build/local_kubernetes.sh stop_localkube"' | |||
|
|||
check: | |||
@./build/check.sh | |||
@$(MAKE) run CMD='-c "./build/check.sh"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ends up checking the Docker image, instead of the local setup. If I set BUILD_IMAGE=debian:bookworm
in the Makefile
, this step will fail.
Can you try this change on OSX to see if it works? I changed the script to use sh
shell and replace the upper case symbols with awk
.
diff --git a/build/check.sh b/build/check.sh
index c6c78c2d..f9f034d6 100755
--- a/build/check.sh
+++ b/build/check.sh
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/bin/sh
GO_BIN=go
DOCKER_BIN=docker
@@ -11,12 +11,12 @@ failed="❌"
pad=$(printf '%0.1s' "."{1..25})
pad_len=25
-function output() {
- local bin=$1
+output() {
+ local bin=$(echo "$1" | awk '{print toupper(substr( $0, 1, 1 )) substr($0, 2)}')
local result=$2
- local msg=$3
+ local msg=$(echo "$3" | awk '{print toupper(substr( $0, 1, 1 )) substr($0, 2)}')
- printf "* need %s" "${bin^}"
+ printf "* need %s" "${bin}"
printf "%*.*s %s" 0 $((pad_len - ${#bin})) "${pad}" "${result}"
if [ ! -z "${msg}" ]; then
printf "\n → %s" "${msg}"
@@ -24,33 +24,33 @@ function output() {
printf "\n"
}
-function check_version_go() {
+check_version_go() {
local result=""
if command -v ${GO_BIN} > /dev/null 2>&1 ; then
result="${ok}"
else
result="${failed}"
- msg="${GO_BIN^} not installed"
+ msg="${GO_BIN} not installed"
fi
output "${GO_BIN}" "${result}" "${msg}"
}
-function check_version_docker() {
+check_version_docker() {
local result=""
if command -v ${DOCKER_BIN} > /dev/null 2>&1 ; then
result="${ok}"
else
result="${failed}"
- msg="${DOCKER_BIN^} not installed"
+ msg="${DOCKER_BIN} not installed"
fi
output "${DOCKER_BIN}" "${result}" "${msg}"
}
-function check_version_kubectl() {
+check_version_kubectl() {
local result=""
if command -v ${KUBECTL_BIN} > /dev/null 2>&1 ; then
result="${ok}"
@@ -59,7 +59,7 @@ function check_version_kubectl() {
msg="${KUBECTL_BIN} not installed"
fi
- output "${KUBECTL_BIN^}" "${result}" "${msg}"
+ output "${KUBECTL_BIN}" "${result}" "${msg}"
}
check_version_go
Let me know if you have any questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This step does fail after making these specific changes and running the check inside the Docker image. The issue was raised because the script fails on OSX as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we try applying the above patch, and run the script on OSX, without using Docker? The purpose of make check
is to validate the toolset on the developer's machine, not a Docker container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran this patch with the Debian Bookworm image and #!/bin/sh
instead of bash on my local OSX (without using Docker) - all checks passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome - feel free to update this PR with the patch. Then we can merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍. Thanks for the fix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Change Overview
This PR modifies
make check
to run inside the build container and modifies thego_version_check
function to check only for the existence of Go rather than the specific Go version.Pull request type
Please check the type of change your PR introduces:
Issues
make check
should run inside the build container #1530Test Plan