wmluke / dokku-domains-plugin

Dokku plugin to create nginx vhost with multiple domains
MIT License
176 stars 21 forks source link

Update existing nginx.conf instead #11

Closed motin closed 10 years ago

motin commented 10 years ago

Fixes https://github.com/wmluke/dokku-domains-plugin/issues/10

jgallen23 commented 10 years ago

this looks awesome! nice job @motin

wmluke commented 10 years ago

Hi @motin,

Thanks for the PR.

How do you propose to handle different SSL cert/domain combos within the single nginx.conf file? See #5 for details.

wmluke commented 10 years ago

Oops, closed on accident. ;-)

motin commented 10 years ago

How do you propose to handle different SSL cert/domain combos within the single nginx.conf file? See #5 for details.

I propose that such feature is either merged into the nginx-vhosts plugin or a separate plugin. Either way, the plugin should use an include file (as per https://github.com/progrium/dokku/pull/579) or alter the existing nginx.conf instead of overwriting it.

motin commented 10 years ago

Thanks jgallen23, glad you like it :)

wmluke commented 10 years ago

Hi @motin,

I appreciate your feedback and PR. Unfortunately, this PR deletes all the SSL support that we have been working on and that I plan to release soon. Also, your PR breaks our CI build.

To move forward...

You can run the test suite with make test.

Thanks, Luke

motin commented 10 years ago

Please propose a way to handle SSL support in this plugin with your approach

A. Remember, we shouldn't aim to create multi-purpose plugins. Remember the Unix philosophy: Write programs that do one thing and do it well. Write programs to work together. With this in mind, first step is to do either 1 or 2.

  1. Create a plugin called dokku-secure-app or similar and port the SSL enhancements there. (Check https://github.com/progrium/dokku/pull/579 which would make it dead easy to add directives like ssl_ciphers etc through a plugin.)
  2. Create a PR with your proposed enhancements with regards to SSL support against https://github.com/progrium/dokku/blob/master/plugins/nginx-vhosts/

B. In this plugin, make sure that adding custom domains still works when the user is using the other SSL plugin or the enhanced standard nginx-vhosts plugin.

Fix the broken unit tests in this PR

Sure.

motin commented 10 years ago

What are the SSL enhancements btw? Comparing https://github.com/progrium/dokku/blob/master/plugins/nginx-vhosts/post-deploy#L30 and https://github.com/wmluke/dokku-domains-plugin/blob/develop/commands#L63 indicates that the implementations are similar but vaguely different. Fragmentation is dangerous and may not be healthy for the dokku community. For instance, a dokku user that has followed the official instructions https://github.com/progrium/dokku#tlsspdy-support to set up SSL support will have his/her set-up broken after installing this plugin.

jgallen23 commented 10 years ago

@motin it might make sense to release yours as a different plugin since it is quite a bit different from the existing

motin commented 10 years ago

@jgallen23, I'd like to prevent further fragmentation if possible. There are already two dokku-domains plugins and even some PRs about including domains-support in the nginx-vhosts plugin... The confusion for new dokku users is already too high.

wmluke commented 10 years ago

Hi @motin,

As I said in https://github.com/wmluke/dokku-domains-plugin/issues/10#issuecomment-43846650, I appreciate your feedback and passion, but I feel that this PR is not a good fit for dokku-domains-plugin. Best of luck with dokku-nginx-vhosts-custom-configuration.

Cheers, Luke

motin commented 10 years ago

Furthermore, adding differing TLD's to nginx.conf as you propose could interfere with dokku's SSL support b/c dokku assumes a a single SSL cert for a sole TLD and subdomains. For example, it would be an issue if your cert is for *.foo.com but you've added foo.com and bar.com to nginx.conf.

Agree, the current implementation does not properly handle the SSL case, but please keep this PR open - otherwise I can't push new commits to this branch with the proper SSL support.

motin commented 10 years ago

For anyone interested, this PR will never be included in the dokku-domains-plugin repo (See #10), so I released a fork containing this PR earlier this morning: https://github.com/neam/dokku-custom-domains