refactor Makefile and Dockerfile, and update README with better dev instructions #391
Conversation
* rework multi-stage Dockerfile build to run faster and remove unused directives * update README with better instructions for running
Dockerfile
Outdated
| # Any python libraries that require system libraries to be installed will likely | ||
| # need the following packages in order to build | ||
| RUN apt-get update && apt-get install -y build-essential git | ||
| RUN apt-get update && \ |
There was a problem hiding this comment.
this should be in the base stage, since it doesn't need to be re-executed each time and can be cached
Dockerfile
Outdated
|
|
||
| ENV CURL_CA_BUNDLE=/etc/ssl/certs/ca-certificates.crt | ||
|
|
||
| ARG install_dev_dependencies=true |
There was a problem hiding this comment.
this doesn't do anything AFAIK
There was a problem hiding this comment.
Yup, this was used before we switched away from pipenv.
Dockerfile
Outdated
| COPY . /app | ||
|
|
||
| ENV PATH=$PATH:/install/bin |
There was a problem hiding this comment.
/install doesn't seem to be used for anything? it gets added to the path and created, but nothing is ever done with it.
There was a problem hiding this comment.
It's used as a directory to hold the outputs of pip install -e but I don't think its needed. The upside is it makes the WORKDIR a little cleaner, downside is it makes the Dockerfile a little more complex. Personally I'm fine removing it.
There was a problem hiding this comment.
Could you explain to me how it's configured to be used as a directory to hold the pipe install -e outputs?
There was a problem hiding this comment.
Ah, I think I actually just stumbled upon the example where this dockerfile came from: https://blog.realkinetic.com/building-minimal-docker-containers-for-python-applications-37d0272c52f3
What's missing is that pip isn't being invoked with --install-option="--prefix=/install", so it's not actually being used. (I thought this might be the case, but there's a lot of implicit python magic I don't know about yet)
Makefile
Outdated
| .PHONY: docker-run | ||
| docker-run: image | ||
| $(run_docker) | ||
| .PHONY: docker-run-all |
There was a problem hiding this comment.
added this so all the commands can be done with make rather than docker-compose.
Makefile
Outdated
| docker-run-all: | ||
| docker-compose up | ||
|
|
||
| .PHONY: docker-run-sqlalchemy |
There was a problem hiding this comment.
explicitly renamed all the targets like docker-run to docker-run-sqlalchemy, since that's what it does.
Makefile
Outdated
|
|
||
| .PHONY: test-pgstac | ||
| test-pgstac: | ||
| $(run_pgstac) /bin/bash -c 'export && ./scripts/wait-for-it.sh database:5432 && cd /app/stac_fastapi/pgstac/tests/ && pytest -vvv --asyncio-mode=auto' |
There was a problem hiding this comment.
there's another PR where this gets added to pytest.ini, merge that PR first.
There was a problem hiding this comment.
actually, went ahead and reverted it.
stac_fastapi/pgstac/README.md
Outdated
| @@ -47,35 +46,16 @@ pip install -e \ | |||
| stac_fastapi/pgstac[dev,server] | |||
| ``` | |||
|
|
|||
| ## Local Development | |||
There was a problem hiding this comment.
duplicated from root Readme now.
Dockerfile
Outdated
| COPY . /app | ||
|
|
||
| ENV PATH=$PATH:/install/bin |
There was a problem hiding this comment.
It's used as a directory to hold the outputs of pip install -e but I don't think its needed. The upside is it makes the WORKDIR a little cleaner, downside is it makes the Dockerfile a little more complex. Personally I'm fine removing it.
If these changes are acceptable, we also need to figure out what should be in the Readme vs. Contributing vs. the published docs, and what should be in the root readme vs. the pgstac readme
Related Issue(s):
Description:
PR Checklist:
pre-commit run --all-files)make test)make docs)