(FM-7230) Specify defaults for multiple devices when using Hiera or Classifier

[tkishel]

This commit implements global and device-type levels of default values.
These defaults are used with device_manager::devices.

This commit also updates the documentation and examples for clarity,
and updates the same examples in the spec tests for consistency.

[shermdog]

This looks great. I did uncover one thing while testing - once we add devices via hiera there doesn't appear to be an easy way to remove all nodes. There is some implicit purge that happens in the module when you manage at least one device, but not if you remove them all.

I thought device_manager::devices: {} would do the trick but no dice.

This affects both this PR and the current version of the module, so I wouldn't say we need to gate this PR on a fix, but I leave that to the expert @tkishel to decide :)

[tkishel]

Uff da :) I have thought about some options to address this: I’ll followup on Monday ...

…

On Mon, Jul 30, 2018 at 9:31 AM Rick Sherman ***@***.***> wrote: This looks great. I did uncover one thing while testing - once we add devices via hiera there doesn't appear to be an easy way to remove all nodes. There is some implicit purge that happens in the module when you manage at least one device, but not if you remove them all. I thought device_manager::devices: {} would do the trick but no dice. This affects both this PR and the current version of the module, so I wouldn't say we need to gate this PR on a fix, but I leave that to the expert @tkishel <https://github.com/tkishel> to decide :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABid_04shDQqVrYRCFtJy6_eO2X0hieDks5uLzT8gaJpZM4VkXOR> .

[tkishel]

The implicit purge is a result of includes of device_manager::conf and device_manager::fact, which use concat. I've added those includes to devices.pp, allowing an empty device_manager::devices hash to purge unmanaged devices from conf and fact. But any associated Cron (or Scheduled Task) resources will be orphaned by an empty device_manager::devices hash. (This is true of any resource transitioning form managed to unmanaged instead of being declared as absent: consider declaring a File resource, running puppet, then removing that File resource declaration, then running puppet).

Best practice is to ensure => absent via device defaults, which the latest commit now allows.

[tkishel]

@shermdog It occurred to me that, based upon key name, users might misinterpret the device_manager::defaults key used by the device_manager::devices class to also be used by the base device_manager defined type. Maybe we should rename the key to device_manager::devices::defaults ?

Let me know if you agree, and if you would like these commits squashed, and I will do so.

[shermdog]

@tkishel that took me a bit to suss out :) You raise a good point. Would it actually make sense to add the defaults hash lookup to the defined type so users could take advantage of those defaults even when explicitly using the defined type? (Assuming that's actually possible).

[tkishel]

A quick test shows that automatic parameter lookup does not support defined types, but I'll double-check my results, because I thought it did (at least in Puppet 5+). If not, I don't know if converting the device_manager from a defined type to a class is appropriate in order to provide defaults for individual device_manager resource declarations (that would seem to better match the use case for device_manager::devices) and defaults can be declared via resource default statements:

Device_manager {
  run_interval => 99,
}

Implementing type-level (f5, cisco_ios, ...) defaults rules out using a standard Hiera deeper merge. Adding or moving the defaults hash lookup to the device_manager defined type somehow feels ... expensive. So, I favor device_manager::devices::defaults but am open to being convinced otherwise.

[shermdog]

@tkishel your opinion is 💯 authoritative - Go ahead and change to device_manager::devices::defaults and squash :)

[tkishel]

Well, I don't know about being authoritative.

Research / Testing:

Defined types do not support automatic parameter lookup.
Converting the device_manager defined type to a class is not appropriate.
Resource default statements are not appropriate when an attribute is sensitive (such as password).

But ...

Adding lookups to the device_manager defined type wasn't expensive.
I was able to implement global and device-type defaults with deep_merge.

So ...

I've submitted PR #25 as an alternative: it implements global and device-type defaults
in both the device_manager defined type and the device_manager::devices class.

[tkishel]

@shermdog I've squashed in any case, to make it easier to compare PRs.

If you prefer this PR, I can change to device_manager::devices::defaults in this PR.

If you prefer PR #25, I can close this PR.

If you prefer just devices.pp from PR # 25 (which replaces merge_default and value_or_default functions with deep_merge) I can update this PR with that change.

[tkishel]

I’ve reviewed this with Reid and I’ll make one more push to consolidate these PRs in the morning ....

[tkishel]

Reid suggested modeling the device data structures after the Inventory file in Bolt, but I could not translate that cleanly without increasing the verbosity. I did incorporate some of his other suggestions.

Closed PR # 25, changed device_manager::defaults to device_manager::devices::defaults in this PR, and squashed the commits. Ready to go, @shermdog