Revert manifest setting default to nil

[dylanratcliffe]

Background

In version 3.19.0 I "fixed" the manifest setting. I had always assumed that it was working and evaluating site.pp in the same way that it usually would on a Puppetserver, however this was not the case. Turns out this had never worked and in "fixing" thus bug I actually broke lots of people's workflows.

Proposed Solution

My proposed solution is to go back to the manifest setting defaulting to nil and therefore site.pp not being evaluate unless you explicitly set the setting. Because onceover handles classification for the purposes of testing and hopefully the only thing that people are doing in their site.pp is classification, it makes sense for them to not be mixed.

I would propose to release this in version 3.20.0 (or maybe 3.19.1) and not to release a major version. The reason for this at that this change is not likely to break anyone's workflow. e.g.

• If a user has put in place a workaround for the changes in 3.19.0 by setting the manifest setting, this will continue to work.
• If they have simply not upgraded then 3.20.0 will also work
• If they have modified their site.pp to not cause failures it simply wont' be called anymore, meaning that it also won't fail

The only instance I can currently see of this being a breaking change is if a user has already come to rely on the changes made in 3.29.0, which I doubt. If this is the case it cals always be worked around by just setting the manifest setting anyway.

I am looking for feedback on this proposal, especially from @neomilium @smortex @genebean

[genebean]

I fully support reverting this as a z release as it was "fixed" in a y release. I think documenting clearly that the option must be set is perfectly sufficient considering it has always actually been that way.

[smortex]

Because onceover handles classification for the purposes of testing and hopefully the only thing that people are doing in their site.pp is classification, it makes sense for them to not be mixed.

we also set default values ;-) :

File {
  backup => false,
}

Apache::Vhost {
  ssl => true,
  # ...
}

I would love to see these taken into account, but any node definition should not. But it is beyond the scope of this PR.

So for today's matter, I also agree with reverting that change. Thank you for clarifying that you though it always worked like this and so did not considered this to be a regression.

Releasing as a patch release makes sense (no new feature, but something "broken" got fixed). I guess people who worked-around the first failure will be able to revert their fix when it fails again, or maybe choose to stick with 3.19.0 until 4.0.0 which add full support for this is made available?

[neomilium]

I agreed the revert to back to nil as default since its described like this in README file.

This PR fixes #292 .

If this PR is merged, the PR #293 should closed.

@dylanratcliffe Note: IMHO, you should use CHANGELOG.md instead of README.md to put the changelog :-)

[dylanratcliffe]

Merged! @neomilium I use the releases page as a changelog. The README.md bit is designed to highlight features or bugfixes that I want everyone to see, which might persist for >1 release. That way important stuff doesn't get buried by bugfix releases. Releases page should be used as the authoritative changelog. If you've got good suggestions for autogenerating both though would be good. Like update CHANGELOG updates releases or vice versa