gatsbyjs / gatsby-docker

Docker image that builds and hosts a Gatsby site
MIT License
356 stars 88 forks source link

Improve Docker image #26

Closed kanolato closed 4 years ago

kanolato commented 5 years ago

When im using Docker i like that i dont have to install anything else but Docker. I thought that it would be nice to build the project in gatsbyjs/gatsby:onbuild and then just copy the public dir to gatsbyjs/gatsby. Now as the documentation says the :onbuild tag builds the project and :latest serves it. Also i first tried to use the image as the documentation but the :onbuild tag lacks of nginx rules and every request gets a 500.

25

polarathene commented 5 years ago

Ok. So these changes create this flow:

Why was the port changed from 80 to 8080? Your related commit says due to "practicity" What does that mean?

This change while nice, will prevent existing users of the current workflow from being able to avoid the build within the container and debugging anything that may go wrong. Perhaps this contribution should rather compliment the existing offering?

This doesn't solve #25, as that's about a development image. One that doesn't require building an image locally for every change.

Also i first tried to use the image as the documentation but the :onbuild tag lacks of nginx rules and every request gets a 500.

What has your PR done to resolve that? Both end up serving nginx from the same main gatsby alpine image?

JaKXz commented 4 years ago

@kanolato I am in agreement with @polarathene's comments - are you able to update this PR and maybe it could get merged?

kanolato commented 4 years ago

Going to update it in the following days :) @JaKXz

kanolato commented 4 years ago

Than you all for your comments!. The required changes were just pushed.

kanolato commented 4 years ago

25

My bad, You are completely right on the last part, this doesn't solve #25, as in the end it only provides a way to serve the built assets. I will explore it later next week. Im almost sure its possible mounting volumes and so. Again thank you for your replies

polarathene commented 4 years ago

IIRC, stretch image dependencies were out of date, especially cwebp. Since Buster has been out for a while, perhaps better to switch to that.

kanolato commented 4 years ago

I have changed to buster and upgrade node to 12

gatsbot[bot] commented 4 years ago

Holy buckets, @kanolato — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!