-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Check KUBE_SERVER_PLATFORMS emptiness #78059
Conversation
cc @mtaufen, appreciate if you could review it since you introduced KUBE_SERVER_PLATFORMS |
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.
Tested on my 1.14.2 testing branch and it works for command make WHAT=cmd/kubectl KUBE_BUILD_PLATFORMS="darwin/amd64 windows/amd64"
OS: ubuntu 16.04, bash version 4.3
/test pull-kubernetes-e2e-gce-100-performance |
/assign @cblecker @BenTheElder |
Works for me on non-homebrew MacOS, for 1.14.2 plus the other stack of make fixes described in #78048 |
hack/lib/golang.sh
Outdated
@@ -292,7 +292,9 @@ kube::golang::server_test_targets() { | |||
IFS=" " read -ra KUBE_TEST_SERVER_TARGETS <<< "$(kube::golang::server_test_targets)" | |||
readonly KUBE_TEST_SERVER_TARGETS | |||
readonly KUBE_TEST_SERVER_BINARIES=("${KUBE_TEST_SERVER_TARGETS[@]##*/}") | |||
readonly KUBE_TEST_SERVER_PLATFORMS=("${KUBE_SERVER_PLATFORMS[@]}") | |||
if [[ -n "${KUBE_SERVER_PLATFORMS:-}" ]]; then | |||
readonly KUBE_TEST_SERVER_PLATFORMS=("${KUBE_SERVER_PLATFORMS[@]}") |
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.
Instead of ifs, just change this line to:
readonly KUBE_TEST_SERVER_PLATFORMS=("${KUBE_SERVER_PLATFORMS[@]}") | |
readonly KUBE_TEST_SERVER_PLATFORMS=("${KUBE_SERVER_PLATFORMS[@]:+"${KUBE_SERVER_PLATFORMS[@]}"}") |
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.
@cblecker thank you for the suggestion, your version is really nit with just one line, but i find it is less obvious and took me sometime to understand.
i would prefer to keep the current ifs if the extra line make it a little easier to understand, thanks
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.
@figo Yours does something different, however. Yours doesn't even set KUBE_TEST_SERVER_PLATFORMS
because it's an if.
Please make the requested change. Thank you!
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.
@cblecker i see what you mean now, PR refreshed, thx
when compile kubectl on platform other than linux/amd64, we need to check the KUBE_SERVER_PLATFORMS array emptiness before assign it. the example command is: make WHAT=cmd/kubectl KUBE_BUILD_PLATFORMS="darwin/amd64 windows/amd64"
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cblecker, figo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
when compile kubectl on platform other than
linux/amd64, KUBE_SERVER_PLATFORMS will be empty which is expected,
so we need to check the KUBE_SERVER_PLATFORMS array emptiness before use it.
the example command is:
make WHAT=cmd/kubectl KUBE_BUILD_PLATFORMS="darwin/amd64 windows/amd64"
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #78055
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
@cblecker @tpepper @jiatongw @dims @BenTheElder
/sig release