-
Notifications
You must be signed in to change notification settings - Fork 345
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 runtime check, runtime logs and build logs #293
Conversation
Happy to review! I see you're still pushing some things. Give me a heads up when it's ready |
Hey, Yeah. Still working through. I'll ping once it's ready for your review 🙌 |
@alishaz-polymath one quick thought, for convenience you could create another docker-compose.yml (maybe in a /test folder? a docker-compose.test.yaml maybe), which provides a different |
@krumware Thank you, I'll give it a go tomorrow, pretty late here 😅 |
Hey @krumware Can you please review? I think we can revisit the suggestion you left earlier for maybe next week (I will take it up next week), but if you could review the rest, it would be super awesome 🙌 |
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.
Self Review added
|
||
|
||
echo "Waiting for the server to start..." | ||
sleep 120 |
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 give 120 seconds for the server to be ready to accept curl request. It was erroring out with code 7 if we requested when it wasn't ready.
for i in {1..60}; do | ||
echo "Checking server health ($i/60)..." | ||
response=$(curl -o /dev/null -s -w "%{http_code}" ${{ env.NEXT_PUBLIC_WEBAPP_URL }}/auth/login) | ||
echo "HTTP Status Code: $response" | ||
if [[ "$response" == "200" ]] || [[ "$response" == "307" ]]; then | ||
echo "Server is healthy" | ||
# Now, shutdown the server | ||
kill $server_pid | ||
exit 0 | ||
fi | ||
sleep 1 | ||
done |
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 every second until true
if we can access /auth/login
, and we tend to currently redirect it to /auth/setup
but even that is a good indication of server being functional, so we accept both 200 and 307 to cover this.
NEXTAUTH_SECRET: 'EI4qqDpcfdvf4A+0aQEEx8JjHxHSy4uWiZw/F32K+pA=' | ||
CALENDSO_ENCRYPTION_KEY: '0zfLtY99wjeLnsM7qsa8xsT+Q0oSgnOL' |
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'm not entirely sure if it's better to pass these from env.<...>
but this works well.
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's probably save since these are just arbitrary test values in this case
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.
Can we split the build and push into two steps that sandwich the test step? It should work pretty close to the way you have it now. That will also let you run the tests against the built image instead of the hardcoded docker.io/calcom/cal.com:add-runtime-check
, then we can prevent the push if the test fails. Currently it will push before it fails.
See for an example:
- name: Build and export to Docker
uses: docker/build-push-action@v5
with:
context: .
load: true
tags: ${{ steps.meta.outputs.tags }}
- name: Test
run: |
docker run --rm ${{ steps.meta.outputs.tags[0] }}
- name: Build and push
uses: docker/build-push-action@v5
with:
context: .
platforms: linux/amd64
push: true
tags: ${{ steps.meta.outputs.tags }}
(full disclosure, I can't remember if I'm selecting the first tag in the array of tags properly in that docker run step)
modified from https://docs.docker.com/build/ci/github-actions/test-before-push/
Oh nice catch indeed! I'll take it up tomorrow morning 🙏 |
- name: Push image | ||
run: | | ||
tags="${{ steps.meta.outputs.tags }}" | ||
IFS=',' read -ra ADDR <<< "$tags" # Convert string to array using ',' as delimiter | ||
tag=${ADDR[0]} # Get the first tag | ||
|
||
docker push $tag |
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 use docker push
to push the built image and avoid re-building it after the runtime check is complete
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 like to check in a future PR if it's better to reuse the docker/build-push-action like in the example. It should't rebuild but that isn't clear in the docs I linked.
# Uncomment below to allow specific version workflow run | ||
# inputs: | ||
# version: | ||
# description: 'Version to build' | ||
# required: true |
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 might be beneficial in the future to test the workflow run against specific versions for a more targeted debugging
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.
good with adding this commented. would be good to add an additional input to toggle pushing as well here
@krumware Gentle nudge for review 🙏 |
Sorry for delay, travelling. Looks good, comments included can be addressed in the future. Thanks! |
This PR attempts to add a runtime check as well as runtime logs and build logs to the
docker-build-push-dockerhub.yml
workflow