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 #26

Merged
merged 6 commits into from
Mar 3, 2020
Merged

Conversation

kanolato
Copy link
Contributor

@kanolato kanolato commented Mar 21, 2019

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

@kanolato kanolato changed the title Clarify dockerfile documentation Improve Docker image Mar 21, 2019
@kanolato kanolato mentioned this pull request Mar 25, 2019
@polarathene
Copy link

Ok. So these changes create this flow:

  • Switch from the alpine:edge base to a Debian Node base image.
  • Install a fixed version of gatsby cli (why is the version fixed?)
  • Copy over the project into the image
  • Install via yarn followed by a gatsby build
  • Require the user to add two files as described in the documention
  • Have the user build that local dockerfile, performing the referenced ONBUILD instructions
  • Followed by a multi-stage build to copy the build output in public to the alpine:edge base image

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?

Dockerfile.onbuild Outdated Show resolved Hide resolved
Dockerfile.onbuild Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@JaKXz
Copy link

JaKXz commented Jul 31, 2019

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

@kanolato
Copy link
Contributor Author

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

Dockerfile.onbuild Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
ONBUILD WORKDIR /app
ONBUILD ADD . ./
ONBUILD RUN yarn
ONBUILD RUN gatsby build

Choose a reason for hiding this comment

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

for most gatsby users, i suspect this new file will be missing required system dependencies for native compile steps from core-js|cwebp|sharp|etc used in the gatsby build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I'm not getting it, wich new file are you referring to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean this deps should also be added to the image? core-js|cwebp|sharp

Copy link

Choose a reason for hiding this comment

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

correct. those things have native compile toolchains, of which the support libraries required for such installations i suspect are not present in this image. gatsby will eventually try and use node-native bindings to hook into them on build, but would not be found. thus, i suggest gatsby build would fail

@kanolato
Copy link
Contributor Author

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

@kanolato
Copy link
Contributor Author

#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
Copy link

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
Copy link
Contributor Author

kanolato commented Mar 2, 2020

I have changed to buster and upgrade node to 12

Copy link

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for taking the time to contribute the improvements! 😄

@polarathene polarathene merged commit e1e58db into gatsbyjs:master Mar 3, 2020
@gatsbot
Copy link

gatsbot bot commented Mar 3, 2020

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!

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.

5 participants