-
Notifications
You must be signed in to change notification settings - Fork 47
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 image building and tagged testing to CI #350
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
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.
A bunch of comments, but overall like it.
TAG="$1" | ||
[[ -z "${TAG}" ]] && TAG="latest_rc" | ||
|
||
NOW="$(date +"%Y-%m-%d_%H_%M")" |
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.
No seconds? At the very least it could be good for debugging.
|
||
NOW="$(date +"%Y-%m-%d_%H_%M")" | ||
ARCH="amd64" | ||
[[ "$(uname -m)" = "arm64" ]] && ARCH="arm64" |
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.
Is it bad to just do ARCH=$(uname -m)
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 want amd64
instead of X86_64
which is what I get on those machines. I don't anticipate trying to run this on android or raspberry pi or (god forbid) powerpc
GIT_LOGFILE="${RUNDIR}/git.log" | ||
DOCKER_LOGFILE="${RUNDIR}/docker.log" | ||
POETRY_LOGFILE="${RUNDIR}/poetry.log" | ||
PYTEST_LOGFILE="${RUNDIR}/pytest.log" | ||
QUERY_LOGFILE="${RUNDIR}/test_queries.log" |
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'd just rename these to _LOG
for brevity. It's no less inaccurate. These are really paths, but nobody wants to read DOCKER_LOGFILE_PATH
. Anyway, a log is a file by default.
build_containers > "${DOCKER_LOGFILE}" 2>&1 | ||
runtests > "${PYTEST_LOGFILE}" 2>&1 | ||
local passed_tests=$? | ||
handle_outputs passed_tests |
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.
Missing a $
?
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 know it! (exit codes don't do anything weird when they become variables, right?)
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 believe the number just carries across the assignment faithfully. It's when used as a conditional that zero becomes true.
else | ||
echo "No changes detected. Skipping integration tests" >&2 | ||
fi | ||
} | ||
|
||
error() { |
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 should call this die
for accuracy and consistency with our other scripts and general convention.
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.
leaving as error but changing so it doesn't exit. I basically never want this script to exit early
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 it's just cleanup you're worried about, you could also look into trap
with EXIT
.
} | ||
|
||
_docker-build-args() { | ||
local branch="$(git status | head -1 | grep 'On branch ' | awk '{print $3}')" |
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.
head -n1 is better (fewer warnings). I'd also use grep -i just in case (get 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.
git branch --show-current
?
_docker-build-args() { | ||
local branch="$(git status | head -1 | grep 'On branch ' | awk '{print $3}')" | ||
local rev="$(git rev-parse --short HEAD)" | ||
local date="$(git show -s --format=%ci HEAD | sed 's/ /_/g')" |
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 preferred to have sed -e <program>
.
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?
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.
Mostly hygiene. If the program happens to have something that might get interpreted as a command-line option, -e protects it. Also, you can do multi-expression scripts with multiple -e directives.
local rev="$(git rev-parse --short HEAD)" | ||
local date="$(git show -s --format=%ci HEAD | sed 's/ /_/g')" | ||
local diff=unknown | ||
if [[ $(git status | grep -c 'nothing to commit, working tree clean') = 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.
Maybe grep -i
but even that seems too dependent on the exact wording of the message. Also, you might be able to use the exit code of grep directly, like:
if git status | grep -iq 'nothing to commit'; 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.
We want to check for the specific message to guarantee the tree is clean (no unexpected files also).
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.
The definition of clean is zero modified files and zero untracked files. The future-proof way to do this is:
if [[ -z $(git status --porcelain) ]]; then
if [[ $(git status | grep -c 'nothing to commit, working tree clean') = 1 ]]; then | ||
diff=clean | ||
else | ||
diff="pending_changes_$(git diff HEAD | shasum | awk '{print $1}')" |
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.
Is the SHA important? If so, SHA-1 is considered obsolete; so, you'd want shasum -a 256
, but they're longer. For a shorter checksum that's OK, try md5sum
.
else | ||
diff="pending_changes_$(git diff HEAD | shasum | awk '{print $1}')" | ||
fi | ||
echo "--build-arg=GIT_BRANCH=${branch} --build-arg=GIT_COMMIT=${rev}--${date} --build-arg=GIT_DIFF=${diff}" |
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 double-delimiters between rev and date?
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.
Because date has -'s in it; and -- makes it easier to read.
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 are better formats for date available, for instance ISO 8601 basic.
main() { | ||
if [[ ! -d ".git" ]]; then | ||
echo "Error: please run this script from sycamore root!" >&2 | ||
exit 1 | ||
fi | ||
mkdir -p "${RUNDIR}" | ||
echo "Building/testing tag ${TAG}" >&2 | ||
echo "Get the newest git commits" >&2 | ||
checkout_main_if_new | ||
local should_run=$? | ||
if [[ $should_run ]]; 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.
You can shorten this to if [[ checkout_main_if_new ]]; 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.
You can ditch the square brackets entirely.
$ mytrue() {
> return 0
> }
$ if mytrue; then
> echo yes
> fi
yes
build_containers | ||
runtests | ||
handle_outputs | ||
poetry install > "${POETRY_LOGFILE}" 2>&1 |
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're going to need || die "poetry install failed"
and similar on all the remaining bits.
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.
well, I always want to get to the handle_outputs
, so I don't think I want to die. I think I can &&
all these things together to get the correct behavior though.
poetry install > "${POETRY_LOGFILE}" 2>&1 \
&& build_containers > "${DOCKER_LOGFILE}" 2>&1 \
&& runtests > "${PYTEST_LOGFILE}" 2>&1
new_sha="$(git rev-parse FETCH_HEAD)" | ||
if [[ "${old_sha}" != "${new_sha}" ]]; then | ||
git pull origin main >&2 | ||
git pull --rebase origin main >> "${GIT_LOGFILE}" |
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.
Check that git status is clean.
[[ $(git status | grep -c 'nothing to commit, working tree clean') = 1 ]] || die "Working tree not clean"
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.
since I dont want to die, does
{ echo "Working tree not clean" > "${GIT_LOGFILE}" && return 1; }
do the right thing after the ||?
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.
Hmm, I need it to be okay with untracked files, bc I create them in apps/integration/runs.
will make this a grep -c -e 'nothing to commit, working tree clean' -e 'nothing added to commit but untracked files present'
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.
Let's not rely on specific English-language messages. How about running git status --porcelain | grep -vF '??'
and using -z
to check for no output.
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.
alternatively I can just add apps/integration/runs to the gitignore and then take the simpler -z git status --procelain you gave me earlier
@@ -1,30 +1,52 @@ | |||
#!/bin/bash | |||
|
|||
TAG="$1" | |||
[[ -z "${TAG}" ]] && TAG="latest_rc" |
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 would do TAG="integration_tests" by default.
[[ -n "${repo_name}" ]] || error "empty repo name" | ||
shift | ||
|
||
local platform=linux/amd64,linux/arm64 |
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 we want this? I would think we would build/test for the local platform.
local rev="$(git rev-parse --short HEAD)" | ||
local date="$(git show -s --format=%ci HEAD | sed 's/ /_/g')" | ||
local diff=unknown | ||
if [[ $(git status | grep -c 'nothing to commit, working tree clean') = 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.
We want to check for the specific message to guarantee the tree is clean (no unexpected files also).
else | ||
diff="pending_changes_$(git diff HEAD | shasum | awk '{print $1}')" | ||
fi | ||
echo "--build-arg=GIT_BRANCH=${branch} --build-arg=GIT_COMMIT=${rev}--${date} --build-arg=GIT_DIFF=${diff}" |
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.
Because date has -'s in it; and -- makes it easier to read.
mv test-output.log "${QUERY_LOGFILE}" | ||
[[ ${passed_tests} = 0 ]] && touch "${RUNDIR}/passed" | ||
[[ ${passed_tests} != 0 ]] && touch "${RUNDIR}/failed" | ||
aws s3 cp -r "${RUNDIR}" "s3://sycamore-ci/${ARCH}" | ||
} | ||
|
||
runtests() { | ||
docker system prune -f --volumes |
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 seems risky since if someone runs it it will prune other stuff they might want; It also means that we get rid of the old volumes; it's also not guaranteed to clean things up if it's still in use.
I'd instead suggest that we generate a unique run id (use the date stamp?). and do the docker compose -p up. If we tag the images in the same way, then if there's a problem someone could go back and debug against the exact set of images. This would imply we don't want to push to docker hub since it's a lot of stuff.
That guarantees you're getting
@@ -33,20 +55,73 @@ checkout_main_if_new() { | |||
|
|||
build_containers() { | |||
echo "Yep, definitely building containers. That's what this function does" >&2 | |||
docker-build-hub apps/crawler/crawler/http/Dockerfile |
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 we want to build and push to docker hub or only build to local? This seems like it's going to generate a lot of transit data. Moreover, given the way you're doing the testing, if we want to test on arm it would be a separate build/test run and then the two builds would clash.
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.
Personally, I'd find it convenient that reasonably up-to-date images are easily available to test with. Having them in DockerHub seems like the lowest-friction way.
_docker-repo-name() { | ||
local docker_file="$1" | ||
echo "Finding repo name in: ${docker_file}" >&2 | ||
local repo_name="$(grep '^# Repo name: ' "${docker_file}" | awk '{print $4}')" |
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 don't need to be very robust here. We control the input file.
…ter testing Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Is it time to rename |
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
you know I love rewriting bash into python! |
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.
Looking better. I'll let Eric look, too.
I'm still not super-happy calling it runtests when it does so much more. Would things improve if it were split into different scripts (or verb arguments) for the various steps: building, pushing, etc.
mkdir -p "${RUNDIR}" | ||
echo "Building/testing tag ${TAG}" >&2 | ||
echo "Get the newest git commits" >&2 | ||
if [[ checkout_main_if_new ]]; 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.
The square brackets here are superfluous. Nowadays, this stuff is built-in for speed, but check out /bin
on a unixy box and you'll see an executable called [
. That's a pretty big clue about how this stuff works. It turns out that [
is just like /bin/test
; they share a man page, which is worth skimming. The other important thing to note is the ;
which terminates the conditional command, which can be a pipeline. Blah blah blah...
&& build_images > "${DOCKER_LOGFILE}" 2>&1 \ | ||
&& runtests > "${PYTEST_LOGFILE}" 2>&1 |
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'd indent these one more level to reduce confusion.
[[ ${passed_tests} = 0 ]] && touch "${RUNDIR}/passed" | ||
[[ ${passed_tests} != 0 ]] && touch "${RUNDIR}/failed" |
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...else would seem more clear here.
new_sha="$(git rev-parse FETCH_HEAD)" | ||
if [[ "${old_sha}" != "${new_sha}" ]]; then | ||
[[ -z $(git status --porcelain) ]] \ | ||
|| { echo "Working tree not clean" > "${GIT_LOGFILE}" && return 1; } |
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 does the semicolon do at the end? I see it a lot in this file.
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.
Not sure, I copied it from the stack overflow I found on grouping. Although I guess I could clean this up a lot by turning the -z into a -n and the || into && and ungrouping it. echo > file will never fail, right?
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
…name it Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
I guess |
@@ -0,0 +1,202 @@ | |||
#!/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.
why name this integration/integration from the start?
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.
wdym?
The apps/integration/integration comes from the python project name being equal to the directory name - that pattern is all over this repo.
TAG="integration_tests" | ||
while [[ $# -gt 0 ]]; do | ||
case "$1" in | ||
--help) |
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.
--help|-h)
#!/bin/bash | ||
|
||
# Parse args | ||
SKIP_BUILD=0 |
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.
generally prefer positive conditions, so
RUN_BUILD=1, etc.
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 it needs to be negative conditions. the logic I need is (if I want to be able to chain each step with &&'s) is:
if (action_condition) -> (action)
else -> true
with, e.g. DO_BUILD
, I have
{ [[ $DO_BUILD ]] && build; } \
&& { [[ $DO_TESTS ]] && tests; } \
&& etc...
but if DO_BUILD
is false this breaks my pipeline. With negative conditions I can do
{ [[ $SKIP_BUILD ]] || build; } \
&& { [[ $SKIP_TESTS ]] || tests; } \
&& etc...
which has the intended selectivity behavior. To get the right behavior with positive variable I think I need to negate them in the conditions, so SKIP seems cleaner to me
;; | ||
--tag) | ||
[[ -z $2 ]] && die "A tag must be speicified when using the --tag arg; e.g. --tag my-tag" | ||
[[ $2 == "--*" ]] && die "Detected tag was $2. Tags should not begin with --" |
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'd suggest tags should always start with lowercase, which is a stronger test. That would prevent --tag -h
QUERY_LOGFILE="${RUNDIR}/test_queries.log" | ||
|
||
main() { | ||
[[ ! -d ".git" ]] && die "Please run this script from sycamore root!" |
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.
prefer positive tests, so [[ -d ".git" ]] || die
echo "Changes detected. Running Tests" >&2 | ||
poetry install --no-root > "${POETRY_LOGFILE}" 2>&1 \ | ||
&& { [[ $SKIP_BUILD ]] || build_images > "${DOCKER_LOGFILE}" 2>&1; } \ | ||
&& { [[ $SKIP_TESTS ]] || runtests > "${PYTEST_LOGFILE}" 2>&1; } |
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'd do:
...
&& touch "${RUNDIR}/passed_tests"
[[ -f "${RUNDIR}/passed_tests" ]] || touch "${RUNDIR}/failed_tests"
If someone put an echo "done with tests" above the local passed_tests=$? line, it would make all the tests appear to pass in the current form.
} | ||
|
||
runtests() { | ||
docker system prune -f --volumes |
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 is a pretty big hammer; my guess is the purpose is to clean up integration test volumes, but it will also clean up build caches and other people's stuff.
I'd suggest:
docker volume rm integration_crawl_data integration_jupyter_data integration_opensearch_data
docker compose -p integration up reset
That will clean up integration specific volumes but leave everything else.
Optionally clean them after successful testing and verify there are no volumes named *integration*
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's not, as far as I can tell, a good way to get testcontainers to do project names...
echo "Successfully built using docker file $docker_file" | ||
} | ||
|
||
docker-push-hub() { |
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 run this on both arm & amd, what shows up in dockerhub? I was building both architectures in a single go because I thought that was necessary for it to show up cleanly (may not matter for integration testing)
|
||
docker-push-hub() { | ||
local docker_file="$1" | ||
[[ -n "${docker_file}" ]] || { error "missing ${docker_file}" && return 1;} |
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.
slightly safer to write { error "..."; return 1 }
That way if error returns an error you don't accidentally continue.
if (( $(wc -w <<< ${repo_name}) != 1 )); then | ||
echo "Unable to find repo name in ${docker_file}" 1>&2 | ||
exit 1 | ||
fi |
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.
[[ "${repo_name}" = *private* ]] && die "Private repo ${repo_name} disallowed"
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 we expect this? but ok
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
now
runtests.sh
also builds images for a tag specified at runtime and pushes them to dockerhub, using a registry cache (also in dockerhub)also push test results to s3 (
s3://sycamore-ci/<arch>/<datetime>
)