-
Notifications
You must be signed in to change notification settings - Fork 39.2k
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 test-cmd
work with OS-X tooling
#61393
Make test-cmd
work with OS-X tooling
#61393
Conversation
OS-X thips with the BSD versions of `date` and `grep`. Those don't have certain features the script relies on: - BSD date does not support nanoseconds (`%N`) - BSD grep does not support perl RegEx (`-P`) As we use `bash` specifically in the hashbang anyway it is probably fine to rely on the `$RANDOM` bashism here.
hack/make-rules/test-cmd-util.sh
Outdated
local ns_name | ||
ns_name="namespace-$(date +%s)-${RANDOM}" | ||
kube::log::status "Creating namespace ${ns_name}" | ||
kubectl create namespace "$ns_name" |
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.
I know I'm nitting but, shouldn't it be ${ns_name}
(consistency)?
And below of course
Use "${VAR}" everywhere, instead of also using "$VAR" in some places.
@apelisse : PTAL :) |
/lgtm Thank you, looks great |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: apelisse, ixdy, totherme 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 |
Thanks all! Um -- I see two lgtms, green tests, and a bunch of green labels.. But this hasn't been merged. Anyone have any idea what needs to happen next? :S |
Automatic merge from submit-queue (batch tested with PRs 61453, 61393, 61379, 61373, 61494). If you want to cherry-pick this change to another branch, please follow the instructions here. |
@totherme the merge robot was just waiting for you to comment and ask the status. :) (I think the submit queue was still catching up on PRs after code thaw.) |
OS-X thips with the BSD versions of
date
andgrep
. Those don't havecertain features the script relies on:
%N
)-P
)As we use
bash
specifically in the hashbang anyway it is probably fineto rely on the
$RANDOM
bashism here.What this PR does / why we need it:
Currently
make test-cmd
doesn't work on OSX. This PR makesmake test-cmd
work on OSX.Release note:
cc: @apelisse