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

New domains plugin and nginx extension #783

Merged
merged 29 commits into from Dec 16, 2014

Conversation

Projects
None yet
4 participants
@josegonzalez
Member

josegonzalez commented Nov 24, 2014

  • New domains plugin that follows the heroku domains plugin api
  • Fix NO_VHOST check
  • Move nginx configuration specification to templates
  • Allow overriding nginx template with a custom one in $APP/nginx.conf.template
  • Unify VHOST file creation

TODO:

  • Update the URL file with all hosts
  • Ensure all new files are backed up properly
  • Show http and https urls in deploy. Currently just shows whatever is in VHOST without a scheme. Should at least show the scheme.
  • Add a CANONICAL_HOST config var that all vhosts should redirect to if available Punt on this until it's requested more frequently.

josegonzalez added some commits Nov 24, 2014

Implement the ability to set multiple server names for a given applic…
…ation

Adding a hostname to the $APP/VHOST file will enable it as a virtualhost for the application.

In addition, any hostname set that matches an associated ssl certificate will also be set as an ssl host. Note that if a hostname does not have a matching SSL host, then it will result in an erroring application.

For the moment, running `dokku url` on an app may not correctly display the current hostnames for said app.
Add domains plugin as written in https://github.com/jtangelder/dokku-…
…nginx-alt

This plugin conforms to the heroku api for setting and removing domains. Note that it does not yet update the URL file for a given application.
Move VHOST file creation to a single domains:setup command
This helps remove duplication and also ensures that all hosts have a proper setup after running `make copyfiles`

Note that this command is not exposed in help as it is not generally useful to developers
Fix issue where VHOST was not properly being set
The URL file is being rewritten on every deploy by `dokku deploy` and thus that file was incorrect. Instead, we just reuse the old logic from the nginx-vhosts plugin to handle creation of the VHOST file
Delete the VHOST file, not the URL file, when NO_VHOST is set
The URL file is useful for understanding what port docker decided to host our app internally on.
Use updated VHOST file instead of URL file when outputting the curren…
…t URL to an app

Note that this may include more than one URL to an app
@josegonzalez

This comment has been minimized.

Show comment
Hide comment
@josegonzalez

josegonzalez Dec 12, 2014

Member

Added a comment to your PR. And thanks for the prs, can you make the cat change against master and a separate one for this PR?

Member

josegonzalez commented Dec 12, 2014

Added a comment to your PR. And thanks for the prs, can you make the cat change against master and a separate one for this PR?

@michaelshobbs

This comment has been minimized.

Show comment
Hide comment
@michaelshobbs

michaelshobbs Dec 13, 2014

Member

The cat change actually breaks dokku help. See my follow up line note. Basically I think we should just remove domains:help from the help output as no other plugin advertises their help command.

Member

michaelshobbs commented Dec 13, 2014

The cat change actually breaks dokku help. See my follow up line note. Basically I think we should just remove domains:help from the help output as no other plugin advertises their help command.

Merge pull request #818 from expa/nginx-cleanup
rebuild nginx config on domain change
@josegonzalez

This comment has been minimized.

Show comment
Hide comment
@josegonzalez

josegonzalez Dec 15, 2014

Member

@michaelshobbs any chance you can take a look at the last two? We can probably drop CANONICAL_HOST for now since no one has asked for that feature, but I can't merge this until the other bit works as it did before.

Also +1 on removing the dupe help.

Member

josegonzalez commented Dec 15, 2014

@michaelshobbs any chance you can take a look at the last two? We can probably drop CANONICAL_HOST for now since no one has asked for that feature, but I can't merge this until the other bit works as it did before.

Also +1 on removing the dupe help.

@michaelshobbs

This comment has been minimized.

Show comment
Hide comment
@michaelshobbs

michaelshobbs Dec 15, 2014

Member

Sure thing.
If we're running both http and https. should we show both?
What about when dokku url <app> is run?

Member

michaelshobbs commented Dec 15, 2014

Sure thing.
If we're running both http and https. should we show both?
What about when dokku url <app> is run?

@josegonzalez

This comment has been minimized.

Show comment
Hide comment
@josegonzalez

josegonzalez Dec 15, 2014

Member

Show https when its supported by a domain, otherwise http. There is code in the nginx:build-config command that detects https support on a per-domain basis, which we can probably repurpose.

For dokku url, I figure we should try to use https, fallback to a custom http, and finally fallback to the port version. Not sure how easy that is (if it's not, lets just output the first https and then first http one).

Member

josegonzalez commented Dec 15, 2014

Show https when its supported by a domain, otherwise http. There is code in the nginx:build-config command that detects https support on a per-domain basis, which we can probably repurpose.

For dokku url, I figure we should try to use https, fallback to a custom http, and finally fallback to the port version. Not sure how easy that is (if it's not, lets just output the first https and then first http one).

@michaelshobbs

This comment has been minimized.

Show comment
Hide comment
@michaelshobbs

michaelshobbs Dec 15, 2014

Member

Ok wrapping my head around more use cases...what do we want to do if there are multiple VHOSTs defined? Return the first one? Last one?

Member

michaelshobbs commented Dec 15, 2014

Ok wrapping my head around more use cases...what do we want to do if there are multiple VHOSTs defined? Return the first one? Last one?

@josegonzalez

This comment has been minimized.

Show comment
Hide comment
@josegonzalez

josegonzalez Dec 15, 2014

Member

Just return the first one (for dokku url) but all when the deploy occurs? Seems reasonable.

Member

josegonzalez commented Dec 15, 2014

Just return the first one (for dokku url) but all when the deploy occurs? Seems reasonable.

@michaelshobbs

This comment has been minimized.

Show comment
Hide comment
@michaelshobbs

michaelshobbs Dec 15, 2014

Member

Cool. I'm doing some testing locally and noticed that domains:clear seems to leave nginx in a non-restartable state because the VHOST file is empty.

What should be the behavior here? Should we go back to a NO_VHOST state or do a domains:setup and get the default VHOST state upon deploy? Seems like the latter, right?

Member

michaelshobbs commented Dec 15, 2014

Cool. I'm doing some testing locally and noticed that domains:clear seems to leave nginx in a non-restartable state because the VHOST file is empty.

What should be the behavior here? Should we go back to a NO_VHOST state or do a domains:setup and get the default VHOST state upon deploy? Seems like the latter, right?

@josegonzalez

This comment has been minimized.

Show comment
Hide comment
@josegonzalez

josegonzalez Dec 15, 2014

Member

I think default vhost is the correct route. NO_VHOST should be explicit.

Member

josegonzalez commented Dec 15, 2014

I think default vhost is the correct route. NO_VHOST should be explicit.

Michael Hobbs added some commits Dec 16, 2014

Michael Hobbs
bugfixes
 - don't match substrings in VHOSTS
 - handle case where NOSSL_SERVER_NAME is \' \'
 - rebuild nginx config on ssl import
 - prevent cwd errors after we rm the temp dir
@michaelshobbs

This comment has been minimized.

Show comment
Hide comment
@michaelshobbs

michaelshobbs Dec 16, 2014

Member

@josegonzalez have a look at #823 and let me know what you think

Member

michaelshobbs commented Dec 16, 2014

@josegonzalez have a look at #823 and let me know what you think

Michael Hobbs and others added some commits Dec 16, 2014

Merge pull request #823 from expa/nginx-cleanup
show URLS with scheme. now with bug fixes!!!
@josegonzalez

This comment has been minimized.

Show comment
Hide comment
@josegonzalez

josegonzalez Dec 16, 2014

Member

Alright this is looking solid. Can you do one more passover and see if we forgot anything? I'll merge and release once that's done.

Again, solid fucking work.

Member

josegonzalez commented Dec 16, 2014

Alright this is looking solid. Can you do one more passover and see if we forgot anything? I'll merge and release once that's done.

Again, solid fucking work.

@JMSwag

This comment has been minimized.

Show comment
Hide comment
@JMSwag

JMSwag commented Dec 16, 2014

👍

@michaelshobbs

This comment has been minimized.

Show comment
Hide comment
@michaelshobbs

michaelshobbs Dec 16, 2014

Member

Ship it! 😄
Look for a PR that fixes help msgs too.

Member

michaelshobbs commented Dec 16, 2014

Ship it! 😄
Look for a PR that fixes help msgs too.

@michaelshobbs

This comment has been minimized.

Show comment
Hide comment
@michaelshobbs

michaelshobbs Dec 16, 2014

Member

....against master of course 😉

Member

michaelshobbs commented Dec 16, 2014

....against master of course 😉

josegonzalez added a commit that referenced this pull request Dec 16, 2014

Merge pull request #783 from progrium/nginx-cleanup
New domains plugin and nginx extension

@josegonzalez josegonzalez merged commit 5bf1ba7 into master Dec 16, 2014

1 check passed

continuous-integration/wercker Build finished
Details

@josegonzalez josegonzalez deleted the nginx-cleanup branch Dec 16, 2014

@motymichaely

This comment has been minimized.

Show comment
Hide comment
@motymichaely

motymichaely Dec 21, 2014

@michaelshobbs - It seems like this line will eliminate the ability to add a domain that is subset of the app name.
For instance, for a VHOST that is set to be example.com and an app with the name of web-app there will automatically be set a domain of web-app.example.com. This means there will be no way to set a new domain like app.example.com.

motymichaely commented on plugins/domains/commands in 4e0670f Dec 21, 2014

@michaelshobbs - It seems like this line will eliminate the ability to add a domain that is subset of the app name.
For instance, for a VHOST that is set to be example.com and an app with the name of web-app there will automatically be set a domain of web-app.example.com. This means there will be no way to set a new domain like app.example.com.

@michaelshobbs

This comment has been minimized.

Show comment
Hide comment
@michaelshobbs

michaelshobbs Dec 21, 2014

Member

Good catch. We had a similar issue in the nginx-vhosts plugin. I'll patch and write a unit test for this.

Member

michaelshobbs commented on 4e0670f Dec 21, 2014

Good catch. We had a similar issue in the nginx-vhosts plugin. I'll patch and write a unit test for this.

This comment has been minimized.

Show comment
Hide comment
@motymichaely

motymichaely replied Dec 21, 2014

Great.

This comment has been minimized.

Show comment
Hide comment
@michaelshobbs
Member

michaelshobbs replied Dec 21, 2014

see #835

@michaelshobbs

This comment has been minimized.

Show comment
Hide comment
@michaelshobbs

michaelshobbs Dec 21, 2014

Member

Good catch. We had a similar issue in the nginx-vhosts plugin. I'll patch and write a unit test for this.

Member

michaelshobbs commented on 4e0670f Dec 21, 2014

Good catch. We had a similar issue in the nginx-vhosts plugin. I'll patch and write a unit test for this.

This comment has been minimized.

Show comment
Hide comment
@motymichaely

motymichaely replied Dec 21, 2014

Great.

This comment has been minimized.

Show comment
Hide comment
@michaelshobbs
Member

michaelshobbs replied Dec 21, 2014

see #835

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment