Add an nginx junebug plugin #52
Conversation
| server { | ||
| server_name %(server_name)s; | ||
| include %(includes)s; | ||
| } |
There was a problem hiding this comment.
Any other settings that should be in the template?
There was a problem hiding this comment.
There's usually a listen 80; here too, but I think if you leave it out it's the implicit default.
There was a problem hiding this comment.
Explicit better than implicit?
|
|
||
| locations_dir = ConfigText( | ||
| "The directory to write location block config files to", | ||
| default='/etc/nginx/includes/junebug/', static=True) |
There was a problem hiding this comment.
We currently recursively create dirs that don't exist here. The docs suggest the created dir's permissions are 0777, but it seems to create the dirs with 0755. Maybe that is an okay permission level for now, and we should think about a configurable level as a future thing?
There was a problem hiding this comment.
I wonder what happens if Junebug doesn't have permission to write to /etc/nginx?
There was a problem hiding this comment.
I think that's okay for now. (referring to the original comment by justinvdm)
There was a problem hiding this comment.
@hodgestar I think we'll get a:
distutils.errors.DistutilsFileError: could not create 'foo/bar': Permission denied
Maybe that is okay? Not sure there's much that can be done about it.
|
Ready for review. |
|
|
||
| vhost_template = ConfigText( | ||
| "Path to the template file to use for the vhost config", | ||
| default='%s/vhost.template' % (DIRNAME,), static=True) |
There was a problem hiding this comment.
Is this so that people can set up their own auth certificates, passthrough to the Junebug API, etc? I'm wondering if it would be easier/better to allow a config string where you can put a text blob that would be included in the template, where you could place that sort of extra config?
There was a problem hiding this comment.
Yep, this is for any vhost configuration people might want to do. I'm not sure if a text blob in a config field is a better way of approaching this though. A template of the file seems closer to the way this kind of thing is usually done with configuration management things like puppet. Any particular reason you think it would be easier?
There was a problem hiding this comment.
With the text blob, you don't need to know how the default config looks to make your own config. Also, you can't break things by forgetting to add the include for junebug things.
There was a problem hiding this comment.
|
Just one main comment about cleaning up when shutting down the plugin. The rest are mainly questions/answers. |
|
|
||
| def reload_nginx(): | ||
| if in_path('nginx'): | ||
| subprocess.call(['nginx', '-s', 'reload']) |
There was a problem hiding this comment.
Do we maybe want to use check_call or check_output instead of .call?
There was a problem hiding this comment.
We can, how are they better than call here?
There was a problem hiding this comment.
|
Comments either replied to or resolved, ready for another review I think. |
|
👍 |
For smaller projects using junebug, it would be helpful to have a plugin that maintains a set of nginx config files, where each file corresponds to an http-based channel and contains a virtual server configuration for that channel that routes requests to the relevant port that the channel is listening on.