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

opendbx support #9

Closed
wants to merge 6 commits into from
Closed

opendbx support #9

wants to merge 6 commits into from

Conversation

jchook
Copy link

@jchook jchook commented Jun 4, 2023

Closes #8

Adds opendbx support for mysql and postgres.

The uncompressed image size difference seems small, all things considered.

OS Previous Updated Difference
Debian 97.1M 98.7M +2.7%
Alpine 57.5M 59.4M +3.2%

@jchook jchook changed the title opendbx: mysql support opendbx support Jun 4, 2023
@jchook
Copy link
Author

jchook commented Jun 8, 2023

@tyranron This is ready to be merged IMO.

Unfortunately testing the db required a full end-to-end test, but I wrote one and it passes.

Happy to make any adjustments per your feedback.

Copy link
Member

@tyranron tyranron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jchook thanks for the amazing effort and work! ♥️

I've tuned a bit the Dockerfile to my preferring.

The only thing that concerns me (in terms of understanding and maintance), is that monstrous E2E test:

  1. It seems that using teardown function rather than a separate test for cleaning "mess" would be better. setup function also would be helpful here to decompose the test clearly.
  2. Could we use the docker-compose.yml to setup the test environment rather than juggling with containers and IPs via raw docker commands?

@jchook
Copy link
Author

jchook commented Jun 9, 2023

@jchook thanks for the amazing effort and work! hearts

Thank you for maintaining this excellent OpenDKIM docker!

I've tuned a bit the Dockerfile to my preferring.

Great work. Much cleaner now.

The only thing that concerns me (in terms of understanding and maintance), is that monstrous E2E test:

Unfortunately I think e2e is the only way to properly test this :(

  1. It seems that using teardown function rather than a separate test for cleaning "mess" would be better. setup function also would be helpful here to decompose the test clearly.

Yep makes sense. I was just unfamiliar with bats.

  1. Could we use the docker-compose.yml to setup the test environment rather than juggling with containers and IPs via raw docker commands?

It's certainly feasible.

Unfortunately the static IP assignment seems necessary to me because Docker's DNS feature does not provide a way to publish arbitrary TXT records (which is necessary for DKIM to function end-to-end). As a result, the DNS host is needed and, baring some fancy entrypoint nsupdate sorcery, known IPs reduce the moving parts.

I originally planned to use a docker-compose.yml approach, but opted for this instead.

To me it seems easier to maintain the sequential execution rather than letting docker compose try to start everything all at once and having to manage separate entrypoint scripts to wait for the database to provision, compose overrides for each db, etc. I also saw other tests used this method.

I'll await the failing test fixes, then update the bats test to use setup/teardown. Let me know your thoughts on docker compose.

@tyranron
Copy link
Member

tyranron commented Jun 12, 2023

@jchook

To me it seems easier to maintain the sequential execution rather than letting docker compose try to start everything all at once and having to manage separate entrypoint scripts to wait for the database to provision, compose overrides for each db, etc. I also saw other tests used this method.

This is solvable with Docker Compose. Just use manifest version "2.4" and healthcheck + depends_on: <service>: condition: service_healthy. This way you can make them start in the desired sequence.

However, regarding the static IP assignment in Docker Compose, I'm not sure if it's possible. Here you should look into docs. Usually, Docker Compose allows everything which raw Docker does.

@jchook
Copy link
Author

jchook commented Jul 4, 2023

I'm going to close this PR.

Feel free to rewrite the tests to your liking.

BTW you can also reduce your image size by ~50% if you combine the RUN statements together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenDKIM Database support
2 participants