search

mikexstudios / dokku-nginx-alt

Alternative nginx plugin for dokku. Supports multiple domains and custom nginx.conf.
35 stars 26 forks source link

look for templates in app bundle #12

Closed michaelshobbs closed 10 years ago

michaelshobbs commented 10 years ago

This will...

  1. Only install nginx if the version is below 1.4.0
  2. Removes default nginx-vhosts plugin during install, if it exists
  3. Allows override of nginx.*tpl by including it in the app repo/container
  4. Looks for VHOST at $DOKKU_ROOT/$APP/VHOST first and then defaults to $DOKKU_ROOT/VHOST

Tested and running on 10 different apps. Definitely open to feedback.

mikexstudios commented 10 years ago

Thanks! This looks great, and I like the ability to override by files in the app repo. I will take some time to review and get back to you soon.

michaelshobbs commented 10 years ago

Any more thoughts on this?

mikexstudios commented 10 years ago

Sorry, I've been busy with work, but I will respond to your comments soon. Thanks!

michaelshobbs commented 10 years ago

No worries man. Thanks.

jamescw commented 10 years ago

Any news on getting this merged, it would really solve an issue im having now

mikexstudios commented 10 years ago

@michaelshobbs Sorry for the delay. From your pull request, I decided to merge the renaming of existing nginx-vhosts plugin. I did not merge "Only install nginx if the version is below 1.4.0" and auto creation of the root VHOST file because I'm now having the install file just call the existing nginx-vhosts install (see https://github.com/mikexstudios/dokku-nginx-alt/blob/4a0b4ec46a3653fb99c43a370edbcff0b89fe904/install).

I am also not merging "Looks for VHOST at $DOKKU_ROOT/$APP/VHOST first and then defaults to $DOKKU_ROOT/VHOST", because I think it's rare that a root VHOST would be used to set the vhost for multiple apps unless the root VHOST is used as the base domain for subdomain generation. For that use case (which is the default dokku behavior with nginx-vhosts/), I would then suggest using https://github.com/mikexstudios/dokku/tree/subdomain2/plugins/auto-subdomains instead, which I'll put in its own repo sometime.

Finally, I decided to not merge "Allows override of nginx._tpl by including it in the app repo/container". Instead, I am making that a separate plugin: https://github.com/mikexstudios/dokku-app-configfiles. So the nginx._tplfiles are checked into the app under the.dokku/ directory. Then the 'app-configfiles' plugin will automatically copy those files over before this plugin's post-deploy hook is called.

Does all of this sound reasonable to you? Thanks.

mikexstudios commented 10 years ago

@jamescw What issue are you having?

jamescw commented 10 years ago

I wanted the ability to configure nginx from a template included in the app repo, it looks like the plugin you mentioned would do that, I'll take a look

— Sent from Mailbox

On Wed, Aug 27, 2014 at 11:17 PM, Michael Huynh notifications@github.com wrote:

@jamescw What issue are you having?

Reply to this email directly or view it on GitHub: https://github.com/mikexstudios/dokku-nginx-alt/pull/12#issuecomment-53634040

michaelshobbs commented 10 years ago

Makes sense.

michaelshobbs commented 10 years ago

I finally had time to go over your notes in further detail this weekend and I am still left with the question around the VHOST file in the app dir vs. the dokku root dir. I won't argue that it's 'rare' to use the $DOKKU_ROOT/VHOST to set the vhosts for the all apps. It just happens we provision our instances with the expectation of running only one app.

I will, however, point out that the default dokku nginx-vhosts plugin does expect a VHOST file in the $DOKKU_ROOT dir.

https://github.com/progrium/dokku/blob/master/plugins/nginx-vhosts/post-deploy#L7-L8

By not supporting that use case, we are preventing this very powerful plugin from being used as a simple drop-in replacement.