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

Allow each amp instance to override the nginx vhost template #47

Merged
merged 4 commits into from
Nov 5, 2017

Conversation

totten
Copy link
Collaborator

@totten totten commented Jul 25, 2017

In nginx, each application/instance is likely to need to a different configuration file -- for example, a Drupal template won't work with WordPress or Symfony. To resolve #21, the amp create command must be able to use different templates for different builds.

This revision performs a structured search to determine the template, e.g.
* If the env has variable NGINX_VHOST_TPL, use that.
* If the web-root or any parent has .amp/nginx-vhost.php, use that.
* Otherwise, use the default template specified in amp.

The PR is not specific to nginx -- if the httpd type is set to apache or apache24, it will conduct a similar search (e.g. APACHE_VHOST_TPL and .amp/apache-vhost.php; or APACHE24_VHOST_TPL and .amp/apache24-vhost.php). However, the issue is most salient for nginx.

…#21)

For example, suppose we've configured the `httpd` as `nginx`:
 * If the env has variable `NGINX_VHOST_TPL`, use that.
 * If the web-root or any parent has `.amp/nginx-vhost.php`, use that.
 * Otherwise, use the default template specified in `amp`.
@totten
Copy link
Collaborator Author

totten commented Aug 8, 2017

Looking back after two weeks, I have mixed feelings, eg:

  • It's better to have something than nothing -- and this is enough to get nginx support un-stuck.
  • There are a couple dimensions that influence one's templates (e.g. the httpd type; the particular application/build-type; the local admin's preferences/quirks). I don't think this design does a great job of blending those dimensions.

@michaelmcandrew
Copy link
Contributor

Hey @totten - I am not sure what you mean about blending the dimensions. I thought this was a nice and flexible solution and would vote for a merge.

@michaelmcandrew
Copy link
Contributor

michaelmcandrew commented Oct 25, 2017

PS. I am using this branch locally for the moment and it is working for me.

@totten totten merged commit c5de740 into master Nov 5, 2017
@totten totten deleted the master-nginx branch November 5, 2017 01:30
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.

Nginx support
2 participants