Remove deprecated functionality and prep for 6.0.0 release #332

DavidS · 2016-06-29T14:36:57Z

This PR contains a bunch of work to bring Puppet4 goodness to NTP. This is still depending on infrastructure work elsewhere(MODULES-3529, MODULES-3538), but the core improvements are in place.

Yes, some of the changes could be done in a single step, but I kept it split out for educational purposes, to make the process visible, and each step simple.

ripienaar · 2016-06-29T14:44:40Z

this makes me happy, well done :)

ripienaar · 2016-06-29T15:28:23Z

manifests/init.pp

-  $authprov               = $ntp::params::authprov,
+  Boolean $broadcastclient,
+  Tea::Absolutepath $config,
+  Optional[Tea::Absolutepath] $config_dir,


where is Tea::Absolutepath from?

https://github.com/voxpupuli/puppet-tea ; one of the things I'm not yet 100% sold on.

interesting, but yes I'd have expected those in core or stdlib - early days though

robinbowes · 2016-06-29T15:50:31Z

Is this a breaking change? ie. only works on puppet >= 4.x ?

DavidS · 2016-06-29T16:10:43Z

@robinbowes the currently released version of the ntp module works on both puppet 3 and 4. These changes take advantage of puppet 4 features, and drop support for running on puppet 3. The last commit also changes the interface, but work before that will provide support for changing up to the last stage. This is not expected to be all released in one go.

ryanycoleman · 2016-06-29T17:38:57Z

metadata.json

@@ -9,8 +9,9 @@
  "issues_url": "https://tickets.puppetlabs.com/browse/MODULES",
  "dependencies": [
    {"name":"puppetlabs/stdlib","version_requirement":">= 4.6.0 < 5.0.0"}


@DavidS what's still being used from puppetlabs/stdlib after this refactor?

Ah, good question.

puppetlabs-ntp/manifests/params.pp

Line 2 in 6331180

if str2bool($facts['is_virtual']) {

seems to be the last stdlib usage, and that looks like something that needs reevaluation, when puppet3 support is dropped.

Missing a comma on this line too, the file is invalid:

Debug: ntp has an invalid and unparsable metadata.json file. The parse error: 399: unexpected token at '{"name":"puppet/tea","version_requirement":"= 0.2.1"}

Yeah, there are many a reason why this is REVIEW ONLY ;-)

DavidS · 2016-07-19T11:04:08Z

Updated with preliminary template work, more cleanups, rebased to current upstream master

CorbyAsCode · 2016-07-29T13:19:07Z

I am in the process of migrating from 3.8 to 2015.2. I am checking my current catalog against the Puppet 4 parser using the catalog_preview module. The catalog_preview module outputs a warning:

Preview Warnings (by issue)
MIGRATE4_REVIEW_IN_EXPRESSION (4)
/etc/puppetlabs/puppet/environments/future_parser/modules/ntp/manifests/service.pp:4:25

This warning is described here.

It looks like validate_re() could be used instead of the "if" conditional with the "in" comparison.

DavidS · 2016-07-29T13:31:37Z

@corbyshaner ah, good catch! Thanks for pointing that out.

alvagante · 2016-08-03T14:18:24Z

data/common.yaml

@@ -0,0 +1,54 @@
+---
+ntp::authprov: ~


Has the tilde here some special meaning? (undef?)

yes. ~ translates to ruby nil, which is undef in the puppet4 ruby api.

On 3 August 2016 at 15:57, Alessandro Franceschi [email protected]
wrote:

In data/common.yaml
#332 (comment)
:

@@ -0,0 +1,54 @@
+---
+ntp::authprov: ~

Has the tilde here some special meaning? (undef?)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/puppetlabs/puppetlabs-ntp/pull/332/files/64db8cd4fd3d437b79e8d8802d64fdc0420a2a4d#r73346135,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABsQE681uJmD67iJhNyc8bfVt-gOcqwks5qcKx3gaJpZM4JBM8x
.

DavidS · 2016-09-13T10:06:42Z

This is shaping up to become the final version (except for fixing https://tickets.puppetlabs.com/browse/MODULES-3852). If you want to have another look, now would be the time. See #335 (or just the second to last commit) for what will released as the intermediate 5.0.0 release.

ripienaar · 2016-09-13T10:17:26Z

manifests/init.pp

+  Optional[String] $authprov,
+) {
+  # defaults for tinker and panic are different, when running on virtual machines
+  if str2bool($facts['is_virtual']) {


this one should already be a boolean no need for str2bool

ahpook

I ade a pass through the recent changes - Looks great! I made a few comments, which are mostly for visibility to other people rather than problems that would block merge/release. Fantastisch @DavidS

ahpook · 2016-09-30T20:06:10Z

CHANGELOG.md

+## Supported Releases 5.0.0 and 6.0.0
+### Summary
+
+This double release adds new Puppet 4 features: data in modules, EPP templates, the $facts hash, and data types. The 5.0.0 release is fully backwards compatible to existing Puppet 4 configurations and provides you with [deprecation warnings](https://github.com/puppetlabs/puppetlabs-stdlib#deprecation) for every argument that will not work as expected with the final 6.0.0 release. See the [stdlib docs](https://github.com/puppetlabs/puppetlabs-stdlib#validate_legacy) for an in-depth discussion of this.


shouldn't this be: "the 5.0 release is fully backwards compatible to existing Puppet 3 configurations" ...?

v5 drops puppet3 support, as none of the newly used puppet 4 features are available there. Folks need to get off puppet3 before they can use any of this.

Do I need to be more explicit about that?

ahpook · 2016-09-30T20:09:01Z

manifests/init.pp

@@ -117,5 +70,4 @@
  class { '::ntp::config': } ~>
  class { '::ntp::service': } ->
  anchor { 'ntp::end': }


are these anchors still required, given the existence of contain? (If so I would consider that a bug/need for enhancement in contain) contain function doc

Not sure. I'll have to investigate.

Without having it run, the solution might look like this:

contain('ntp::config') contain('ntp::service') Class['ntp::config'] ~> Class['ntp::service']

I'm not sure I like that much better. I've created MODULES-3917 to follow up

It's crappy for sure, since 4.8 you'll be able to do:

contain("foo") -> contain("bar") ~> contain("baz")

which will make things a lot better.

Shiny, but very tight version constraint. :-/ @ahpook what's your opinion on switching the puppet version requirement from >=4.5 to >=4.8?

IMHO the refresh of services when configs changes should be a behaviour, enabled by default, that users must be able to change, via a class parameter. There can be situations (probably it's not the case for ntpd, but can be valid for other services) where an immediate restart of the service is not desired.

In a pinch, you can always set ntp::service_manage to false to avoid this, and replicate the service resource. I know that is not a optimal solution, but the main focus of this patch set is not changing anything, as perverse as that may sound.

ahpook · 2016-09-30T20:13:03Z

metadata.json

  "author": "Puppet Labs",
  "summary": "Installs, configures, and manages the NTP service.",
  "license": "Apache-2.0",
  "source": "https://github.com/puppetlabs/puppetlabs-ntp",
  "project_page": "https://github.com/puppetlabs/puppetlabs-ntp",
  "issues_url": "https://tickets.puppetlabs.com/browse/MODULES",
  "dependencies": [
-    {"name":"puppetlabs/stdlib","version_requirement":">= 4.6.0 < 5.0.0"}
+    { "name":"puppetlabs/stdlib","version_requirement":">= 4.13.0 < 5.0.0" },
+    { "name":"puppet/tea","version_requirement":">= 0" }


hmm, as puppet-tea is a tiny, experimental module, and it's below 1.0 so no API guarantees, perhaps it'd be better to define the type aliases here inside ntp, and eliminate this dependency? alternately maybe we could pull those aliases over into stdlib or some place that they will get visibility and use but not restrict puppet-tea from changing API? // cc @igalic

Talked to @igalic offline and we decided to start pulling those types into stdlib. We'll start out with the easy ones: paths, and URLs, and we'll see from there.

MODULES-3919

ahpook · 2016-09-30T20:16:29Z

templates/ntp.conf.epp

+tinker<% if $ntp::_panic { %> panic <%= $ntp::_panic %><% } %><%if $ntp::stepout { %> stepout <%=$ntp::stepout %><% } %>
+<% } -%>
+
+<% if $ntp::disable_monitor {-%>


I haven't seen this construct in use before and {-%> is an aummm... "unusual" sigil sequence. I understand how it got there but, wow. // cc @loriland

Would { -%> (with a space) be better? I'm totally up for fixing style in the last minute. I need to do a pass at outstanding PRs and rebase this AGAIN anyways 😩 .

alvagante · 2016-10-04T07:27:47Z

data/common.yaml

+ntp::tos_minsane: 1
+ntp::tos: false
+ntp::udlc_stratum: 10
+ntp::udlc: false


Is this really the way to go? One class parameter for every possible configuration directive of the managed application?
What happens when a new release of an application adds new directives? You need to update the module and always catch up the upstream software.
What's the performance impact when there are a LOT of such parameters?
Thanks God at least here we have a parameter that allows users to provide custom templates (I find modules that don't offer a similar parameter inherently broken).

If it were for me templates+options hash pattern for the win!
But I've heretic opinions, probably :-D

An option hash implies no validation, no type hints, no discoverability of available options, no validation of whats provided.

Makes sense sometimes sure, but when its a fairly fixed thing with a large % of known options, stating them wins :)

To the options hash, I have talked to puppet users who see the params list as one of the huge advantages of puppet over hand-crafting the config file, because it is a unified syntax, order-independent, and tested. My current PoV is that having the template/epp params for folks who "know better" is essential for maximum flexibility, but the main value of a module lies in reducing the amount of effort people have to put in.

See also our profiles-in-modules conversation from earlier this year. I haven't forgotten this, and I will come back to it!

I see the options hash pattern as inherently broken, for the reasons stated by @ripienaar.

I concur that not supporting a custom template is also a broken pattern.

I think the best combo is parameters for the most common options given for the application, with the ability to also define the content completely as a parameter. This gives the flexibility to allow low-touch usage of a module, but also allow drop-in replacement for the content from a given template, possibly in a module.

The arguments agains an options_hash for lack of validation I think are obsoleted by epp templates. Validation can be done there, fully exploiting Puppet type system.

I agree with @petems , my preference is towards a sane number of params to manage important or commonly used application specific settings + a parameter to set a custom template (and I suppose most people agree here) AND a parameter to populate a custom hash, in whatever desired way, so that settings used in (custom) templates can be defined as data (on hiera or wherever).

This finishes the move to a cleaner puppet4 interface. Since this is a breaking change, it also bumps the major version. Users of the previous version who were running without receiving any deprecation warnings can use this version without any changes.

ripienaar reviewed Jun 29, 2016
View reviewed changes

ryanycoleman reviewed Jun 29, 2016
View reviewed changes

DavidS force-pushed the experiments/puppet4 branch from 6331180 to f60f5bc Compare July 19, 2016 10:59

DavidS force-pushed the experiments/puppet4 branch from f60f5bc to 9f765c0 Compare July 26, 2016 09:54

DavidS mentioned this pull request Jul 26, 2016

(FM-5598) NTP 5.0.0 features and release-prep #335

Closed

DavidS force-pushed the experiments/puppet4 branch 2 times, most recently from 0692f3b to 64db8cd Compare July 28, 2016 16:54

alvagante reviewed Aug 3, 2016
View reviewed changes

DavidS force-pushed the experiments/puppet4 branch 7 times, most recently from b87a645 to 1515af2 Compare September 12, 2016 11:27

ripienaar reviewed Sep 13, 2016
View reviewed changes

DavidS force-pushed the experiments/puppet4 branch from 1515af2 to a3291d8 Compare September 16, 2016 11:11

DavidS force-pushed the experiments/puppet4 branch from a24dbdd to eea5e2a Compare September 30, 2016 18:04

ahpook reviewed Sep 30, 2016

View reviewed changes

alvagante reviewed Oct 4, 2016

View reviewed changes

DavidS force-pushed the experiments/puppet4 branch 7 times, most recently from 5378b7a to 9f8be59 Compare October 14, 2016 15:55

DavidS force-pushed the experiments/puppet4 branch from 9f8be59 to 7c728d8 Compare October 18, 2016 19:15

DavidS mentioned this pull request Oct 18, 2016

Replacing anchor functionality with contain DavidS/puppetlabs-ntp#8

Closed

DavidS and others added 2 commits October 26, 2016 21:53

Replacing anchor functionality with contain

77fbf3f

DavidS force-pushed the experiments/puppet4 branch from 7c728d8 to 77fbf3f Compare October 26, 2016 20:53

DavidS changed the base branch from master to release October 28, 2016 14:28

DavidS changed the title ~~[REVIEW ONLY] Puppet 4 cleanups~~ Release Prep 6.0.0 Oct 28, 2016

DavidS changed the title ~~Release Prep 6.0.0~~ Remove deprecated functionality and prep for 6.0.0 release Oct 28, 2016

bmjen merged commit c4f586c into puppetlabs:release Oct 28, 2016

DavidS deleted the experiments/puppet4 branch August 8, 2018 12:35

		@@ -0,0 +1,54 @@
		---
		ntp::authprov: ~

Remove deprecated functionality and prep for 6.0.0 release #332

Remove deprecated functionality and prep for 6.0.0 release #332

Conversation

DavidS commented Jun 29, 2016

ripienaar commented Jun 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robinbowes commented Jun 29, 2016

DavidS commented Jun 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidS commented Jul 19, 2016

CorbyAsCode commented Jul 29, 2016

DavidS commented Jul 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidS commented Sep 13, 2016

Choose a reason for hiding this comment

ahpook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment