Skip to content
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

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Conversation

HenryL27
Copy link
Collaborator

@HenryL27 HenryL27 commented Apr 12, 2024

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>)

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>
@HenryL27 HenryL27 requested a review from eric-anderson April 12, 2024 17:25
@HenryL27 HenryL27 changed the title Add image building and tagged testing to CO Add image building and tagged testing to CI Apr 12, 2024
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
Copy link
Contributor

@alexaryn alexaryn left a 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")"
Copy link
Contributor

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"
Copy link
Contributor

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)

Copy link
Collaborator Author

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

Comment on lines 11 to 15
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"
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a $?

Copy link
Collaborator Author

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?)

Copy link
Contributor

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() {
Copy link
Contributor

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.

Copy link
Collaborator Author

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

Copy link
Contributor

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}')"
Copy link
Contributor

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?).

Copy link
Contributor

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')"
Copy link
Contributor

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>.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...why?

Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Collaborator

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).

Copy link
Contributor

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}')"
Copy link
Contributor

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}"
Copy link
Contributor

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?

Copy link
Collaborator

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.

Copy link
Contributor

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
Copy link
Collaborator

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

Copy link
Contributor

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
Copy link
Collaborator

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.

Copy link
Collaborator Author

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}"
Copy link
Collaborator

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"

Copy link
Collaborator Author

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 ||?

Copy link
Collaborator Author

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'

Copy link
Contributor

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.

Copy link
Collaborator Author

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"
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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}"
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

Copy link
Contributor

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}')"
Copy link
Collaborator

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>
@alexaryn
Copy link
Contributor

Is it time to rename runtests.sh to something more reflective of it's current effects? Also, if we drop the suffix, we'd be free to rewrite this in Python without identifying and changing its callers.

@HenryL27
Copy link
Collaborator Author

you know I love rewriting bash into python!

Copy link
Contributor

@alexaryn alexaryn left a 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
Copy link
Contributor

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...

Comment on lines 25 to 26
&& build_images > "${DOCKER_LOGFILE}" 2>&1 \
&& runtests > "${PYTEST_LOGFILE}" 2>&1
Copy link
Contributor

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.

Comment on lines 70 to 71
[[ ${passed_tests} = 0 ]] && touch "${RUNDIR}/passed"
[[ ${passed_tests} != 0 ]] && touch "${RUNDIR}/failed"
Copy link
Contributor

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; }
Copy link
Contributor

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.

Copy link
Collaborator Author

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?

HenryL27 added 3 commits May 3, 2024 09:09
Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
…name it

Signed-off-by: Henry Lindeman <hmlindeman@yahoo.com>
@HenryL27
Copy link
Collaborator Author

HenryL27 commented May 3, 2024

I guess git mv and edit a file in one commit doesn't do the right thing w.r.t. comments. ugh, sorry

@@ -0,0 +1,202 @@
#!/bin/bash
Copy link
Collaborator

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?

Copy link
Collaborator Author

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)
Copy link
Collaborator

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
Copy link
Collaborator

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.

Copy link
Collaborator Author

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 --"
Copy link
Collaborator

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!"
Copy link
Collaborator

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; }
Copy link
Collaborator

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
Copy link
Collaborator

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*

Copy link
Collaborator Author

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() {
Copy link
Collaborator

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;}
Copy link
Collaborator

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
Copy link
Collaborator

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"

Copy link
Collaborator Author

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

HenryL27 added 7 commits May 7, 2024 10:25
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>
HenryL27 added 6 commits May 10, 2024 11:28
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants