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 image #526

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

edde746
Copy link
Contributor

@edde746 edde746 commented Dec 4, 2023

  • Removes unnecessary http_server.js
  • Reduces size from ~970 MB to 18 MB

Bechmark with 100k requests:

Before

Summary:
  Total:	14.0344 secs
  Slowest:	0.0427 secs
  Fastest:	0.0006 secs
  Average:	0.0070 secs
  Requests/sec:	7125.3674

After

Summary:
  Total:	4.8001 secs
  Slowest:	0.0404 secs
  Fastest:	0.0002 secs
  Average:	0.0024 secs
  Requests/sec:	20832.8811

@edde746 edde746 force-pushed the improve-docker-image branch from a3cf12c to 6a14025 Compare December 4, 2023 19:34
@unclekingpin unclekingpin self-requested a review December 8, 2023 15:10
Copy link
Contributor

@unclekingpin unclekingpin left a comment

Choose a reason for hiding this comment

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

Nice! The only difference is that we dont have a custom 404 page.

Copy link
Contributor

@unclekingpin unclekingpin left a comment

Choose a reason for hiding this comment

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

actually we need the 404 handler for sure, which we will customize later

@edde746
Copy link
Contributor Author

edde746 commented Dec 8, 2023

The current dockerfile does not have a custom 404 page either, it just has a comment saying it should be added later. To add a custom 404 page, a 404.html would need to be created and passed as the fallback argument to goStatic.

@prom3theu5
Copy link

Have you considered using caddy instead?
That way you can make it super super simple for people to also auto gen tls etc?
https://caddyserver.com/

Also has a 17mb base image size for caddy
https://hub.docker.com/_/caddy

@edde746
Copy link
Contributor Author

edde746 commented Jan 2, 2024

TLS should be handled by the user IMO, gives them control of how they want to do it. The purpose of the Docker image should just be to serve the static files and nothing more.

As for the size, while ~16 MB difference does not matter, the base size of pierrezemb/gostatic is only ~2 MB. 18 MB is for goStatic + the static files.

@prom3theu5
Copy link

prom3theu5 commented Jan 2, 2024

While I agree with the statement TLS should be handled by the user, I have to add it still would be. They'd still have to mount a caddy file to enable it, configure their load balancing, cache settings, rewrites etc if they wanted to opt into the features. The point is they'd have the option to.

Just gives users who are hosting in something like azure container apps / instances, where they probably wouldn't want to pay for azure gateway or introduce a second container instance as a load balancer the ability to have easily opt-in features, with the use of a single file server rather than a static file server reverse proxied by a second file server.

That's ok I did say "considered" 😛

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.

3 participants