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

Build optimizations #643

Merged
merged 1 commit into from
Nov 13, 2023
Merged

Build optimizations #643

merged 1 commit into from
Nov 13, 2023

Conversation

am97
Copy link
Contributor

@am97 am97 commented Aug 4, 2023

wait-for-it is a widely used script for checking if a service is responding on a given port.

This PR reduces the uncompressed size of maxking/mailman-web from 248MB to 229MB, and simplifies the code a bit.

I tested it with Postgres and MariaDB

@am97 am97 force-pushed the main branch 3 times, most recently from 60c52ec to b2a9f93 Compare August 4, 2023 17:17
Before this commit, if docker-entrypoint or any file in mailman-web changed, the Docker build cache would not be used, so the install of dependencies would run again, uselessly
@am97
Copy link
Contributor Author

am97 commented Oct 11, 2023

Hello @maxking , I rebased my PR on main, it is ready to be merged. Let me know if you're not interested in optimizations ;)

@maxking
Copy link
Owner

maxking commented Nov 9, 2023

Hey @am97, thanks for taking time to work on this :-)

I would like to avoid depending on yet-another script for code that is already there. More dependencies tend to bite in the future and existing code to check for server is simple enough that I don't mind maintaining it.

The optimizations are nice, if you can remove the changes for wait-for-it, we can go ahead with merging this.

@am97 am97 changed the title Use wait-for-it script instead of postgresql-client and mysql-client Build optimizations Nov 10, 2023
@am97
Copy link
Contributor Author

am97 commented Nov 10, 2023

Ok no problem, I left just the optimizations so I changed the title of the PR.

Are you still open to the following in a separate PR ?

	# TODO: Use python3's psycopg2 module to do this in python3 instead of
	# installing postgres-client in the image.

@maxking
Copy link
Owner

maxking commented Nov 12, 2023

Yeah, we can do that. Although, i've also been thinking if we can just use composes' new startup order feature instead of doing it manually in the entrypoint script (although, maybe outside of compose, there is some value in having the checks in entrypoint script)

https://docs.docker.com/compose/startup-order/

@maxking maxking merged commit d97abcd into maxking:main Nov 13, 2023
2 checks passed
@am97
Copy link
Contributor Author

am97 commented Nov 17, 2023

Yeah, we can do that. Although, i've also been thinking if we can just use composes' new startup order feature instead of doing it manually in the entrypoint script (although, maybe outside of compose, there is some value in having the checks in entrypoint script)

https://docs.docker.com/compose/startup-order/

That sounds better at first sigh, although it has some drawbacks:

  • The Compose v2 plugin would be required
  • It won't work in Swarm mode, which is what I'm using to deploy this image (there doesn't seem to be a lot of people using Swarm mode however, and I can adapt)

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