-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add a 'local' pod mode to the pod-utils and mkpod to generate decorated pods suitable for local testing. #13561
Conversation
prow/cmd/mkpod/local-kind.sh
Outdated
echo config=${config} | ||
echo job_config=${job_config} | ||
|
||
if [[ -n ${job_config} ]] |
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.
you enforce that this is set above with :-
so no need to check
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 used -
not :-
so I think the check is necessary.
Allowing JOB_CONFIG_PATH=""
is necessary to override the default if you are using this script with a different Prow instance that doesn't have separate job config.
This seems like gross UX to me though. I'm open to suggestions.
Changed to having a general script and a prow.k8s.io specific one that defaults the config locations. We can create similar scripts for other prow instances. |
config/pj-on-kind.sh
Outdated
|
||
export CONFIG_PATH="$(readlink -f $(dirname "${BASH_SOURCE[0]}")/../prow/config.yaml)" | ||
export JOB_CONFIG_PATH="$(readlink -f $(dirname "${BASH_SOURCE[0]}")/jobs)" | ||
bash <(curl -s https://raw.githubusercontent.com/cjwagner/test-infra/local-pod-go/prow/pj-on-kind.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.
cjwagner?
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.
That was pointed at my fork for testing. Fixed.
config/pj-on-kind.sh
Outdated
|
||
export CONFIG_PATH="$(readlink -f $(dirname "${BASH_SOURCE[0]}")/../prow/config.yaml)" | ||
export JOB_CONFIG_PATH="$(readlink -f $(dirname "${BASH_SOURCE[0]}")/jobs)" | ||
bash <(curl -s https://raw.githubusercontent.com/kubernetes/test-infra/master/prow/pj-on-kind.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.
Why a curl
instead of relative path?
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.
With curl
, this script can be copied into any other repo with Prow config (e.g. istio/test-infra
) and the only change needed to make everything work is to adjust the config envvars accordingly. Then, jobs for that Prow instance can be run with ./pj-on-kind.sh my-istio-job
. Fetching the k/t-i/prow/pj-on-kind.sh
file each time ensures that the latest version is always used.
In this case a relative path is equivalent, but I want a copy-pasteable example.
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 changed to use a relative path, but left the commented out the curl
version.
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.
Might want to just package it in a container image for re-use
prow/pj-on-kind.sh
Outdated
job="${1:-""}" | ||
config="${CONFIG_PATH:-""}" | ||
job_config="${JOB_CONFIG_PATH:-""}" | ||
out_dir="${OUT_DIR:-/tmp/prowjob-out}" |
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.
nit: if you're going to quote the empty strings for defaults above you might as well quote this default, too :)
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.
Done.
prow/pj-on-kind.sh
Outdated
echo JOB_CONFIG_PATH=${job_config} | ||
echo OUT_DIR=${out_dir} "(May be different when reusing an existing kind cluster.)" | ||
|
||
if [[ -z ${job} ]] |
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.
quote your expansions
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 think I got them all.
prow/pj-on-kind.sh
Outdated
out_dir="${OUT_DIR:-/tmp/prowjob-out}" | ||
|
||
echo job=${job} | ||
echo CONFIG_PATH=${config} |
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.
quote strings with embedded expansions
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.
Done.
prow/pj-on-kind.sh
Outdated
fi | ||
|
||
# Install kind and set up cluster if not already done. | ||
if [[ -z $(which kind) ]] |
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.
which
will output something like /usr/bin/which: no kind in (${PATH})
on failure so this does not do what you want.
use:
if ! which kind >/dev/null 2>&1; then
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.
and i think there was some weirdness with macos on this one, so you might have to use command -v
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.
Switched to command -v
based on this SO post: https://stackoverflow.com/a/677212
prow/pj-on-kind.sh
Outdated
found="true" | ||
fi | ||
done | ||
if [[ ${found} == "false" ]] |
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.
nit: could just inline this into the if
where you find the match instead of doing it after the loop`
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.
We only want to do this if we don't find a match though.
prow/pj-on-kind.sh
Outdated
fi | ||
export KUBECONFIG="$(kind get kubeconfig-path --name="mkpod")" | ||
|
||
# Install mkpj and mkpod if not already done. |
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.
maybe prefer running the published containers?
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 wanted to avoid the volume mounting and figured this workflow makes it easier to tweak mkpj
and mkpod
when needed.
I'll add a docker mode in a follow up and we can even make that the default if that seems preferable, but this mode is easier to start with while I'm developing.
GCSupload/ go code LGTM |
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 should be ready for review now. I tried to clean up the bash and I moved the LocalOutputDir
field into the prowapi.GCSOptions
type so that it could be specified in ProwJob config as well.
I'll keep improving this in follow up PRs, but I'd like to get this merged so that you don't have to update the prow config to use my forked versions of the pod utils to use this.
prow/pj-on-kind.sh
Outdated
job="${1:-""}" | ||
config="${CONFIG_PATH:-""}" | ||
job_config="${JOB_CONFIG_PATH:-""}" | ||
out_dir="${OUT_DIR:-/tmp/prowjob-out}" |
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.
Done.
prow/pj-on-kind.sh
Outdated
out_dir="${OUT_DIR:-/tmp/prowjob-out}" | ||
|
||
echo job=${job} | ||
echo CONFIG_PATH=${config} |
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.
Done.
prow/pj-on-kind.sh
Outdated
echo JOB_CONFIG_PATH=${job_config} | ||
echo OUT_DIR=${out_dir} "(May be different when reusing an existing kind cluster.)" | ||
|
||
if [[ -z ${job} ]] |
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 think I got them all.
prow/pj-on-kind.sh
Outdated
fi | ||
|
||
# Install kind and set up cluster if not already done. | ||
if [[ -z $(which kind) ]] |
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.
Switched to command -v
based on this SO post: https://stackoverflow.com/a/677212
prow/pj-on-kind.sh
Outdated
found="true" | ||
fi | ||
done | ||
if [[ ${found} == "false" ]] |
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.
We only want to do this if we don't find a match though.
prow/pj-on-kind.sh
Outdated
fi | ||
export KUBECONFIG="$(kind get kubeconfig-path --name="mkpod")" | ||
|
||
# Install mkpj and mkpod if not already done. |
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 wanted to avoid the volume mounting and figured this workflow makes it easier to tweak mkpj
and mkpod
when needed.
I'll add a docker mode in a follow up and we can even make that the default if that seems preferable, but this mode is easier to start with while I'm developing.
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.
One bash if
nit otherwise good
prow/pj-on-kind.sh
Outdated
# Ensures installation of prow tools, kind, and a kind cluster named "mkpod". | ||
function ensureInstall() { | ||
# Install mkpj and mkpod if not already done. | ||
if [[ $(command -v mkpj &>/dev/null) ]]; then |
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.
If you're checking the result of a call just do
if ! command -v mkpj >/dev/null 2>&1; then
No need to test if the output string is or is not empty
Also switches to building mkpj and mkpod with go instead of bazel.
/lgtm |
LGTM label has been added. Git tree hash: f367fcefb3c4bf5f2a3dd4a1b73ff72a1aa3d68b
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cjwagner, stevekuznetsov 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 |
This PR is broken into 3 commits:
hostPath
output volume is mounted and files that would have been uploaded to GCS are copied there.--local
it configures the pod utilities to output locally and prompts for each volume that is not ahostPath
oremptyDir
to be replaced by anemptyDir
,hostPath
, or kept the same (this assumes the volume will be available in the cluster where the pod will run).local-kind.sh
script is a simple wrapper aroundmkpj
,mkpod
, andkind
. It creates a kind cluster namedmkpod
if one doesn't exist and usesmkpj
andmkpod
to create a pod that is then applied to the kind cluster and watched.Example usage after installing docker and kind:
Testing a job from a different Prow instance in a different repo (
istio/test-infra
):I still need to add tests and documention, but I wanted to get some feedback on the direction first.
/assign @stevekuznetsov @fejta
fixes #13475