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 test of dev container #13

Merged
merged 31 commits into from
May 29, 2024
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
7f0df2a
Use same operating system consistently
StevenMaude May 3, 2024
7af340c
Load the Docker image by file instead of stdin
StevenMaude May 3, 2024
4e47a26
Speed up saving image
StevenMaude May 3, 2024
e82d27f
Use faster compression option
StevenMaude May 3, 2024
cb4f241
Add minimal test for dev container
StevenMaude May 3, 2024
45f8788
Check Python packages in dev container
StevenMaude May 16, 2024
11be6ca
Check Docker image version in `devcontainer.json`
StevenMaude May 9, 2024
7b45371
Strip `devcontainer.json` comments more robustly
StevenMaude May 15, 2024
339bca7
Consistently double quote shell variables
StevenMaude May 16, 2024
313353c
Check R packages in dev container
StevenMaude May 15, 2024
3cc0dc0
Remove unused `__init__.py`
StevenMaude May 16, 2024
d884920
Rename variables for consistency
StevenMaude May 16, 2024
de2d467
Rename workflow step for clarity
StevenMaude May 21, 2024
9557043
Pin `devcontainers/ci` action to a commit
StevenMaude May 23, 2024
275e04b
Split package checks out from dev container checks
StevenMaude May 23, 2024
596189f
Remove explicit use of `opensafely` from test
StevenMaude May 23, 2024
b692acf
Add scheduled check of research-template container
StevenMaude May 23, 2024
9fd4e1e
Add comment to workflow explaining test setup
StevenMaude May 23, 2024
8280ee1
Add comment to test script
StevenMaude May 23, 2024
8a93100
Amend comment
StevenMaude May 23, 2024
fe2b23e
Tag Docker image for testing with `dev`
StevenMaude May 23, 2024
b217131
Rework package test to work without dev container
StevenMaude May 23, 2024
35b169d
Rename `justfile` test recipes
StevenMaude May 23, 2024
3d95ba6
Move R Studio tests out of dev container checks
StevenMaude May 28, 2024
1b64daa
Remove tagging of image
StevenMaude May 28, 2024
69e9483
Check Docker image exists before running any test
StevenMaude May 28, 2024
07dd6f6
Amend `docker load` to `docker image load`
StevenMaude May 29, 2024
0b37773
Use consistent naming in workflows
StevenMaude May 29, 2024
07f66f8
Move smoke test to a check of Python for ehrQL
StevenMaude May 29, 2024
f7426e2
Rename R Studio test for consistency
StevenMaude May 29, 2024
4153dce
Add rudimentary check for ehrQL in dev container
StevenMaude May 29, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .github/workflows/dev_container.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
name: Check research template dev container functions
StevenMaude marked this conversation as resolved.
Show resolved Hide resolved

on:
schedule:
- cron: "18 8 * * *"
workflow_dispatch:

jobs:
dev-container:
runs-on: ubuntu-latest
steps:
- name: Checkout research-template repository
uses: actions/checkout@v4
with:
repository: 'opensafely/research-template'
Comment on lines +12 to +15
Copy link
Member

Choose a reason for hiding this comment

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

Checking out and running opensafely/research-template seems like unnecessarily tight coupling to me. What are the advantages over, say, checking in a simple dataset_definition.py to this repo and calling opensafely exec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal of this test is to check end-to-end that the dev container setup in research-template runs (which is a little indirect, I agree).

However, it was suggested originally to keep this tests here rather than in the research-template repository.

The reason to checkout the repository is so that we build a dev container from the .devcontainer configuration there.

That said, I'm not sure if it's necessary to run the full research-template project (which is also tested when on PRs to that repository).

The only real extra pieces of installation that happen in the research-template .devcontainer configuration are:

  • the OpenSAFELY CLI
  • ehrQL for autocompletion

So maybe it's sufficient to check those are installed correctly (by running and importing respectively).

(It's also possible that those could move in future to here.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open to this being moved or restructured, but maybe that's the subject of a subsequent PR.


- name: Checkout research-template-docker repository in subdirectory
uses: actions/checkout@v4
with:
path: 'research-template/research-template-docker'

- name: Build and run dev container task
StevenMaude marked this conversation as resolved.
Show resolved Hide resolved
uses: devcontainers/ci@a56d055efecd725e8cfe370543b6071b79989cc8 # v0.3.1900000349
with:
runCmd: ./research-template/research-template-docker/tests/dev_container.sh
53 changes: 42 additions & 11 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ name: CI

env:
IMAGE_NAME: research-template
IMAGE_VERSION: v0
PUBLIC_IMAGE_NAME: ghcr.io/opensafely-core/research-template
REGISTRY: ghcr.io

Expand Down Expand Up @@ -30,23 +31,53 @@ jobs:
- name: Build docker image
run: just build

- name: Test docker image
run: just smoke-test
- name: Smoke test docker image
run: just test-smoke

- name: Save docker image
run: |
docker save research-template | gzip > /tmp/research-template.tar.gz
docker save research-template | pigz --fast > /tmp/research-template.tar.gz

- name: Upload docker image
uses: actions/upload-artifact@v4
with:
name: research-template-image
path: /tmp/research-template.tar.gz
# Disable compression; the file is already compressed
compression-level: 0

publish:
test-docker-image:
StevenMaude marked this conversation as resolved.
Show resolved Hide resolved
needs: [build-and-test]
runs-on: ubuntu-latest

steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Install just
uses: "opensafely-core/setup-action@v1"
with:
install-just: true

- name: Download Docker image
uses: actions/download-artifact@v4
with:
name: research-template-image
path: /tmp/image

runs-on: ubuntu-22.04
- name: Import Docker image
run: docker load --input /tmp/image/research-template.tar.gz
StevenMaude marked this conversation as resolved.
Show resolved Hide resolved

- name: Check packages
StevenMaude marked this conversation as resolved.
Show resolved Hide resolved
run: just test-packages

- name: Check R Studio installation
StevenMaude marked this conversation as resolved.
Show resolved Hide resolved
run: just test-rstudio

publish:
needs: [build-and-test, test-docker-image]

runs-on: ubuntu-latest

permissions:
contents: read
Expand All @@ -67,12 +98,12 @@ jobs:
path: /tmp/image

- name: Import docker image
run: gunzip -c /tmp/image/research-template.tar.gz | docker load
run: docker load --input /tmp/image/research-template.tar.gz
StevenMaude marked this conversation as resolved.
Show resolved Hide resolved

- name: Publish image
run: |
echo ${{ secrets.GITHUB_TOKEN }} | docker login $REGISTRY -u ${{ github.actor }} --password-stdin
docker tag $IMAGE_NAME $PUBLIC_IMAGE_NAME:v0
docker tag $IMAGE_NAME $PUBLIC_IMAGE_NAME:latest
docker push $PUBLIC_IMAGE_NAME:v0
docker push $PUBLIC_IMAGE_NAME:latest
echo ${{ secrets.GITHUB_TOKEN }} | docker login "$REGISTRY" -u ${{ github.actor }} --password-stdin
docker tag "$IMAGE_NAME" "$PUBLIC_IMAGE_NAME:$IMAGE_VERSION"
docker tag "$IMAGE_NAME" "$PUBLIC_IMAGE_NAME:latest"
docker push "$PUBLIC_IMAGE_NAME:$IMAGE_VERSION"
docker push "$PUBLIC_IMAGE_NAME:latest"
StevenMaude marked this conversation as resolved.
Show resolved Hide resolved
14 changes: 12 additions & 2 deletions justfile
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,15 @@ default:
build:
docker build . -t research-template

smoke-test:
docker run --rm research-template ls /opt/venv/bin/python3.10
check-image-exists:
# Extra brackets are for Just escaping.
Comment on lines +10 to +12
Copy link
Member

Choose a reason for hiding this comment

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

If the image didn't exist, then wouldn't Docker tell us? What's the benefit of this check?

Copy link
Contributor Author

@StevenMaude StevenMaude May 29, 2024

Choose a reason for hiding this comment

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

If you do docker run and the image doesn't exist, Docker will tell you.

However, Docker will then immediately try and pull an arbitrary research-template image from the internet, which isn't at present registered, but someone could do so. There is no way that Docker gives you to disable this.

I dislike this behaviour a lot. So this is a preventative measure to ensure that we only run a local image.

The alternative would be to refer to a local image with a name that we do control like ghcr.io/opensafely-core/research-template and give it a dev version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As elsewhere, I'm not too tied to this specifically, but I'm inclined to merge this now as it's been open a while, and get the benefit of having the tests running.

If we decide this could work better, this is OK, and would be a reasonable follow-up PR.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for clarifying. That is, indeed, very dislikable behaviour.

docker image inspect --format='{{{{.Id}}}}' research-template

test-smoke: check-image-exists
StevenMaude marked this conversation as resolved.
Show resolved Hide resolved
docker run --rm research-template ls /opt/venv/bin/python3.10

test-packages: check-image-exists
tests/packages.sh

test-rstudio: check-image-exists
docker run -i --entrypoint /bin/bash research-template < ./tests/r_studio.sh
Empty file removed tests/__init__.py
Empty file.
8 changes: 8 additions & 0 deletions tests/dev_container.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#!/bin/bash
set -euxo pipefail

# Check the OpenSAFELY research-template example runs.
opensafely run run_all

# Check the RStudio server is running.
curl -L 'http://localhost:8787' | grep 'RStudio'
54 changes: 54 additions & 0 deletions tests/packages.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
#!/bin/bash
set -euxo pipefail

# This test requires:
# * Docker
# * the availability of the research-template Docker image
research_template_image="research-template"
docker image inspect "$research_template_image" > /dev/null

# These checks assume that the packages do not change
# between building the Docker image in this repository,
# and checking the packages against the OpenSAFELY R and Python images.

# Check that the R packages in the OpenSAFELY R image are available in the dev container.
# The R setup we have leads to two sources of packages.
# As a result, this only compares package names;
# some packages are installed multiple times with different version numbers.
# See https://github.com/opensafely-core/research-template-docker/issues/22

# These packages are known to be extra to the local installation.
r_installed_known_extra_packages=$(cat <<-END
"docopt"
"littler"
"spatial"
END
)

# This function expects a CSV format with header line:
# "Package","Version"
# "SomeRPackage","1.0"
extract_quoted_package_names_from_csv() {
tail -n +2 | \
cut -d ',' -f1 | \
sort -u
}

r_docker_packages=$(curl 'https://raw.githubusercontent.com/opensafely-core/r-docker/main/packages.csv' |
extract_quoted_package_names_from_csv)
r_installed_packages=$(docker run -i "$research_template_image" /bin/bash -c "Rscript -e 'write.csv(installed.packages()[, c(\"Package\", \"Version\")], row.names = FALSE)'" |
extract_quoted_package_names_from_csv)
r_packages_extra_to_local_install=$(comm -13 <(echo "$r_docker_packages") <(echo "$r_installed_packages"))

[ "$r_packages_extra_to_local_install" == "$r_installed_known_extra_packages" ]

# Check that the Python packages in the OpenSAFELY Python image are available in the dev container.
# This checks package names and versions.
python_image_version='v2'
python_image="ghcr.io/opensafely-core/python:$python_image_version"

docker pull "$python_image"

python_docker_packages=$(docker run "$python_image" python -m pip freeze)
python_installed_packages=$(docker run "$research_template_image" /opt/venv/bin/python3.10 -m pip freeze)
diff <(echo "$python_docker_packages") <(echo "$python_installed_packages")
14 changes: 14 additions & 0 deletions tests/r_studio.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#!/bin/bash
set -euxo pipefail

# Check that we are the rstudio user.
[ "$(whoami)" == 'rstudio' ]

# Check the RStudio server installation.
sudo rstudio-server verify-installation
sudo rstudio-server start

# Check the RStudio server is running.
curl -L 'http://localhost:8787' | grep 'RStudio'

sudo rstudio-server stop
Loading