The Road to 1.0: Cleanup part 1

[jfryman]

I have been thinking for some time it's long overdue to clean up this module. Chief complaints include:

• Whoa, what's happening in these specs?
• Specs do not cover XXX
• My package and how it gets installed is super different than XXX
• What is even happening in params.pp
• Why are all of the defined types in nginx::resources namespace?

Furthermore, there are a ton of bad things that have made it into the module that really shouldn't be there and may be causing unknown consequences:

• Resource defaults in defined types,
• WAY too many attributes currently able to be placed on the vhost and location resources.

So, I began the great slog to start working on this stuff. The journey of 1000 miles begins with the first step, right?

This PR covers a few starting items:

• Clean up params.pp. Introduces puppet_module_data, and obliterates params.pp completely. A good writeup of this can be found at http://www.devco.net/archives/2013/12/08/better-puppet-modules-using-hiera-data.php
• Deprecate the resources namespace. We will not get rid of these just yet, but once this lands, no new features will be added to the old resources.
• Factory pattern for class/service management. I will be writing a blog post soon to explain this pattern.

This PR does not pass tests yet, but it is far enough along where I can publish it for comments and help getting things cleaned up. Once this is merged, I will start looking for places where we can break out vhost and location into more manageable chunks. Bonus points for anyone who helps clean this up. ❤️

For any change on the way to v1.0, we will keep old interfaces intact for now. However, once we hit v1.0, you can and should expect things to change. I will do my best to keep it measured and make sure deprecation warnings are in place so you know what will be coming.

/cc @3flex

[3flex]

I like the direction this is going, couple of high-level comments so far:

• You said "we will keep old interfaces intact" but a lot of parameters were torn out of init.pp. To keep this backwards compatible these would have to remain, unless I misunderstood your meaning?

• I don't think everything from params.pp should go straight to the module data directory. Instead only stuff that relates to OS-specific settings, such as file paths, package names, service names, user/group names etc. Stuff that's just for nginx like keepalive_timeout for example can be a default value in nginx::config since they won't change based on the OS; there's no need to have this in the module data.

• The shift from nginx::resources namespace might be too aggressive. Not because this change breaks anything, but because there are issues with the existing resources:

  • Some of the module's default parameters differ from the nginx defaults (e.g. proxy_redirect default value #395, but there are others)
  • SSL files are being copied around; (e.g. ssl certificates #404, some discussion in Subdir for ssl certs #80)
  • It's currently not possible to have totally custom vhosts/locations since the module is a bit too "helpful" and makes some parameters mandatory when they don't really have to be (Catch all requests with wrong host and return 444 status #261, Always set the root parameter in nginx::vhost resources. #359, A redirect-only vhost #370)
  • Files like fastcgi_params are copied from the module when they are provided by the distro/nginx.org's packages already
  • SSL config is confusing (Allow to use multiple locations in vhost #189 (comment)) - it makes far more sense to me to either allow configs where there's an extra listen directive allowing for http://nginx.org/en/docs/http/configuring_https_servers.html#single_http_https_server, or to require one vhost for HTTP, and another for HTTPS, rather than sort of combining them as they are now.

  Just changing the namespace means these issues will also exist in the new resources; maybe you have a different plan in mind but to me this is an opportunity for a clean break from some of these issues. None have to be addressed in this PR of course but they should be on the roadmap IMHO.

I'll have a closer look over the next few days and try and get the tests working to see what's busted.

[3flex]

@jfryman I've got the tests working. Do you want me to submit a PR to merge to this branch, or just commit directly?

I don't have any other comments on the PR other than what I mentioned above (which after looking more closely at the proposed changes all still stand); there are a number other minor issues that I didn't address in my test-related fixes but a detailed review can come later.

[jfryman]

@3flex go ahead and commit directly to this branch. Let's do it in one transaction.

[jfryman]

@3flex great questions. Let's go through them.

Some of the module's default parameters differ from the nginx defaults (e.g. #395, but there are others)
SSL files are being copied around; (e.g. #404, some discussion in #80)

Agreed. My objective here is to move around the module internals as-is, and start cleaning up the actual data to match with upstream defaults.

It's currently not possible to have totally custom vhosts/locations since the module is a bit too "helpful" and makes some parameters mandatory when they don't really have to be (#261, #359, #370)

Agreed. Again, module internals only. I'd like a user to be able to provide a compiled template for any of the resources offered in the event that someone strays too far off the beaten path.

Our end target should be to accomplish 80% with this module, and then let folks get crazy if need be.

Files like fastcgi_params are copied from the module when they are provided by the distro/nginx.org's packages already

Let's audit these in another pull and see what we can reasonably get rid of.

SSL config is confusing (#189 (comment)) - it makes far more sense to me to either allow configs where there's an extra listen directive allowing for http://nginx.org/en/docs/http/configuring_https_servers.html#single_http_https_server, or to require one vhost for HTTP, and another for HTTPS, rather than sort of combining them as they are now.

Couldn't agree more. This should be a toggle. Too many options to mess with right now on the part of the user.

---

So, this pull does not represent making it to 1.0. Instead, it really is just some cleanup internally to make it easier to extract things. But, you bring up an interesting point.

maybe you have a different plan in mind but to me this is an opportunity for a clean break from some of these issues

Yes, and no. What needs to happen here is in two phases. The first is the deprecation warning for the old resources. Users will be notified that the old resources resources will be going 🔥 before long. In order for this to work, we'll need to have the new interface work exactly as expected so we can begin the move. Then, we'll start working on another release that starts addressing these issues (0.1.x branch), where we will make explicit that these are unstable releases, repair, and then release at 1.0 with the cleanup.

My only concern is that I might be too aggressive with the init.pp class. That might be worth rolling back to keep compatibility high.

I'll be hacking more this weekend, hopefully landing this thing. :)

[3flex]

Yeah, if it's purely internals then I'd keep init.pp as is, but still add package_class and service_class since that's something that can be introduced without breaking anything.

I'd also either leave nginx::service where it is, or add a deprecation to it and include nginx::service::init from that class. I have to say I'm not sure I understand the reason for having a class specifically for init.d systems, since Puppet will use whichever init system is provided by the OS. If it's to keep compatibility with the service_restart command then that should be reconsidered because when I looked into it the init scripts provided by nginx.org, Debian, Ubuntu, Fedora and Gentoo all do a config test before (re)starting the service so the module itself doesn't need to do that. I'd expect derivatives would do the same.

So you could leave nginx::service as is but still have the service_class parameter override it.

I might push an update to the tests in this branch so that what I'd expect to still work based on an "internals only" philosophy gets flagged if it's broken.

[jfryman]

Going to close this and take another pass tomorrow. This was a good Mikado, and good discussion.