DM-46257: Add Docker support for ppdb-replication #3
Conversation
JeremyMcCormick
force-pushed
the
tickets/DM-46257
branch
19 times, most recently
from
702d3f2 to
f87628b
Compare
JeremyMcCormick
force-pushed
the
tickets/DM-46257
branch
3 times, most recently
from
3620429 to
9449163
Compare
JeremyMcCormick
changed the title
JeremyMcCormick
force-pushed
the
tickets/DM-46257
branch
5 times, most recently
from
147abf0 to
b45406f
Compare
JeremyMcCormick
force-pushed
the
tickets/DM-46257
branch
2 times, most recently
from
ba21a7e to
5437383
Compare
JeremyMcCormick
force-pushed
the
tickets/DM-46257
branch
4 times, most recently
from
238ef40 to
10340e9
Compare
JeremyMcCormick
force-pushed
the
tickets/DM-46257
branch
5 times, most recently
from
7534446 to
b48e754
Compare
JeremyMcCormick
force-pushed
the
tickets/DM-46257
branch
from
b48e754 to
9b710c6
Compare
docker/Dockerfile.replication
Outdated
| WORKDIR /app | ||
|
|
||
| # Install sdm_schemas | ||
| RUN git clone https://github.com/lsst/sdm_schemas.git |
There was a problem hiding this comment.
Maybe add --depth=0 option to avoid cloning full history?
Just a general comment for the future - we may need to install a specific version of sdm_schemas that matches the version of the database schema. It may be even better to delay or avoid cloning sdm_schemas into the image, to be able to use the same image with different schema version. Something to work out later.
There was a problem hiding this comment.
Maybe add
--depth=0option to avoid cloning full history?
Good idea - will do.
Just a general comment for the future - we may need to install a specific version of
sdm_schemasthat matches the version of the database schema. It may be even better to delay or avoid cloning sdm_schemas into the image, to be able to use the same image with different schema version. Something to work out later.
I improved the Dockfile so that it has an SDM_SCHEMAS_REF variable with the branch of sdm_schemas to use. By default, it will use main. Right now, we can't change this in the GitHub Action (it doesn't support setting environment variables for the Docker build), but the version can be updated directly in Dockerfile.replication for development if another ref than main should be used.
| [ -n "${PPDB_REPLICATION_APDB_CONFIG:-}" ] && _CMD="$_CMD $PPDB_REPLICATION_APDB_CONFIG" | ||
| [ -n "${PPDB_REPLICATION_PPDB_CONFIG:-}" ] && _CMD="$_CMD $PPDB_REPLICATION_PPDB_CONFIG" |
There was a problem hiding this comment.
These two must be non-empty, no need to check them again?
| # Check the exit status of the command | ||
| status=$? | ||
| if [ $status -ne 0 ]; then | ||
| echo "Command failed with exit status $status" | ||
| exit $status | ||
| fi |
There was a problem hiding this comment.
With set -e if $_CMD fails then the shell will exit immediately, this if is only ever executed if status=0.
There was a problem hiding this comment.
Good point, will remove it.
JeremyMcCormick
force-pushed
the
tickets/DM-46257
branch
5 times, most recently
from
ed041ae to
5480e09
Compare
This will run the ppdb-replication command, which is installed through pip. The entrypoint script used to run the command will convert the PPDB environment variables into the appropriate command line arguments.
This only builds the Docker image for ppdb-replication. Add additional steps to build other Docker images.
JeremyMcCormick
force-pushed
the
tickets/DM-46257
branch
from
5480e09 to
c0ce551
Compare
No description provided.