Skip to content
This repository has been archived by the owner on Oct 19, 2023. It is now read-only.

Provide a default nginx-app.conf #139

Merged
merged 2 commits into from
Aug 10, 2016
Merged

Provide a default nginx-app.conf #139

merged 2 commits into from
Aug 10, 2016

Conversation

tmatsuo
Copy link
Contributor

@tmatsuo tmatsuo commented Aug 10, 2016

Note: with this image, we can just remove the nginx-app.conf in our most of the (probably all?) sample applications.

try_files $uri /index.php$is_args$args;
}
```

I hope this mechanism can cover most of the web frameworks, but let us
You might need to add some rewrite rules, or you might want to change
the behavior. In those cases, put a file named `nginx-app.conf` in the
Copy link
Contributor

Choose a reason for hiding this comment

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

Symfony uses app.php in the root of the project as a convention, so you should also add instructions on how to do this:

In this case, index.php is used as the framework front controller. You may need to change this to something different for your project. The Symfony framework, for instance, uses app.php instead of index.php.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added this to the suggested edit above.

@tmatsuo tmatsuo force-pushed the default-nginx-conf branch from 301a530 to 018b2ff Compare August 10, 2016 00:35
@bshaffer
Copy link
Contributor

It would be great to include a section with the proper app.yaml configuration and (if necessary) nginx-app.conf file for popular frameworks, i.e.

Framework Document root front controller
Symfony2 / Symfony 3 web app.php
Drupal8 project root index.php
Laravel public index.php
Wordpress project root custom nginx-app.conf

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Aug 10, 2016

@bshaffer Great suggestion, but I want put those info on our documentation, not on the README in this repo.

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Aug 10, 2016

We exhausted quotas for Container Builder API. Probably I've worked too much today!

@cedricziel
Copy link
Contributor

cedricziel commented Aug 10, 2016

+1 for adding it to the documentation. Great and necessary addition 👍

IMHO documentation is the single most important bit you can provide besides the image.

"The 3 step guide to run $product" [with some sidenotes] would be really nice.

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Aug 10, 2016

Ok, let's add the document, but can we do that in another PR?

I filed #140 for discussion

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Aug 10, 2016

@bshaffer PTAL!

@bshaffer
Copy link
Contributor

LGTM!

@tmatsuo tmatsuo merged commit 41e723c into master Aug 10, 2016
@tmatsuo tmatsuo deleted the default-nginx-conf branch August 10, 2016 17:43
@tmatsuo
Copy link
Contributor Author

tmatsuo commented Aug 10, 2016

Alright, the current image is available at:
gcr.io/google_appengine/php:2016-08-10_18_55
and
gcr.io/google_appengine/php:staging

Please feel free to test it.

You may still get a nasty permission error if you try to modify files in sub directories in ${APP_DIR} due to moby/moby#6047
(I hope it will be fixed when we move newer docker on container builder)

Possible wordarounds:

  • use "--storage-driver=devicemapper" for docker storage driver and build the image locally
  • use /tmp for temporary files

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Aug 11, 2016

@bshaffer tried that image and gave good feedback. We should loosen suhosin etc for php cli. I'll work on that next and I'll stage the image again, after doing do.

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Aug 11, 2016

See also #141

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants