search

gatsbyjs / gatsby-docker

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

Issue #42: nginx config generated by nginx-boot.sh does bogus 301 red… #43

Closed torenware closed 4 years ago

torenware commented 4 years ago

Fixes Issue #42.

Modified nginx settings to enable correct redirects when using the -p argument in docker run.

polarathene commented 4 years ago

This has only been tested locally right? Not as mapping to public port 80 on a server or inbetween a reverse proxy?

While it does fix the local issue you brought up(your PR should also update the description to add Fixes #42 btw, it will auto close it on merge and reference the PR in the related issue), I'm not sure of any potential gotchas switching from absolute to relative redirects.

I will try to fit in some time to test those two other situations and look into the impact of the change. Otherwise if you'd like to switch it to using an env var like other options do, we can leave it with the default on for now and I can merge. I just don't want to merge without testing if there is risk of introducing breakage to existing users.

torenware commented 4 years ago

I'll update the PR and #43 to make the cross references.

I thought a bit about how one tests an image. I'm used to projects where you can do automated testing, and this one's a bit different, especially since it's specific to cases like using the image in local development.

Probably the best approach is to make the change a setting and put an environment variable in front of it, where port_redirect and absolute_redirect are set as was before and continues to be default behavior, and relative redirection is new behavior that the EV can set.

If this works for you, I'll go ahead and and do that, and modify README.md to document the new behavior.

torenware commented 4 years ago

It would be useful to know the use cases for these images. I tested the most specific thing I could: does nginx on the image create an absolute ref in Location response header or not. An excellent question would be which use cases care about this, and which type of response they need. Then you can do a simple table of who the setting is good for.

gatsbot[bot] commented 4 years ago

Holy buckets, @torenware — 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!

polarathene commented 4 years ago

It would be useful to know the use cases for these images

I just have the ability to maintain this repo, presumably granted from earlier activity in 2018/2019.

I don't think this is really an official or endorsed image by Gatsby team, but a community maintained one that they've added under the organization umbrella for the community to collaborate on. Gatsby focuses on what they know best.

AFAIK, there isn't anything special about this image, a standard nginx server image should be fine, you can combine with a multi-stage build that has another image build your site and pull the build output from that into your final image.