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

16856 - Add a module data directory and hiera backend to consume it #1217

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ripienaar
Contributor

ripienaar commented Oct 8, 2012

While hiera does a decent job of separating code and data for users
it is quite difficult for module authors to use hiera to create reusable
modules. This is because the puppet backend is optional and even when
it is loaded the module author cannot influence the hierarchy.

With this commit we add a new module_data backend that loads data from
within a module and allow the module author to set a hierarchy for this
data.

The goal of this backend is to allow module authors to specify true
module default data in their modules but still allow users to override
the data using the standard method - especially useful with the puppet 3
hiera integration.

This backend is always loaded as the least important tier in the
hierarchy - unless a user choose to put it somewhere specific, but this
backend will always be enabled.

Given a module layout:

your_module
├── data
│   ├── hiera.yaml
│   └── osfamily
│       ├── Debian.yaml
│       └── RedHat.yaml
└── manifests
    └── init.pp

The hiera.yaml is optional in this example it would be:


---
:hierarchy:
- %{::osfamily}
- common

But when no hiera.yaml exist in the module, the default would be:


---
:hierarchy:
- common

The data directory is then a standard Hiera data store.

@ahpook

View changes

lib/hiera/backend/module_data_backend.rb Outdated
Hiera.debug("Reading config from %s file" % module_config)
config = load_data(module_config)
rescue => e
Hiera.debug("Failed to parse config file %s: %s: %s" % [module_config, e.class, e.to_s])

This comment has been minimized.

@ahpook

ahpook Dec 12, 2012

Contributor

should this be a higher error level than debug? this means it will silently fail unless ppl are running puppet master --debug, right?

This comment has been minimized.

@ripienaar

ripienaar Dec 12, 2012

Contributor

agreed

This comment has been minimized.

@ripienaar

ripienaar Dec 13, 2012

Contributor

have updated this pull request with warn level logging

config["path"] = path
return default_config.merge(config)

This comment has been minimized.

@ahpook

ahpook Dec 12, 2012

Contributor

what would be in the per-module config file? a special hierarchy that only applies to the module?

trying to understand the use case -- wouldn't we just need to include a data file that has a default value for each parameter?

This comment has been minimized.

@ripienaar

ripienaar Dec 12, 2012

Contributor

I think the PR message covers this? I can gist up a sample module but I think it's in the main message

This comment has been minimized.

@ripienaar

ripienaar Dec 13, 2012

Contributor

I updated the PR message and the first commit to show the relationship of the config files as per our phone call

ripienaar added some commits Oct 8, 2012

16856 - Add a module data directory and hiera backend to consume it
While hiera does a decent job of separating code and data for users
it is quite difficult for module authors to use hiera to create reusable
modules.  This is because the puppet backend is optional and even when
it is loaded the module author cannot influence the hierarchy.

With this commit we add a new module_data backend that loads data from
within a module and allow the module author to set a hierarchy for this
data.

The goal of this backend is to allow module authors to specify true
module default data in their modules but still allow users to override
the data using the standard method - especially useful with the puppet 3
hiera integration.

This backend is always loaded as the least important tier in the
hierarchy - unless a user choose to put it somewhere specific, but this
backend will always be enabled.

Given a module layout:

    your_module
    ├── data
    │   ├── hiera.yaml
    │   └── osfamily
    │       ├── Debian.yaml
    │       └── RedHat.yaml
    └── manifests
        └── init.pp

The hiera.yaml is optional in this example it would be:

    ---
    :hierarchy:
    - %{::osfamily}
    - common

But when no hiera.yaml exist in the module, the default would be:

    ---
    :hierarchy:
    - common

The data directory is then a standard Hiera data store.
16856 - Add a module data directory and hiera backend to consume it
The module HieraPuppet exist to be a helper that the various parts of
Puppet use to access Hiera - parts like the various parser functions and
the indirector.

Before this commit the indirector did not use this module instead it
duplicated a bunch of functionality and tests.

This commit refactors this to use the module and provide some
consistency and a single place to affect behavior changes
16856 - Add a module data directory and hiera backend to consume it
Adjust the logging level to warn when module config file fails to parse
end
end
def lookup(key, scope, order_override, resolution_type)

This comment has been minimized.

@zaphod42

zaphod42 Dec 18, 2012

Contributor

This method ends up being an implementation of Hiera inside Hiera but only supporting the yaml backend. This seems less than ideal to me since the user will be creating a hiera.yaml file that isn't the same as the file used by hiera itself and could just lead to confusion.

This would be better implemented by using hiera in here to get data local to the module.

This comment has been minimized.

@ripienaar

ripienaar Dec 18, 2012

Contributor

would you suggest constructing a new Hiera instance pointing to this file as a config file? I guess thats feasable, would need to see. As far as actual duplication go it's actually not much.

Further I think is actually quick close to the patterns found in other backends see

https://github.com/puppetlabs/puppet/blob/master/lib/hiera/backend/puppet_backend.rb#L46-99
https://github.com/puppetlabs/hiera/blob/master/lib/hiera/backend/json_backend.rb#L10-43

These code blocks is not particularly less complex than the code here - but I agree its not DRY and this stuff can be simpler but in current hiera code base there really isnt that much we can do about it.

I think the real problem here is captured in http://projects.puppetlabs.com/issues/10362 and only once we really address that can we end up with significantly better backends.

@backend = Module_data_backend.new(@cache)
end
describe "#load_module_config" do

This comment has been minimized.

@zaphod42

zaphod42 Dec 18, 2012

Contributor

This shouldn't be a public method of this class. It looks like there is a config file class here.

end
end
describe "#load_data" do

This comment has been minimized.

@zaphod42

zaphod42 Dec 18, 2012

Contributor

This is an internal method that should be private and so not tested here.

This comment has been minimized.

@ripienaar

ripienaar Dec 18, 2012

Contributor

what's the preferred way to test this then? I know some people believe not to test private methods but the complexity here certainly warrants it

This comment has been minimized.

@ripienaar

ripienaar Dec 18, 2012

Contributor

Just to be clear, I get the conventional wisdom about taking the private code out into another class whose purpose it is to do what the private method does - then test it there.

But I dont think this code lends itself well to this approach - we could consider making the config load class a helper on the Hiera::Config class or something along those line then refactor this backend and the config class to use this new shared method. I'd generally handle a refactor of that scope in another ticket though.

@cache = cache
else
begin
require 'hiera/filecache'

This comment has been minimized.

@zaphod42

zaphod42 Dec 18, 2012

Contributor

Why would we expect this to fail? It seems like this is just dependent on getting puppetlabs/hiera#98.

This comment has been minimized.

@ripienaar

ripienaar Dec 18, 2012

Contributor

indeed, if thats merged we can simplify this code

end
end
config = {} unless config.is_a?(Hash)

This comment has been minimized.

@zaphod42

zaphod42 Dec 18, 2012

Contributor

Wouldn't this end up ignoring an improperly formed hiera.yaml? It should probably error in that case.

This comment has been minimized.

@ripienaar

ripienaar Dec 18, 2012

Contributor

agree, I thought I saw similar treatment elsewhere and kept it for consistency but cant find that now.

else
data = YAML.load_file(path)
unless data.is_a?(Hash)
data = {}

This comment has been minimized.

@zaphod42

zaphod42 Dec 18, 2012

Contributor

Wouldn't this cover up a malformed data file? Shouldn't it error in this case?

This comment has been minimized.

@ripienaar

ripienaar Dec 18, 2012

Contributor

yeah I think either way has merits, I don't really care which if failing her is preferred that's fine.

16856 - Add a module data directory and hiera backend to consume it
While DRYing the Hiera indirector to use the HieraPuppet class a slight
behavior change in the HieraPuppet from the code in the indirector was
not noticed which meant that in the case where Hiera found no data an
Puppet::ParseError exception from the HieraPuppet class was not handled
by the indirector which prevented code like:

   class x($y=$x::y::z) inherits x::y { }

from working correctly, the value for $x::y::z would not be used as
a default here
@adrienthebo

This comment has been minimized.

Contributor

adrienthebo commented Mar 18, 2013

My understanding is that this pull request doesn't address all the requirements for hiera data in modules, per the conversation in the redmine ticket (http://projects.puppetlabs.com/issues/16856). I'm closing this pull request for the time being until these things get resolved so we don't have stale/forgotten pull requests. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment