-
Notifications
You must be signed in to change notification settings - Fork 561
Conversation
Partially addresses #24 |
Also, @cjwagner -- fyi |
testing/bootstrap.sh
Outdated
@@ -0,0 +1,41 @@ | |||
#!/bin/bash | |||
# | |||
# This script is used to bootstrap our prow jobs. |
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.
what's a prow 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.
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.
haha, thanks. I meant i wanted a comment from @Kashomon here ;) I'm still looking for a one-sentence explanation of what it is and why i want 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.
How's this
testing/bootstrap.sh
Outdated
# The point of this script is to check out the kubeflow/kubeflow repo | ||
# at the commit corresponding to the Prow job. We can then | ||
# invoke the launcher script at that commit to submit and | ||
# monitor an Argo workflow |
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 what's an argo workflow. (1 sentence)
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.
Oh, I need to remove that. That's some copy pasta.
testing/Makefile
Outdated
|
||
build: | ||
mkdir -p staging | ||
cp ../requirements-cpu.txt staging/requirements-cpu.txt |
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.
do you need to copy the python files in here?
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.
Nope. Those are pulled in via bootstrap.sh from github. I pull in the requirements so I can preload them (which isn't strictly required).
testing/Dockerfile
Outdated
RUN chmod a+x /usr/local/bin/bootstrap.sh | ||
|
||
# We could pre-load requirements to make running the tests | ||
# faster, but this requires rebuilding every time we have a new dep. |
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.
please forgive my ignorance: is this something that is run for automated tests only? Or is the idea that local devs also run their tests this way?
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.
It's only for automated tests, but I will probably create a local version for testing.
@@ -0,0 +1,41 @@ | |||
#!/bin/bash |
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 writing your own bootstrap script to handle checking out the code you should consider using ours. In addition to checking out the repo at the correct commit and merging the PR changes before testing, our script uploads logs to a GCS bucket. This provides persistence (pod logs are only around until the pod is deleted by sinker) and allows logs and test results to be consumed by https://k8s-gubernator.appspot.com.
You can use the 'execute' bootstrap scenario to run your tests if you move the test commands (looks like the last 3 commands in this file) into a shell script.
To configure bootstrap to use the execute scenario to run your test you will need to add an entry like this to config.json. Here is the prowjob config for that same job so you have a complete example to look through.
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 don't know quite how to evaluate this option. I'll come chat with you about 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.
So, I think probably the best way to go is to merge this as-is (since it's working locally) and then to rebase it to use your python runner.
cluster/deploy-gpu-player.sh
Outdated
@@ -16,4 +16,17 @@ | |||
|
|||
source ./common.sh | |||
|
|||
envsubst < gpu-player.yaml | kubectl apply -f - | |||
# envsubst doesn't exist for OSX. needs to be brew-installed |
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.
obsolete comment?
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 shouldn't be changed any longer.
cluster/deploy-cpu-player.sh
Outdated
|
||
source ./common.sh | ||
|
||
# envsubst doesn't exist for OSX. needs to be brew-installed |
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.
obsolete comment?
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 shouldn't be changed any longer.
testing/Dockerfile
Outdated
@@ -53,8 +53,10 @@ RUN chmod a+x /usr/local/bin/bootstrap.sh | |||
|
|||
# We could pre-load requirements to make running the tests | |||
# faster, but this requires rebuilding every time we have a new dep. | |||
COPY staging/requirements-cpu.txt /tmp/requirements-cpu.txt | |||
COPY staging/requirements.txt /tmp/requirements.txt |
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 did this go into /tmp?
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.
Oh, I dunno. Made the workdir /testing and put it there.
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.
seems fine.
Based off the kubeflow test harness, this is a container that'll get pulled down and run by prow. It currently:
In the future, it'd be awesome to spin up clusters.