Update existing nginx.conf instead

[motin]

Fixes #10

[jgallen23]

this looks awesome! nice job @motin

[wmluke]

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]

Oops, closed on accident. ;-)

[motin]

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 dokku/dokku#579) or alter the existing nginx.conf instead of overwriting it.

[motin]

Thanks jgallen23, glad you like it :)

[wmluke]

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...

• Please propose a way to handle SSL support in this plugin with your approach
• Fix the broken unit tests in this PR

You can run the test suite with make test.

Thanks,
Luke

[motin]

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 Plugin nginx-vhosts includes files in folder nginx.conf.d dokku/dokku#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]

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]

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

[motin]

@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]

Your PR does not include SSL support, so we can't remove this.

[wmluke]

Hi @motin,

As I said in #10 (comment), 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]

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]

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