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

Plugin nginx-vhosts includes files in folder nginx.conf.d #579

Merged
merged 2 commits into from
Nov 16, 2014

Conversation

motin
Copy link
Contributor

@motin motin commented May 18, 2014

This allows dokku ops and other plugins to set custom per-app nginx configuration without having to overwrite the file post-deploy or in nginx-pre-reload.

@motin
Copy link
Contributor Author

motin commented May 19, 2014

Once this PR is merged, we can add https://github.com/neam/dokku-nginx-vhosts-custom-configuration to the list of Dokku plugins in the wiki.

@ghost
Copy link

ghost commented May 19, 2014

CI is currently broken - see #568 for details. I haven't had time to try and diagnose lately

@alecgorge
Copy link

This file will also need to be updated in most use cases I would imagine: https://github.com/wmluke/dokku-domains-plugin/blob/master/commands#L62

@motin
Copy link
Contributor Author

motin commented May 20, 2014

@alecgorge, no I rewrote the dokku-domains-plugin to not replace the existing configuration, see wmluke/dokku-domains-plugin#11 (Fixes wmluke/dokku-domains-plugin#10)

@javoire
Copy link

javoire commented May 21, 2014

If I'd want to have two server names like

server_name myapp.apps.mydomain.com myapp.com;

Would this be possible to do with the change suggested in this branch?

@motin
Copy link
Contributor Author

motin commented May 21, 2014

@javoire: This PR does not make it possible to add an extra server_name configuration directive in order to configure multiple domains. For that, you should instead check out https://github.com/neam/dokku-custom-domains

@javoire
Copy link

javoire commented May 21, 2014

Ok thanks :)

Jonatan Dahl http://www.linkedin.com/pub/jonatan-dahl/31/127/813
+46 (0) 70 450 39 16
Student, MSc Human-Computer Interaction
The Royal Institute of Technology (KTH)

On 22 May 2014 00:12, Fredrik Wollsén notifications@github.com wrote:

@javoire https://github.com/javoire: You can't add an extra server_name
configuration directive in order to configure multiple domains. For that,
you should instead check out wmluke/dokku-domains-plugin#11wmluke/dokku-domains-plugin#11


Reply to this email directly or view it on GitHubhttps://github.com//pull/579#issuecomment-43821142
.

@motin
Copy link
Contributor Author

motin commented May 23, 2014

Can this PR be accepted? :)

@AlJohri
Copy link
Contributor

AlJohri commented May 24, 2014

+1

@timsuchanek
Copy link

+1!

@sibsfinx
Copy link

+1

@mikexstudios
Copy link
Contributor

Another way to set per-app nginx configs (including multiple domains) is using: https://github.com/mikexstudios/dokku-nginx-alt

@motin
Copy link
Contributor Author

motin commented Aug 14, 2014

Another way to set per-app nginx configs (including multiple domains) is using: https://github.com/mikexstudios/dokku-nginx-alt

Yes, I tried that first but using it meant swapping out a huge portion of what is considered dokku core. This PR is a one-line change meant to make it easy for plugins to set per-app nginx configuration directives.

@mikexstudios
Copy link
Contributor

👍

@kirbysayshi
Copy link

Any chance on this getting merged soon? I need to increase the request size limit, and something like this seems the easiest path forward.

@motin
Copy link
Contributor Author

motin commented Sep 2, 2014

+1 for getting this merged. We use this patch together with https://github.com/neam/dokku-nginx-vhosts-custom-configuration for increasing upload size limits and timeout values.

@nikfalstie
Copy link

I have manually inserted this change into /dokku/plugins/nginx-vhosts/post-deploy on my server.
However, it seems that changes to the post-deploy script are ignored i.e. a new nginx.conf file gets generated, when I push or do a dokku deploy, but it does not contain my changes.

I'm a bit new to Docker and Dokku, so it might be something obvious.

Any suggestions?

Thanks in advance.

@motin
Copy link
Contributor Author

motin commented Oct 2, 2014

@nikfalstie: The default location for the nginx-vhost plugin is /var/lib/dokku/plugins/nginx-vhosts - try modifying it there.

@motin
Copy link
Contributor Author

motin commented Oct 2, 2014

Btw, we have used this patch in https://github.com/neam/dokku-host-provisioning for about 4.5 months now and it 1. works great and haven't caused any side effects for us and 2. is essential for us in order to for instance allow large file uploads (even if client_max_body_size is increased in the the buildpack nginx configuration, file uploads otherwise fails since the vhost nginx configuration limits this before it even gets to the buildpack's nginx configuration).

@nikfalstie
Copy link

I knew it was something stupid - just thought I had looked everywhere for other post-deploy files. Thanks a lot for your help :)

@motin
Copy link
Contributor Author

motin commented Oct 18, 2014

@progrium: Any chance merging this for 0.3.0 release?

@aral
Copy link

aral commented Oct 25, 2014

@motin Until this is merged, it would probably be a good idea to add a note to https://github.com/neam/dokku-nginx-vhosts-custom-configuration that the plugin does not work without this patch. I stumbled on the Git repo and didn’t realise it didn’t work on the current release version of Dokku and others might lose time over that also. Thanks for the awesome plugin + patch, by the way :)

@josegonzalez josegonzalez self-assigned this Oct 25, 2014
@josegonzalez
Copy link
Member

I need to make a release (0.2.4) but this will likely be out for 0.3.0.

@motin
Copy link
Contributor Author

motin commented Nov 16, 2014

@aral Glad you find it useful :) The fact that the plugin requires this PR is stated in the docs: https://github.com/progrium/dokku/blob/master/docs/plugins.md#dokku-features and hopefully this will be merged soon so that it doesn't need it anymore. I'll update the PR to include change in docs.

I need to make a release (0.2.4) but this will likely be out for 0.3.0.

@josegonzalez It doesn't seem to be included in 0.3.0, any idea of when it will be merged?

@motin motin force-pushed the include-nginx-conf.d branch from 3537c90 to 26a9e2f Compare November 16, 2014 11:18
@motin
Copy link
Contributor Author

motin commented Nov 16, 2014

Btw, this is a 100% backward-compatible change, so it could be released in 0.3.1

josegonzalez added a commit that referenced this pull request Nov 16, 2014
Plugin nginx-vhosts includes files in folder nginx.conf.d
@josegonzalez josegonzalez merged commit 3e224cc into dokku:master Nov 16, 2014
@josegonzalez
Copy link
Member

Merged, it'll be out in 0.3.1. Thanks for the PR.

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.