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 healthcheck for divviup-api to compose.yaml #1097

Merged
merged 10 commits into from
Jun 13, 2024

Conversation

tgeoghegan
Copy link
Contributor

Adds a simple healthcheck on the divviup-api service in compose.yaml that tickles the health endpoint. Also adds a check to the Docker CI job that runs Docker compose and waits 120 seconds for it to become healthy.

Part of #1096

Adds a simple healthcheck on the `divviup-api` service in `compose.yaml`
that tickles the health endpoint. Also adds a check to the Docker CI job
that runs Docker compose and waits 120 seconds for it to become healthy.

Part of #1096
@tgeoghegan tgeoghegan requested a review from a team as a code owner June 13, 2024 14:32
@divergentdave
Copy link
Collaborator

I think the CI error here must be a permissions issue on the repositories. curl -i https://us-west2-docker.pkg.dev/v2/divviup-artifacts-public/janus/janus_aggregator/manifests/0.6.35 works for me, while curl -i https://us-west2-docker.pkg.dev/v2/divviup-artifacts-public/divviup-api/divviup_api/manifests/0.3.12 doesn't.

@tgeoghegan
Copy link
Contributor Author

tgeoghegan commented Jun 13, 2024

I know what it is -- we need to grant roles/artifactregistry.reader to allUsers on divviup-artifacts-public/divviup-api. I'm making a janus-ops change: https://github.com/divviup/janus-ops/pull/1833

@tgeoghegan
Copy link
Contributor Author

tgeoghegan commented Jun 13, 2024

There we go -- now I'm really glad to have added this test. I want to add a test for docker compose -f compose.dev.yaml and then we'll slam this into main.

Comment on lines 39 to 48
# continue on error so we can inspect containers in the next step
continue-on-error: true
- name: Inspect containers
if: steps.compose.outcome != 'success'
run: |
docker compose ps
for NAME in `docker compose ps --format json | jq -r '.Name'`; do
docker inspect $NAME
done
exit 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could simplify this by replacing the default if: success() condition on the last step, and getting rid of continue-on-error: true. See https://docs.github.com/en/actions/learn-github-actions/expressions#example-of-failure-with-conditions.

Suggested change
# continue on error so we can inspect containers in the next step
continue-on-error: true
- name: Inspect containers
if: steps.compose.outcome != 'success'
run: |
docker compose ps
for NAME in `docker compose ps --format json | jq -r '.Name'`; do
docker inspect $NAME
done
exit 1
- name: Inspect containers
if: ${{ failure() && steps.compose.outcome != 'success' }}
run: |
docker compose ps
for NAME in `docker compose ps --format json | jq -r '.Name'`; do
docker inspect $NAME
done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, cool, I wasn't aware of this advance in the state of the art of encoding program logic as YAML

@tgeoghegan
Copy link
Contributor Author

A few more changes here:

  • use David's superior suggestion for inspecting containers on failure
  • remix the jobs: we now test the regular compose (which should always pull images from remote repos) in its own job across the three OSes where we want this to work, and test compose.dev.yaml after building and loading the containers

@tgeoghegan
Copy link
Contributor Author

Turns out we can't easily test Docker Compose on macOS because those action runners don't and won't install Docker (actions/runner-images#2150). There are workarounds, but TBH I've hit my quota of jumping through hoops to ship code to Macs for the month, and in any case, I feel OK trusting that the macOS Docker will be able to boot a Linux VM and run our container.

Comment on lines 57 to 58
os: [ubuntu-latest, windows-latest]
runs-on: ${{ matrix.os }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I think this is missing shell: bash

Copy link
Contributor Author

@tgeoghegan tgeoghegan Jun 13, 2024

Choose a reason for hiding this comment

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

Yes, I'll try that but also I think the Docker in Windows images won't work:

image operating system "linux" cannot be used on this platform: operating system is not supported

actions/runner#904

@tgeoghegan tgeoghegan merged commit 5d3c317 into main Jun 13, 2024
8 checks passed
@tgeoghegan tgeoghegan deleted the timg/compose-healthcheck branch June 13, 2024 19:43
@tgeoghegan tgeoghegan mentioned this pull request Jun 13, 2024
9 tasks
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