Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BREAKING: refactor hiera data lookup #76

Merged
merged 1 commit into from Sep 12, 2017

Conversation

dhollinger
Copy link
Member

Replacing the deprecated hiera_hash functions with the lookup function. Additionally, I have replaced the create_resources function calls using the method found at https://docs.puppet.com/puppet/5.1/lang_resources_advanced.html#implementing-the-createresources-function

That replacement gives users and contributors alike a better idea as to what the code is actually doing when generating the defined resources.

@@ -86,11 +86,15 @@
}

if $mounts {
$data = hiera_hash('autofs::mounts', $mounts)
create_resources('autofs::mount', $data)
lookup('autofs::mounts', { default_value => $mounts, merge => hash })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't use merge => deep then there's no point in doing the lookup at all. You may as well just use $mounts.

@@ -86,11 +86,15 @@
}

if $mounts {
$data = hiera_hash('autofs::mounts', $mounts)
create_resources('autofs::mount', $data)
lookup('autofs::mounts', { default_value => $mounts, merge => hash })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the merge strategy should be defined in hiera itself.

https://docs.puppet.com/puppet/5.1/hiera_merging.html#configuring-merge-behavior-in-hiera-data

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the merge strategy is defined as lookup_options there is also no use of calling lookup() for autofs::mounts because the automatic parameter lookup will do it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm avoiding anything that is not explicitly available in Puppet 4.7.0

While Hiera 4 does exists, it's technically "beta" until Puppet 4.9.0, hence why I'm defining merge strategy in the lookup instead. I'm am unsure if lookup_options is available for Hiera 3

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. Realistically I think using lookup_options would mean we'd only support a very recent puppet 4.10.x (think 4.10.6) or puppet 5. Hiera 5 was officially available (and not experimental) since 4.9.0, but I think it was also really buggy.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like using pure puppet functions and letting hiera just apply on class params.

Optional[Hash] $mounts = undef,
Optional[Hash] $maps = undef,
Optional[Hash] $mounts = {},
Optional[Hash] $maps = {},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional can be dropped if you set a default. It can even be Hash[String, Hash]

create_resources('autofs::map',$_datamaps)
}
$mounts.each |String $mount, Hash $attributes| {
autofs::mount { $mount: * => $attributes }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Odd indenting here

@dhollinger dhollinger added backwards-incompatible enhancement New feature or request needs-feedback Further information is requested labels Sep 12, 2017
@dhollinger dhollinger changed the title replace hiera_hash with lookup BREAKING: replace hiera_hash with lookup Sep 12, 2017
@dhollinger dhollinger changed the title BREAKING: replace hiera_hash with lookup BREAKING: refactor hiera data lookup Sep 12, 2017
Copy link
Member

@alexjfisher alexjfisher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but probably commits want squashing before merge.

Hiera lookups now use the default automatic lookup
merge options. README has been updated to note how
to perform a Hash or Deep merge from within your
data file using the lookup_options key
@dhollinger dhollinger removed the needs-feedback Further information is requested label Sep 12, 2017
@dhollinger dhollinger merged commit 00896e0 into voxpupuli:master Sep 12, 2017
@dhollinger dhollinger deleted the replace_hiera_with_lookup branch September 12, 2017 22:03
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for minimal code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants