fix: docker build #65
Conversation
|
I'd prefer that we did know exactly which changes are positively contributing to the fix, otherwise I'll feel that we're just guessing. From my point of view (on Linux), the only problem is that uvicorn was not installed in the container - installing it fixes that problem. I don't have a Windows machine, so I cannot tell what other issues exist beyond that. The The entrypoint script is intended to make the Google credentials file available to the app at the correct location.The credentials are currently required when running the app (via uvicorn) and when running the tests (via pytest). The script is intended to do its thing and then run whatever command is passed to the container, or the default command. If you really want to temporarily skip the entrypoint, I recommend using the The app directory mount in the Compose file is a convenience for developers, so that the container does not have to be rebuilt after every code change. The |
docker-compose.yaml
Outdated
| ###################### | ||
| # Debugging | ||
| ###################### | ||
|
|
||
| # entrypoint: /bin/bash -l -c "tail -f /dev/null" | ||
| # command: tail -f /dev/null | ||
| # volumes: | ||
| # - "./app:/app" | ||
| # - "./service-account.json:/app/service-account.json" |
There was a problem hiding this comment.
I'd prefer not to have commented directives. I recommend storing this in a docker-compose.override.yml file (which should be git ignored), if these are personal preferences. If they are intended to be used by others, I suggest creating a separate docker compose file - e.g. docker-compose.debug.yml.
There was a problem hiding this comment.
All good suggestions, I've moved over to the troubleshooting readme with 552c24e
Fair point. The suggestion was that all the proposed changes were positive contributions in one way or another, although good to take the case-by-case comments as you've outlined.
Possible, feel free if you want to open an alternate PR to just fix the core build and I can revert this to draft/follow-up. For now these changes do manage to unblock the work I need to get done on the picsa app, so not super time-sensitive (but would be nice to have a working build soon)
Oh, good to know. I think it would be better to use some alternate naming conventions to make it obvious when referring to app what layer we are currently in (given the possibility for both absolute and relative paths in code). If there is strong reason to keep
I understand the need for this, I'm just not entirely convinced this is the best way to go about it. The entrypoint is designed to be a gateway to the shell/binary/cli where commands should run from, e.g. running python which can take a name of script/args to execute. If we really want to use an entrypoint to configure the environment, I think best practice would be to also use that shell to trigger the python script
The build step from the code changes only take a couple seconds due to the docker cache layers, so I don't think this is significant. The place it could save time is if developers are running the api locally through the docker container, although if promoting this we should update the readme accordingly. Should also note that live-reload will not work in windows due to the way file change detection is propogated through containers (not tested myself on linux, would be interested to know if it does).
This comment seems to ignore what I wrote in the description, which is a little frustrating.
To re-iterate, when checking out a git repo on windows line endings are converted to |
|
I did not know that git converts line endings by default on Windows - thanks for explaining. I now fully understand why the Live reload works on Linux and Mac, so it would be best to keep the mount point and binding in place. I too would prefer not to use entrypoint, and I think the way to avoid it is to change the app so that the location of the credentials file is configurable (via env vars) and not hardcoded, as it is currently. However, I consider this to be out of scope. |
| **Docker container fails** | ||
| This can happen for many reasons, check the logs for any specific errors raised. | ||
|
|
||
| If troubleshooting the code in the container, it may be useful to bypass default entrypoint or commands so that the container can be kept running and inspected. | ||
| Typically this can be done by overriding the `docker-compose.yml`, such as: | ||
|
|
||
| ```yml | ||
| entrypoint: /bin/bash -l -c "tail -f /dev/null" | ||
| command: tail -f /dev/null | ||
| ``` |
There was a problem hiding this comment.
Sorry to go on about this - I'm not trying to torment you, honestly.
I would like to know how this helps with troubleshooting since I have never done anything like this before. What is the method?
I would like to avoid any suggestion that the Docker Compose file should be modified temporarily to accommodate a short-term interest, like debugging. The reason being to avoid accidental commits of overrides. I would like to make it very clear that short-term or personal overrides should go in docker-compose.override.yml.
There was a problem hiding this comment.
There's quite often times that I find a docker container fails and the messages reported not always very clear or actionable. A simple case like a python "module not found" error could be related to a file not being present, having incorrect permissions, or any number of other issues related to the code, dependencies etc. Usually as a starting point for debugging I various other potential issues which are more easily debugged by spinning up the container, looking at the files, and trying to execute commands from a shell within the container itself.
The trouble will this is if a container fails it immediately shuts down. If it has a restart policy it will try to restart, but fail again and shut down ad-infinitum.
So two tools I frequently find myself falling back on are:
- The ability to spin up a container and run a different command, with the sole purpose of getting it to stay running. There's various ways to do this depending on OS and shell in use, one link I have bookmarked for such occasions is:
https://kodekloud.com/blog/keep-docker-container-running/
Most of the time you can simply provide an override command in the docker-compose to do this. Although in the case above standard commands like cat and tail were not working due to a custom entrypoint being specified, meaning this needed override too (as in the troubleshooting example).
- The ability to remote into the container to view files and execute directly on the container. While this is possible through docker commands like
docker exec -it /bin/bash ...(https://kodekloud.com/blog/docker-exec/), I find it easier to use the vscode devcontainers extension to attach to a running container, from where you can view all files within the container and run commands
https://code.visualstudio.com/docs/devcontainers/attach-container
Debugging is indeed a short term interest, but here I think the priority should be on efficiency more than isolation. I would say most developers tend to lean into things like adding console logs and temporary file overrides as part of the debug process, and it is routine to rely on developer competencies, review processes and automated testing to try make sure things that shouldn't be included in production don't get committed (same goes for most code in general). I think this is better than trying to setup temporary environments just for debugging, as even with a dedicated compose override developers will still need to make ongoing modifications to production files while troubleshooting. If we need more strict safeguards I would recommend looking more into linting or other CI automation that can flag issues as part of review process.
There was a problem hiding this comment.
It is not clear enough that the intention is to use 'exec' or something like devcontainers once the container is running. I understand the advice you have given as your personal preference for debugging, in which case, those changes should be kept in your override file. I think it is worthwhile to encourage people to use the override file first, then, if the change is generally applicable, to change the default Compose file.
As you have noted, there are several ways to keep a container running indefinitely. When I need to inspect a container I tend to use a run command. For this project, it would be:
docker compose run --rm app bash
This keeps the container running, so I assume that it would also be compatible with the devcontainers extension. If so, there is no need to change the Docker Compose file for the sake of debugging. If the entrypoint also needs to be overriden, the --entrypoint flag will do that.
There was a problem hiding this comment.
Can you please try to clarify which of your comments are blocking on this PR vs not. I'm not sure whether you are strongly opposed to me having that text in the readme or not, and am quite keen to get some form of PR merged to unblock the main build.
Alternatively if you are able to open a PR with just the changes you deem necessary I'd be happy to review, merge and deal with whatever other follow-up in a separate PR
|
I intended to extract the essential changes that fix the Docker issues and create a new PR, but I have ended up pushing those changes directly to main. This is an honest mistake on my part - sorry about that. Nevertheless, would you check that the main branch works for you. Please raise a new issue or PR to address any remaining problems. If you agree, I will close this PR. |
…limate-api into fix/docker-build
No worries, it happens. I checked out out main and for the most part worked, just needed to install |
WORKDIRfrom dockerfile to fix issue related tohttps://github.com/fastapi/fastapi/issues/1988on windowsapp/appfolder structuresENTRYPOINTscript in favour of existingCMD(could see argued either way, but in practice using the entrypoint instead of leaving standard bash would limit what binaries available to subsequent CMD)uvicornto explicit list of requirements@istride
I'm not sure which of the updates above are strictly required to fix the current build, just that the combined fixes appear to be working as per the test action run I triggered via https://github.com/IDEMSInternational/epicsa-climate-api/actions/runs/12720672214?pr=65
Should also note that tests fail independently of this code, as the test scripts are currently used to quality-check production data which itself has issues. Recommend decoupling these two types of testing in the future, but beyond scope of this PR