set manage_repo to "false" by default

[3flex]

Closes #863

From now on, this will have to be manually enabled by users according to the
needs of their environment.

cc @wyardley

As I was updating the spec tests, it occurred to me that if this parameter defaults to "false", that it's probably redundant - if a user were to set a value for package_source then we already know that they must set manage_repo => true (so it's redundant). So maybe a better approach is to add a warning so that anyone who currently has manage_repo set to "true" is notified that in a future release they must explicitly set package_source, and that manage_repo is going away.

I think this is better than just changing the default without any notification, and eventually leads us to the same place.

Anyway I'll leave this open and await any feedback on the above, since otherwise this can be merged as is.

[bastelfreak]

so this changes the default behavior, right? Can you add a section to the CHANGELOG.md that mentions it? So we don't forget to do a proper major verion bump for the next release.

[wyardley]

@bastelfreak: looks like changelog notes are automated? I don't know enough about the release process to know if there's something / somewhere we can add notes about breaking changes to, maybe whoever does the next release can put a note about breaking changes in there?

[3flex]

As is, yes, this changes default behaviour.

Any feedback on my alternate approach, described above? That should have no effect on existing installs, other than throwing a new warning. I'm ambivalent about which way you want to go, just something to consider.

For the changelog - add something now, or will it be auto-generated later?

[wyardley]

One other question... on CentOS / RHEL, there is no 'nginx' package unless EPEL is enabled, so if we're moving towards vendor packages, do we need some kind of docs change or soft dependency?

The acceptance tests already include EPEL, which I added fairly recently, more to do with something Passenger related:

      elsif fact('osfamily') == 'RedHat'
        # Soft dep on epel for Passenger
        install_package(host, 'epel-release')
      end

However, my guess is that with default params, these tests would fail if there was no EPEL.

I don't really love the approach of requiring an 'epel' module as a require or forced dependency, as users may have their own way of including EPEL locally if they use it that wouldn't create a class named 'epel', and whether or not to put it in metadata.json when it's only needed on RedHat type systems is also a question, but I also don't like the idea of enabling EPEL within this module either, since it could cause unpredictability for end-users.

[wyardley]

I agree about getting rid of $manage_repo in favor of just using $package_source. It would be nice if there was a way for the end user to pass in a version that gets passed on to the provider, so that the package version can be ensured (to latest, or a specific version), rather than just "present", at the user's request.

[bastelfreak]

@3flex, we fixed the failing ruby231 test on master, they will be gone if you rebase.

[3flex]

I'm not sure what the best thing is for CentOS & Red Hat. I don't have experience with them, so hadn't considered the lack of vendor packages... though I see that it's apparently in the Red Hat Software Collections as of Red Hat 7? Could that be used? Is there an equivalent package source for CentOS? Even if there is, it doesn't help with Red Hat 5 or 6...

I'll make the change to push people towards using package_source to specify alternate sources, and can add a way to select the version.

Will need someone to provide a solution for Red Hat systems though.

[wyardley]

EPEL is a semi-official repo; using it is very common among RedHat / CentOS users, and it does provide an Nginx package that's decent, and has a fairly recent version.

The issue, though, is that due to the way Puppet handles packages, we can't (easily) "just" install Nginx and its modules from EPEL, so the repo would need to be enabled for this to work. I still think some kind of requirement for EPEL with the default package source is reasonable, it's just whether it should be a "hard" dependency somehow, or a soft one. There are modules like https://forge.puppet.com/stahnma/epel, but some people prefer to handle configuring it differently.

Just listing it in the docs is how puppetlabs-rabbitmq handles it, FWIW (which is what I'm calling a 'soft' dep; you'll notice it's not listed in metadata.json):
https://github.com/puppetlabs/puppetlabs-rabbitmq#module-dependencies

It got a version bump recently, so for EL6-7, it's a reasonable version (1.10.x); I just removed official 5 support in #925.
https://dl.fedoraproject.org/pub/epel/7/x86_64/n/
https://dl.fedoraproject.org/pub/epel/6/x86_64/n/
FWIW, the EPEL version does use dynamic modules.

[wyardley]

@bastelfreak and I chatted about this, and think it makes sense to not try to change the code for a soft dep, but just mention the EPEL dependency in the docs, and also mention the workaround of using a different repo source instead of default.

@3flex: are you going to rework this to get rid of $manage_repo entirely?

SCL is an option, but I don't prefer it personally (as the default).

[wyardley]

@3flex: is any more discussion needed?
Can you remove the $manage_repos option if we think that's what's best, and add a docs bit about the EPEL requirement?

Something like this, maybe, if we want it in README rather than quickstart?

"On RHEL and CentOS, when $package_source is not set, Puppet expects to have EPEL available to get the nginx package. This can be managed by the stahnma/epel module, if you like, or you can handle the dependency at the site level. Alternately, you can choose one of the other available package sources as described in quickstart.md`

quickstart.md already has a mention of EPEL, though @bastelfreak says that the term I used "soft dependency" is the wrong one here. In any event, the section in quickstart.md needs to be updated to reflect these changes for sure.

I think after I rework streamhost support, we're going to try and cut a Puppetforge release pretty soon (even though there are clearly lots more things to be done in terms of the various changes we've been talking about).

[3flex]

Life's getting in the way of making these updates - if someone wants to open a new PR (or make changes to this branch directly) feel free. I know @wyardley has been making a tonne of great changes in short order so feel free to run with this.

I'll come back to it eventually but I don't want it to be a blocker for any new Forge release.

[wyardley]

No worries, we can just hold off on this change and skip it in this release. If I have time during the contributor summit, might take a pass at reworking it, but going to leave it here for now.

[wyardley]

Replacing with #938, leaving @3flex's commit attributed to him.
Still working on some of the structural changes in that one.