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

Improve Docker and compose definitions #861

Merged
merged 3 commits into from
Aug 3, 2024

Conversation

hanefi
Copy link
Contributor

@hanefi hanefi commented Aug 2, 2024

This is a PR that contains several improvements of our Docker infra.

Below is a detailed overview of all the changes covered in sections:

  1. Remove versions of compose files

We are moving away from the legacy version of compose files. This commit
removes the version key from the compose files as it is obsolete.

See https://docs.docker.com/compose/compose-file/04-version-and-name/


  1. Rename compose files to compose.yaml

Excerpt from Docker documentation states:

The default path for a Compose file is compose.yaml (preferred) or
compose.yml that is placed in the working directory. Compose also
supports docker-compose.yaml and docker-compose.yml for backwards
compatibility of earlier versions. If both files exist, Compose prefers
the canonical compose.yaml.


  1. Use exec form of Dockerfile CMD instructions

This change updates the Dockerfile CMD instructions to use the exec form
instead of the shell form. This is a best practice for Dockerfiles as it
avoids the overhead of running a shell process.

One downside of shell form is that it does not pass signals to the
process. This can cause the process to not shut down cleanly when
stopping the container. The exec form does not have this issue.

We are moving away from the legacy version of compose files. This commit
removes the version key from the compose files as it is obsolete.

See https://docs.docker.com/compose/compose-file/04-version-and-name/
Excerpt from Docker documentation states:

The default path for a Compose file is compose.yaml (preferred) or
compose.yml that is placed in the working directory. Compose also
supports docker-compose.yaml and docker-compose.yml for backwards
compatibility of earlier versions. If both files exist, Compose prefers
the canonical compose.yaml.
This change updates the Dockerfile CMD instructions to use the exec form
instead of the shell form. This is a best practice for Dockerfiles as it
avoids the overhead of running a shell process.

One downside of shell form is that it does not pass signals to the
process. This can cause the process to not shut down cleanly when
stopping the container. The exec form does not have this issue.
@hanefi
Copy link
Contributor Author

hanefi commented Aug 2, 2024

Summary: I used to have 4 commits here. I am removing one as it does not look ready.

Details:

Now that the shared volumes are managed by docker, host OS can not directly set file mode bits to fix permission errors.

The fact that we need to do a chmod a+rwx tests/*/workdir in our test suite indicate that we do a poor job managing users in the docker containers. To be able to fix shared volumes, I need to first fix the users and privileges in the containers.

For the time being, I am removing the fix on docker volumes from this PR. I hope I can fix that in a future PR instead.

@hanefi hanefi requested a review from dimitri August 2, 2024 23:16
@dimitri dimitri merged commit db08b75 into dimitri:main Aug 3, 2024
19 checks passed
@hanefi hanefi deleted the improve-docker-usage branch August 3, 2024 11:44
@hanefi hanefi self-assigned this Aug 4, 2024
VaibhaveS pushed a commit to VaibhaveS/pgcopydb that referenced this pull request Aug 5, 2024
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.

2 participants