Switch to using concat{} instead of lots of file{} magic.

[3flex]

Taken from #136 and rebased. I removed the formatting changes to make things a bit easier to read.

Note this question from the previous PR was left outstanding, if anyone has an opinion please let me know.

Should I automatically delete autogen.conf as part of these changes so that you don't end up with duplicate entries or would you prefer that users do that themselves?

I lean towards a new file{} for autogen that deletes the existing file

Resolves #135
Resolves #136
Resolves #159

Still to do:

• Fix current tests on 1.8.7
• Delete existing conf.mail.d/vhost_autogen.conf
• Delete existing conf.d/vhost_autogen.conf
• Give the mail config a different filename to avoid clashes with vhost configs (using the same host name for both would affect the same file and compilation would fail) Instead, have excluded mailhosts from sites-available/enabled pattern since mail config is separate from http config
• Add tests for all new functionality (particularly sites-available/enabled)
• Update a few remaining tests for the concat pattern (marked TODO in the spec files)
• Actually test this on a system (!)
• Update rspec-system tests
• Tidy the commits for merging

[CtrlC-Root]

So is this going to get merged?

[deric]

👍 looks good. any progress on this?

[jfryman]

Going to take a pass at this now that we have some spec coverage. There are a bunch of things I want to make sure pass before going forward.

@apenney can you rebase one more time? I'm going to attack this during the Thanksgiving Holiday

[3flex]

Oh this is my branch, I took @apenney's work from #136 and rebased with a couple of changes. I'll rebase later today or tomorrow and update the specs.

To test this properly you'd need coverage from rspec-system or similar. I might look into that for this since I want to use it for some of my own work, so it'll teach me a lot (like writing the rspec tests did). I think a lot of the rspec tests could be reused, and it could check the full file rather than the fragments.

Two questions:

• Do you want these in a sites-available directory instead of conf.d and include it from nginx.conf? I think it makes a lot of sense. I'm happy to add that since I want that too, since it will order the configs correctly (conf.d before sites-available).
• Should I automatically delete conf.d/vhost_autogen.conf and conf.mail.d/vhost_autogen.conf as part of these changes so that you don't end up with duplicate entries or would you prefer that users do that themselves?

[CtrlC-Root]

@3flex I don't think your first question should even be a question. If you do not separate them that way then we won't be able to define proper configurations. I ran into this problem myself as currently the files are not separated. (ex: try with the current module to properly set up a reverse proxy).

[3flex]

@CtrlC-Root I agree, it was just when I rebased the old PR I didn't want to make any functional changes, but just get it in a state to be merged.

I'll add that then, and try to model off the way puppetlabs-apache does it for some level of consistency unless there are any objections.

[jfryman]

1. I agree. Let's go with the sites-available pattern. That matches closer to what I would expect to see in the Real World.

2. I think you delete the entries. If what is included here solves the problem and matches 1:1, leaving cruft around is only going to make things even more complicated. Just need to make sure tests look good.

[3flex]

I'd appreciate a review at this point. I've added the sites-available/enabled pattern similar to the puppetlabs-apache module, and removed the files/directories the module created under /tmp.

[3flex]

Ping @jfryman. I think I've fixed up all the issues, and I've run the rspec-system tests successfully on Ubuntu 12.04, CentOS 6.4 and Fedora 18. I couldn't add many scenarios since rspec-system is slow and it's hard to iterate but nginx starts and responds to requests on each system.

cc @CtrlC-Root @deric in case you have feedback

[jfryman]

I'm a little confused by this here. I know that this pull incorporates the sites-available and sites-enabled pattern. I like how that's laid out, but I want to say that if I define it in code, that it should be in sites-enabled.

Thoughts?

[3flex]

Are you suggesting this module just put the vhost file in sites-enabled directly, with no symlink to sites-available? puppetlabs-apache puts the generated config file in sites-available then symlinks from sites-enabled if the vhost is enabled, and removes both the file and symlink if absent. See https://github.com/puppetlabs/puppetlabs-apache/blob/master/manifests/vhost.pp#L426.

Also if there's no symlink there'll be no way to remove a site from sites-enabled since puppetlabs-concat doesn't have $ensure support yet so it'll be impossible to remove the file concat generated.

[jfryman]

Muy bueno. A few comments, but otherwise 👍

[3flex]

I'll remove the file_ensure stuff. I was hoping puppetlabs-concat would have a new release we could depend on with that support but no such luck yet! Might ping them to request one.

Let me know about the symlink. My preference is to leave it as is.

[jfryman]

I'd really like to get rid of the symlink. I don't see how it provides value other than sticking with the old debian way of filtering out what is available and what is enabled.

This certainly isn't closed, but I'd like to understand why this helps. Otherwise, I think it should come out.

[CtrlC-Root]

@jfryman It follows the principle of least surprise. For whatever reason, common web servers are configured this way. Maybe it's a holdover from the past or maybe there's a really clever reason for it, but whatever the reason may be it's the defacto standard. This helps because:

1. If I decide to stop managing nginx with puppet this would leave it in a familiar configuration. 2) If a sysadmin wants to see how nginx is configured without looking at puppet (maybe he doesn't have access? maybe there's a bug?) s/he won't be surprised by the layout. 3) Lastly, this opens up the possibility down the road of allowing non-managed vhosts to coexist with puppet managed ones, once again in a manner most sysadmins are used to.

[3flex]

I was going to say "consistency" but like @CtrlC-Root's explanation. I could add comments to the header in the vhost templates saying both the file and the symlink in sites-enabled are managed by Puppet.

Also without the symlink there's no way to remove the vhost from the nginx config if the file goes directly in sites-enabled:

nginx::resource::vhost { 'test':
  ensure => 'present',
}
#vhost added to sites-enabled

but say I want to remove my test vhost:

nginx::resource::vhost { 'test':
  ensure => 'absent',
}

The vhost config will remain in sites-enabled since right now we can't say ensure => absent on the outputted concat file.

[deric]

I would also vote for keeping the symlinks, at least for Debian. If other distributions choose different pattern, then Puppet module should respected the convention for each of them. It could be quite frustrating when nginx configuration doesn't look like the one from the distribution.

[3flex]

I'd need to know the layouts for RH/Fedora and Suse if they're going to differ. But it's easy to add support.

[CtrlC-Root]

@3flex They don't. Here's the module I'm currently using in production on my RHEL6.4 machines: https://github.com/CtrlC-Root/puppet-nginx. You can look at it for reference if it helps.

[3flex]

I know the current setup will work as is on other distros, I was only wondering if they had different conventions for laying out their nginx vhosts and if so what they are so they can be supported as well as Debian based distros.

[CtrlC-Root]

@3flex sorry for the confusion, I misunderstood your question. I do believe the conventions are the same on RHEL. I remember reading the NGINX documentation when I wrote my module, and I think it's the same layout on all platforms. Of course, this could be changed by supplying a different default config file with packaged versions but this is not the case on RHEL.

[jfryman]

All good points. Thanks @CtrlC-Root @3flex.

Okay, here we go!

[jfryman]

I'm going to let this bake with folks not using the Puppet Forge module for a few days, and cut a release at the end of week. Any folks that wanna give this a go and report back with issues (if any) would be greatly appreciated.