Uploaded image for project: 'Puppet'
  1. Puppet
  2. PUP-5395

Set resolution_type when using data bindings by using lookup_options

    Details

    • Template:
    • Story Points:
      1
    • Sprint:
      Language 2015-10-28, Language 2015-11-11, Language 2015-12-02
    • Release Notes:
      New Feature
    • Release Notes Summary:
      Hide
      This adds a new feature to automatic data binding and the lookup function that allows the lookup options to be defined in the data by binding keys in a hash bound to the key {{lookup_options}}. This makes it possible to get deep merge behavior for select class parameters.
      Show
      This adds a new feature to automatic data binding and the lookup function that allows the lookup options to be defined in the data by binding keys in a hash bound to the key {{lookup_options}}. This makes it possible to get deep merge behavior for select class parameters.

      Description

      Because it's the responsibility of the caller to set `resolution_type` in hiera, and there is no explicit caller when using data bindings, there's no way to tell hiera to use the array (append) or hash (merge) resolutions. This is a pretty bad flaw as it prevents sophisticated lookups like deep merges (or, well, any merges at all) when using the fancy data bindings.

      UPDATE


      This new feature allows the default lookup options to be individually controlled per parameter by binding a hash to the key lookup_options, where each key in that hash is the fully qualified name of a parameter. The value should be a hash that defines the default lookup options for that key.

      The lookup_options are looked up with hash-merge, making it possible to compose the content of the lookup_options hash using multiple sources (data files in a hierarchy, across, global hiera, environment data provider and module providers. With data in modules in effect, this means that a module can specify the behavior of its own parameters (e.g. 'merge' for some of its keys). At higher priority layers no action needs to be taken except if there is a desire to change the default specification.

      The options bound per key under lookup_options are also used as the defaults for the lookup function.
      Note that the defaults does not apply to the hiera_xxx family of functions, they continue to work as before (as these functions themselves represents specifying some of the options (e.g. hiera_array is for arrays, and hiera_hash is for hashes). It is recommended to always use the lookup function instead of the hiera_xxx functions since they a) work with "data in modules", and b) makes use of the default lookup options.

      The only thing that can be specified (currently) is the merge option (as specified by the lookup function). This means that the behavior can be one of the strings: first, unique, hash, merge, deep, or a hash with its strategy key set to one of those strings, and where further options are knock_out_prefix, sort_merged_arrays, unpack_arrays, and merge_hash_arrays - (See the lookup function for details).

      The lookup_options will never be found when used as the key in a regular lookup. It's not considered to be data in the normal sense but rather meta_data describing the data. Also, there is never a need to prefix the lookup_options key with a module name since the module name will be determined by looking at the key that initiated the lookup.

        Issue Links

          Activity

          Hide
          chuck Charlie Sharpsteen added a comment -

          This one is tricky because it might require a change in how parameterized classes are declared.

          Eric Sorenson looks like this needs a decision

          Show
          chuck Charlie Sharpsteen added a comment - This one is tricky because it might require a change in how parameterized classes are declared. Eric Sorenson looks like this needs a decision
          Hide
          eric.sorenson Eric Sorenson added a comment -

          No, what it needs is a some technical design work.

          Show
          eric.sorenson Eric Sorenson added a comment - No, what it needs is a some technical design work.
          Hide
          eric.sorenson Eric Sorenson added a comment -

          Andy I've put this in Designing and assigned it to you for assessment or delegation

          Show
          eric.sorenson Eric Sorenson added a comment - Andy I've put this in Designing and assigned it to you for assessment or delegation
          Hide
          ghoneycutt garrett honeycutt added a comment -

          +1 Currently I am attempting to account for the use of merge lookups in puppet code, which is not very nice.
          https://github.com/ghoneycutt/puppet-module-ssh/blob/v3.6.0/manifests/init.pp#L280-286

          Show
          ghoneycutt garrett honeycutt added a comment - +1 Currently I am attempting to account for the use of merge lookups in puppet code, which is not very nice. https://github.com/ghoneycutt/puppet-module-ssh/blob/v3.6.0/manifests/init.pp#L280-286
          Hide
          jcbollinger John Bollinger added a comment -

          When I thought a bit about this because of a question on puppet-users, I realized that it is not an issue that can be solved by a change to Puppet DSL or to the way Puppet uses Hiera internally. Whether a priority lookup, a hash-merge lookup, or an array lookup is wanted in a particular data-binding situation is not (or at least should not be) a function of the class. Rather, it is an aspect of the data themselves.

          That is, even if there were a way to express in a class definition that a particular parameter should be bound via a hash-merge lookup (for example), that would just turn the problem around on people who want to use a priority lookup for that parameter.

          And, wow, I just saw the broader implication: the same argument applies to any use of hiera_array() or hiera_hash(). The intended meaning of the data is part of the data definition, and looking it up via a different function than intended changes that meaning. Hiera (the YAML back end) does not have a mechanism to express this aspect of the data, so the data consumer is left to guess which of three functions is the appropriate one to use for retrieval, without even a means to check after the fact whether they guessed right.

          Bottom line: hiera_hash() and hiera_array() should both be deprecated in favor of a means to express the intended retrieval type directly in the data. The hiera() function, whether invoked from the DSL or internally by Puppet, should use that means to retrieve the data correctly.

          Show
          jcbollinger John Bollinger added a comment - When I thought a bit about this because of a question on puppet-users, I realized that it is not an issue that can be solved by a change to Puppet DSL or to the way Puppet uses Hiera internally. Whether a priority lookup, a hash-merge lookup, or an array lookup is wanted in a particular data-binding situation is not (or at least should not be) a function of the class. Rather, it is an aspect of the data themselves. That is, even if there were a way to express in a class definition that a particular parameter should be bound via a hash-merge lookup (for example), that would just turn the problem around on people who want to use a priority lookup for that parameter. And, wow, I just saw the broader implication: the same argument applies to any use of hiera_array() or hiera_hash(). The intended meaning of the data is part of the data definition, and looking it up via a different function than intended changes that meaning. Hiera (the YAML back end) does not have a mechanism to express this aspect of the data, so the data consumer is left to guess which of three functions is the appropriate one to use for retrieval, without even a means to check after the fact whether they guessed right. Bottom line: hiera_hash() and hiera_array() should both be deprecated in favor of a means to express the intended retrieval type directly in the data. The hiera() function, whether invoked from the DSL or internally by Puppet, should use that means to retrieve the data correctly.
          Hide
          andy Andrew Parker added a comment -

          John Bollinger hit the nail on the head. The fundamental problem here is that in the model that has been put together with the data-bindings and with hiera there are 3 parts: the "name" of the data, the multiple sources of the data, the mechanism by which the multiple sources will be combined. The current system of hiera(), heira_array() and hiera_hash() exposes the combining mechanism to the users, but also puts the burden of getting the correct data for a given name on the user. In effect, what that has done is exposed a very limited query language to users. I think what is really needed, in order to support how puppet actually tries to use hiera is that the combining mechanism is inside hiera and cannot be changed from the lookup side. In that case along with the hierarchy and data sources, there also needs to be a way of specifying how to perform the lookup and combine in the hiera configuration.

          I hit this problem when I tried to spike out different ways of implementing something like hiera. We see this in all of the requests for functions to perform lookups inside the hiera data (they are attempts to construct queries, in a sense).

          Show
          andy Andrew Parker added a comment - John Bollinger hit the nail on the head. The fundamental problem here is that in the model that has been put together with the data-bindings and with hiera there are 3 parts: the "name" of the data, the multiple sources of the data, the mechanism by which the multiple sources will be combined. The current system of hiera() , heira_array() and hiera_hash() exposes the combining mechanism to the users, but also puts the burden of getting the correct data for a given name on the user. In effect, what that has done is exposed a very limited query language to users. I think what is really needed, in order to support how puppet actually tries to use hiera is that the combining mechanism is inside hiera and cannot be changed from the lookup side. In that case along with the hierarchy and data sources, there also needs to be a way of specifying how to perform the lookup and combine in the hiera configuration. I hit this problem when I tried to spike out different ways of implementing something like hiera. We see this in all of the requests for functions to perform lookups inside the hiera data (they are attempts to construct queries, in a sense).
          Hide
          tedivm Robert added a comment -

          It would be nice if there were some way to define this merge behavior, as it would seriously increase the flexibility and general awesomeness of hiera.

          Show
          tedivm Robert added a comment - It would be nice if there were some way to define this merge behavior, as it would seriously increase the flexibility and general awesomeness of hiera.
          Hide
          traylenator Steve Traylen added a comment - - edited

          Thisi is not possible since hiera_array cannot be used as a lookup functions. However if it were possible the following might work so long as it's the winning setting under the current data binding.

          ---
          mymodule::settings:
            - value1
            - value2
            - {hiera_array('mymodule::settings')}
          

          clearly some potential recursion problems but.

          Show
          traylenator Steve Traylen added a comment - - edited Thisi is not possible since hiera_array cannot be used as a lookup functions . However if it were possible the following might work so long as it's the winning setting under the current data binding. --- mymodule::settings: - value1 - value2 - {hiera_array('mymodule::settings')} clearly some potential recursion problems but.
          Hide
          rnelson0@gmail.com Rob Nelson added a comment -

          At the contributor's summit last week, Henrik, Felix, Vanessa and myself hashed out a potential solution. Our proposal requires two pieces.

          In puppet.conf's master section, hiera_advanced_parameter_bindings must be set to true.

          [master]
          hiera_advanced_parameter bindings = true
          

          If a key of lookupoptions::<key> is found and it contains the hash key 'merge' with a valid value (typically 'deep'), the APL merge strategy is set to the specified value. This key can be found anywhere but it is suggested to be placed in a more specific location, to give per-tier control over the merge strategy (i.e. do not set it in global/common, set it in a clientcert or datacenter/network/etc. type hierarchy level). It would look like this:

          lookupoptions::mysql::server::overrideoptions:
            merge: deep
          

          Then, any keys matching <key> would be found during APL and merged using the specified behavior.

          You can see the few changes required to make the APL lookups work at https://github.com/ffrank/puppet/commit/9ba281986f88cb9d889fa02a650779ef92d0b6eb. If this proposal is accepted, the puppet.conf setting and tests can be generated and docs fleshed out, but I'd like to know if this proposal is sufficient before fleshing out the changes and tweaking debug messages, etc.

          Thanks to Henrik, Vanessa, and Felix for their assistance during the summit!

          Show
          rnelson0@gmail.com Rob Nelson added a comment - At the contributor's summit last week, Henrik, Felix, Vanessa and myself hashed out a potential solution. Our proposal requires two pieces. In puppet.conf's master section, hiera_advanced_parameter_bindings must be set to true. [master] hiera_advanced_parameter bindings = true If a key of lookupoptions::<key> is found and it contains the hash key 'merge' with a valid value (typically 'deep'), the APL merge strategy is set to the specified value. This key can be found anywhere but it is suggested to be placed in a more specific location, to give per-tier control over the merge strategy (i.e. do not set it in global/common, set it in a clientcert or datacenter/network/etc. type hierarchy level). It would look like this: lookupoptions::mysql::server::overrideoptions: merge: deep Then, any keys matching <key> would be found during APL and merged using the specified behavior. You can see the few changes required to make the APL lookups work at https://github.com/ffrank/puppet/commit/9ba281986f88cb9d889fa02a650779ef92d0b6eb . If this proposal is accepted, the puppet.conf setting and tests can be generated and docs fleshed out, but I'd like to know if this proposal is sufficient before fleshing out the changes and tweaking debug messages, etc. Thanks to Henrik, Vanessa, and Felix for their assistance during the summit!
          Hide
          ghoneycutt garrett honeycutt added a comment -

          +1 I like this approach because it is a clear pattern that can be expressed entirely in Hiera. With regards to my example, it moves the logic from being implemented in the module to a standard interface in Hiera, saving me a ton of code and spec tests

          Show
          ghoneycutt garrett honeycutt added a comment - +1 I like this approach because it is a clear pattern that can be expressed entirely in Hiera. With regards to my example, it moves the logic from being implemented in the module to a standard interface in Hiera, saving me a ton of code and spec tests
          Hide
          jcbollinger John Bollinger added a comment -

          +1 for lookupoptions. This seems to be a viable way to define the lookup type on a per-item basis.

          Questions:

          • Is it intended that a merge strategy being defined for a given key indicates that ordinary lookups for that key should employ merging (of the specified flavor)? The alternative would be that only hash-merge lookups that happen to be performed for the key, as requested by some other means, would employ the specified strategy.
          • The details presented in the proposal seem specific to hash-valued items. Supposing that the answer to my previous question is "yes", is it intended or desirable that array lookup type can be specified this way as well (i.e. as if hiera_array() were used)?
          • I think the proposal says that the lookupoptions::* keys are intended themselves to be looked up in the data hierarchy. What type of lookup is used for them? Does the facility they provide apply recursively?
          Show
          jcbollinger John Bollinger added a comment - +1 for lookupoptions . This seems to be a viable way to define the lookup type on a per-item basis. Questions: Is it intended that a merge strategy being defined for a given key indicates that ordinary lookups for that key should employ merging (of the specified flavor)? The alternative would be that only hash-merge lookups that happen to be performed for the key, as requested by some other means, would employ the specified strategy. The details presented in the proposal seem specific to hash-valued items. Supposing that the answer to my previous question is "yes", is it intended or desirable that array lookup type can be specified this way as well ( i.e. as if hiera_array() were used)? I think the proposal says that the lookupoptions::* keys are intended themselves to be looked up in the data hierarchy. What type of lookup is used for them ? Does the facility they provide apply recursively?
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          The options should be given as for the lookup function (it covers the functionality of hiera, hiera_array and hiera_hash and works with both hiera as well as "data in modules" (a.k.a data providers).

          The puppet setting should not be specific to "hiera" - it is a data-binder feature.

          The lookup options for the key only applies to automatic data binding. For other use cases (i.e. directly calling lookup the user can manually look up the lookup options for the key. For manual calls to the hiera_xxx functions, the user also needs to pick a specific hiera_xxx call based on the options as well as transforming the options to adapt to the hiera_xxx functions limited set of features.

          Show
          henrik.lindberg Henrik Lindberg added a comment - The options should be given as for the lookup function (it covers the functionality of hiera , hiera_array and hiera_hash and works with both hiera as well as "data in modules" (a.k.a data providers). The puppet setting should not be specific to "hiera" - it is a data-binder feature. The lookup options for the key only applies to automatic data binding. For other use cases (i.e. directly calling lookup the user can manually look up the lookup options for the key. For manual calls to the hiera_xxx functions, the user also needs to pick a specific hiera_xxx call based on the options as well as transforming the options to adapt to the hiera_xxx functions limited set of features.
          Hide
          rnelson0@gmail.com Rob Nelson added a comment -

          John:

          It's possible in the future to set up lookupoptions::* for the class, but that won't be in this first revision.

          I'll see if I can get some docs and tests ready this weekend.

          Show
          rnelson0@gmail.com Rob Nelson added a comment - John: This would only be for automatic parameter lookup according to the merge strategy defined at https://github.com/puppetlabs/puppet/blob/d2199af906fa5ca19ec0c37c82794934ea4fb4b5/lib/puppet/functions/lookup.rb#L58-L59 This would work for arrays as well as hashes. The lookupoptions are done as a first-fit lookup (standard). Since the value can be set at any level of the hierarchy this lets the user define at whatever level makes sense, without worrying about conflicts or recursion. It's possible in the future to set up lookupoptions::* for the class, but that won't be in this first revision. I'll see if I can get some docs and tests ready this weekend.
          Hide
          jcbollinger John Bollinger added a comment -

          The proposed solution seems a win to me, but it also seems to be missing an opportunity to be an even bigger win. Going back to my May, 2014 comments:

          Bottom line: hiera_hash() and hiera_array() should both be deprecated in favor of a means to express the intended retrieval type directly in the data. The hiera() function, whether invoked from the DSL or internally by Puppet, should use that means to retrieve the data correctly.

          To put it another way, the problem stated in the title of this issue is only one aspect of a deeper problem, which I described in those earlier comments. Those comments seemed to be acknowledged and well received at the time. I really hope the whole problem can be solved, especially since a partial solution now greatly reduces the likelihood that a complete solution will ever be implemented.

          Show
          jcbollinger John Bollinger added a comment - The proposed solution seems a win to me, but it also seems to be missing an opportunity to be an even bigger win. Going back to my May, 2014 comments: Bottom line: hiera_hash() and hiera_array() should both be deprecated in favor of a means to express the intended retrieval type directly in the data. The hiera() function, whether invoked from the DSL or internally by Puppet, should use that means to retrieve the data correctly. To put it another way, the problem stated in the title of this issue is only one aspect of a deeper problem, which I described in those earlier comments. Those comments seemed to be acknowledged and well received at the time. I really hope the whole problem can be solved, especially since a partial solution now greatly reduces the likelihood that a complete solution will ever be implemented.
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          John Bollinger Completely agree with you. The current proposal is really a stop-gap solution for automatic data binding only. It would be much better if the data composition was completely done in the data and that it does not require the user to know how to look something up.

          The problem with implementing this is that we have not come up with a good solution that would be both performant and backwards compatible; the hiera data formats pretty much blocks all attempts to add meaning to any combinations of keys and values. For that reason, we decided that the first implementation of "Data in Modules" (which is really the beginning of a refactored and improved Hiera) should be 100% data file compatible. Going forward, I think we should introduce new backends that when used allows richer features in the data files themselves; awareness of data types, ability to alias/interpolate/combine/transform values other than strings; which includes merging of arrays and hashes, to mention a few.

          If you recall ARM-8, this was very much a goal of that proposal. It turned out to be too complex and was rejected as the mechanism for data binding. There are however many concepts from that ARM that we can use and make available in simpler form expressed in hiera data.

          As the proposal at hand now provides a much sought after feature I think it is of value to get it into a Hiera 3 release even if we do not yet have a design for the desired features, and that it means having to make some changes to data files when such a solution is available. I think that is more favorable than waiting for the additional desired features to be designed.

          Show
          henrik.lindberg Henrik Lindberg added a comment - John Bollinger Completely agree with you. The current proposal is really a stop-gap solution for automatic data binding only. It would be much better if the data composition was completely done in the data and that it does not require the user to know how to look something up. The problem with implementing this is that we have not come up with a good solution that would be both performant and backwards compatible; the hiera data formats pretty much blocks all attempts to add meaning to any combinations of keys and values. For that reason, we decided that the first implementation of "Data in Modules" (which is really the beginning of a refactored and improved Hiera) should be 100% data file compatible. Going forward, I think we should introduce new backends that when used allows richer features in the data files themselves; awareness of data types, ability to alias/interpolate/combine/transform values other than strings; which includes merging of arrays and hashes, to mention a few. If you recall ARM-8, this was very much a goal of that proposal. It turned out to be too complex and was rejected as the mechanism for data binding. There are however many concepts from that ARM that we can use and make available in simpler form expressed in hiera data. As the proposal at hand now provides a much sought after feature I think it is of value to get it into a Hiera 3 release even if we do not yet have a design for the desired features, and that it means having to make some changes to data files when such a solution is available. I think that is more favorable than waiting for the additional desired features to be designed.
          Hide
          rnelson0@gmail.com Rob Nelson added a comment -

          I'm almost ready to submit the PR on puppet (will need to do puppet-docs as well) but I can't find any existing related tests. I'm modifying the function at https://github.com/puppetlabs/puppet/blob/master/lib/puppet/resource/type.rb#L564 and would expect those tests at https://github.com/puppetlabs/puppet/blob/master/spec/unit/resource/type_spec.rb. I'm not sure how to craft a new test to call lookup_external_default_for() with a mock up. I'll submit the PR without tests unless someone can point me in the right direction.

          Show
          rnelson0@gmail.com Rob Nelson added a comment - I'm almost ready to submit the PR on puppet (will need to do puppet-docs as well) but I can't find any existing related tests. I'm modifying the function at https://github.com/puppetlabs/puppet/blob/master/lib/puppet/resource/type.rb#L564 and would expect those tests at https://github.com/puppetlabs/puppet/blob/master/spec/unit/resource/type_spec.rb . I'm not sure how to craft a new test to call lookup_external_default_for() with a mock up. I'll submit the PR without tests unless someone can point me in the right direction.
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          Rob Nelson Have not looked at where similar tests are located, I would imagine they are related to data binding. It may also be that there are very few tests since it has always been troublesome with the circular dependencies between hiera and puppet; instead there is probably just unit tests for the respective features in the respective project.

          We can help sort out testing and documentation. We are however close to feature complete cutoff for Puppet 4.3.0, which means that we are swamped with work in need of final touches, so I cannot promise that we will have time to take this on, but we will do our best. Since the change can be done without a new hiera release being required, there is a bigger chance of getting this in.

          Show
          henrik.lindberg Henrik Lindberg added a comment - Rob Nelson Have not looked at where similar tests are located, I would imagine they are related to data binding. It may also be that there are very few tests since it has always been troublesome with the circular dependencies between hiera and puppet; instead there is probably just unit tests for the respective features in the respective project. We can help sort out testing and documentation. We are however close to feature complete cutoff for Puppet 4.3.0, which means that we are swamped with work in need of final touches, so I cannot promise that we will have time to take this on, but we will do our best. Since the change can be done without a new hiera release being required, there is a bigger chance of getting this in.
          Hide
          rnelson0@gmail.com Rob Nelson added a comment -

          Henrik Lindberg I didn't find any tests, but I documented my testing methods and attached both code and document PRs to this ticket. They're ready for review on this week's triage call, I should be able to attend it. Here's hoping to making Puppet 4.3.0

          Show
          rnelson0@gmail.com Rob Nelson added a comment - Henrik Lindberg I didn't find any tests, but I documented my testing methods and attached both code and document PRs to this ticket. They're ready for review on this week's triage call, I should be able to attend it. Here's hoping to making Puppet 4.3.0
          Hide
          josh Josh Cooper added a comment -

          Henrik Lindberg We reviewed this in PR triage and looked good. Rob Nelson discovered a pre-existing order dependent test failure, and I filed that as PUP-5401. I'm putting this in your teams sprint backlog.

          Show
          josh Josh Cooper added a comment - Henrik Lindberg We reviewed this in PR triage and looked good. Rob Nelson discovered a pre-existing order dependent test failure, and I filed that as PUP-5401 . I'm putting this in your teams sprint backlog.
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          Thanks Josh Cooper and Rob Nelson. I noted the ticket about the test order failure. Also commented on the PR.

          Show
          henrik.lindberg Henrik Lindberg added a comment - Thanks Josh Cooper and Rob Nelson . I noted the ticket about the test order failure. Also commented on the PR.
          Hide
          thomas.hallgren Thomas Hallgren added a comment -

          I question the use of a global parameter like hiera_advanced_parameter_bindings or data_binding_lookupoptions to control the behavior. If the intent is to make the lookup behavior an aspect of the data itself, then there should be no global override. Performance wise it shouldn't matter much if the behavior is instead controlled by checking the existence of the lookupoptions::* key. It has already been parsed into a hash.

          One thing that could make a small difference performance wise would be if we, instead of using a lookupoptions::* key, would use a lookupoptions key with an associated object where the subkeys are specified. That would avoid unnecessary string concatenations in order to build keys. E.g. instead of this:

          lookupoptions::mysql::server::override_options:
            merge: deep
          

          we could have this:

          lookupoptions:
             mysql::server::override_options:
                merge: deep
          

          Now the backend only needs to check of the presence of lookupoptions once and then use it's presence to control its behavior for all lookups. The actual key used for a lookup can also be used inside of the lookupoptions object. No more string concatenation.

          To really do a deep(er) merge will of course consume some more CPU-cycles but having a global flag to control this will not help. If the data layout was made with the intention of it being merged, then everything will crash if it's used some other way.

          Show
          thomas.hallgren Thomas Hallgren added a comment - I question the use of a global parameter like hiera_advanced_parameter_bindings or data_binding_lookupoptions to control the behavior. If the intent is to make the lookup behavior an aspect of the data itself, then there should be no global override. Performance wise it shouldn't matter much if the behavior is instead controlled by checking the existence of the lookupoptions::* key. It has already been parsed into a hash. One thing that could make a small difference performance wise would be if we, instead of using a lookupoptions::* key, would use a lookupoptions key with an associated object where the subkeys are specified. That would avoid unnecessary string concatenations in order to build keys. E.g. instead of this: lookupoptions::mysql::server::override_options: merge: deep we could have this: lookupoptions: mysql::server::override_options: merge: deep Now the backend only needs to check of the presence of lookupoptions once and then use it's presence to control its behavior for all lookups. The actual key used for a lookup can also be used inside of the lookupoptions object. No more string concatenation. To really do a deep(er) merge will of course consume some more CPU-cycles but having a global flag to control this will not help. If the data layout was made with the intention of it being merged, then everything will crash if it's used some other way.
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          That is nice. Needs to do a lookup of "lookupoptions" with a deep merge to avoid having to have central definition of all lookup options. But that is fine, far less expensive that looking up each lookupoption for each and every parameter.

          Show
          henrik.lindberg Henrik Lindberg added a comment - That is nice. Needs to do a lookup of "lookupoptions" with a deep merge to avoid having to have central definition of all lookup options. But that is fine, far less expensive that looking up each lookupoption for each and every parameter.
          Hide
          ffrank Felix Frank added a comment - - edited

          My knowledge on YAML is failing me. Does the merge allow the same key to appear multiple times in one hash declaration? I.e.

          lookupoptions:
            mysql_server:
              ...
          ...
          lookupoptions:
            apache:
              ...
          

          If this is an issue, then that would be a usability minus compared to the lookupoptions::* keys.

          Show
          ffrank Felix Frank added a comment - - edited My knowledge on YAML is failing me. Does the merge allow the same key to appear multiple times in one hash declaration? I.e. lookupoptions: mysql_server: ... ... lookupoptions: apache: ... If this is an issue, then that would be a usability minus compared to the lookupoptions::* keys.
          Hide
          thomas.hallgren Thomas Hallgren added a comment -

          There can only be one lookupoptions object per file which means that all overrides would be declared in one place (per file). I think there's both pros and cons to that approach. Not sure one outweighs the other.

          Show
          thomas.hallgren Thomas Hallgren added a comment - There can only be one lookupoptions object per file which means that all overrides would be declared in one place (per file). I think there's both pros and cons to that approach. Not sure one outweighs the other.
          Hide
          ffrank Felix Frank added a comment -

          True. So yes, I guess we can go with this design instead.

          Show
          ffrank Felix Frank added a comment - True. So yes, I guess we can go with this design instead.
          Hide
          rnelson0@gmail.com Rob Nelson added a comment -

          I like the idea of a lookupoptions key, that does seem more appealing than a global setting even if it's slightly more verbose.

          Show
          rnelson0@gmail.com Rob Nelson added a comment - I like the idea of a lookupoptions key, that does seem more appealing than a global setting even if it's slightly more verbose.
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          Thomas Hallgren also brought up (in a conversation we had) that it is confusing if the lookup options are not also the default lookup options when performing a lookup using the lookup function, and I agreed to that.

          For the hiera_xxx functions it may become problematic, since they are themselves already specifying one aspect of the lookup (the type), and there is a global parameter in hiera.yaml controlling how hash lookup is expected to work. As an example, should a hiera (priority) lookup perform a merge if the lookup options in hiera data specifies this, or is the call to hiera (priority) the way the user specifies that it should not do a merge (no matter what the default is)? It was for these reasons I originally said that the lookup options should only have effect for automatic data binding.

          Opinions? Is it ok if we leave the hiera_xxx functions as they are since they have the style that implies that the user knows the data configuration? Instead users should use the lookup function where the lookup options from data files becomes the default if not given as an argument to lookup.

          Show
          henrik.lindberg Henrik Lindberg added a comment - Thomas Hallgren also brought up (in a conversation we had) that it is confusing if the lookup options are not also the default lookup options when performing a lookup using the lookup function, and I agreed to that. For the hiera_xxx functions it may become problematic, since they are themselves already specifying one aspect of the lookup (the type), and there is a global parameter in hiera.yaml controlling how hash lookup is expected to work. As an example, should a hiera (priority) lookup perform a merge if the lookup options in hiera data specifies this, or is the call to hiera (priority) the way the user specifies that it should not do a merge (no matter what the default is)? It was for these reasons I originally said that the lookup options should only have effect for automatic data binding. Opinions? Is it ok if we leave the hiera_xxx functions as they are since they have the style that implies that the user knows the data configuration? Instead users should use the lookup function where the lookup options from data files becomes the default if not given as an argument to lookup .
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          Also agree to that we do not really need a puppet setting. If the lookupoptions key is there - the feature is on.

          Show
          henrik.lindberg Henrik Lindberg added a comment - Also agree to that we do not really need a puppet setting. If the lookupoptions key is there - the feature is on.
          Hide
          ffrank Felix Frank added a comment -

          Sounds like a good plan. I think this will even satisfy John Bollinger's request for consistent lookup behavior.

          Show
          ffrank Felix Frank added a comment - Sounds like a good plan. I think this will even satisfy John Bollinger 's request for consistent lookup behavior.
          Hide
          thomas.hallgren Thomas Hallgren added a comment -

          Henrik Lindberg, the way I see it, using the hiera_xxx functions is similar to explicitly specify the options with a parameter when using the lookup function. I.e. the user expresses a desire to override the default and should be able to do so. Therefore I'm in favor of keeping the hiera_xxx functions as they are.

          Another reason for this is that I view them as legacy functions that should be deprecated eventually. As such, it's better to just leave them as they are.

          Show
          thomas.hallgren Thomas Hallgren added a comment - Henrik Lindberg , the way I see it, using the hiera_xxx functions is similar to explicitly specify the options with a parameter when using the lookup function. I.e. the user expresses a desire to override the default and should be able to do so. Therefore I'm in favor of keeping the hiera_xxx functions as they are. Another reason for this is that I view them as legacy functions that should be deprecated eventually. As such, it's better to just leave them as they are.
          Hide
          thomas.hallgren Thomas Hallgren added a comment -

          All keys in a module are qualified with the module name. In fact, the module data provider is selected based on this key. A key that isn't prefixed with the module name will hence never reach a module data provider which means that the keys like lookupoptions or lookupoptions::* will never be found in a module. So in a module, the format must instead be:

          mysql::lookupoptions
              mysql::server::override_options:
                  merge: deep
          

          Show
          thomas.hallgren Thomas Hallgren added a comment - All keys in a module are qualified with the module name. In fact, the module data provider is selected based on this key. A key that isn't prefixed with the module name will hence never reach a module data provider which means that the keys like lookupoptions or lookupoptions::* will never be found in a module. So in a module, the format must instead be: mysql::lookupoptions mysql::server::override_options: merge: deep
          Hide
          jcbollinger John Bollinger added a comment -

          Is it ok if we leave the hiera_xxx functions as they are since they have the style that implies that the user knows the data configuration? Instead users should use the lookup function where the lookup options from data files becomes the default if not given as an argument to lookup.

          If the lookup() function honors the lookup options specified in the data, then I would be satisfied for hiera() to not do so. I agree that it does not make sense for the behavior of the hiera_xxx() functions to change in any case, but I do still recommend that those functions be deprecated. If lookup() behaves as suggested then the hiera() function should be deprecated, too. Overall, this could be less surprising than altering hiera() to sometimes perform hash-merge or array lookups.

          If deprecation is too strong to be palatable at this time, then at minimum start strongly promoting use of lookup() and the in-data lookup options as superior practice, which it is.

          Show
          jcbollinger John Bollinger added a comment - Is it ok if we leave the hiera_xxx functions as they are since they have the style that implies that the user knows the data configuration? Instead users should use the lookup function where the lookup options from data files becomes the default if not given as an argument to lookup. If the lookup() function honors the lookup options specified in the data, then I would be satisfied for hiera() to not do so. I agree that it does not make sense for the behavior of the hiera_xxx() functions to change in any case, but I do still recommend that those functions be deprecated. If lookup() behaves as suggested then the hiera() function should be deprecated, too. Overall, this could be less surprising than altering hiera() to sometimes perform hash-merge or array lookups. If deprecation is too strong to be palatable at this time, then at minimum start strongly promoting use of lookup() and the in-data lookup options as superior practice, which it is.
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          John Bollinger The intent is to deprecate the hiera_xxx functions sometime in the 4.x series. As the functionality for "data in modules"/"lookup" is brand new in puppet 4.3.0 we want it to be use a bit before we start banging on the big drum and doing the deprecations. If all goes well, the idea is to do that in Puppet 4.4.0.

          Show
          henrik.lindberg Henrik Lindberg added a comment - John Bollinger The intent is to deprecate the hiera_xxx functions sometime in the 4.x series. As the functionality for "data in modules"/"lookup" is brand new in puppet 4.3.0 we want it to be use a bit before we start banging on the big drum and doing the deprecations. If all goes well, the idea is to do that in Puppet 4.4.0.
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          Thomas Hallgren Hm (the need to use a module name in the lookupoptions key) - how do you see that working. I imagined we would simply do a lookup of "lookupoptions" with deep merge and get all of them. Does this mean we need to do a lookup per module?

          Show
          henrik.lindberg Henrik Lindberg added a comment - Thomas Hallgren Hm (the need to use a module name in the lookupoptions key) - how do you see that working. I imagined we would simply do a lookup of "lookupoptions" with deep merge and get all of them. Does this mean we need to do a lookup per module?
          Hide
          thomas.hallgren Thomas Hallgren added a comment - - edited

          Henrik Lindberg, forget about my previous comment about module name prefix on the lookupoptions key. I did some refactoring that god rid of the need to do that.

          Regarding deep merging the lookupoptions. Is that really what we want? In my mind a hash merge is a better fit since it would allow keys to be added but not altered or "knocked out".

          Another thing that bugs me slightly is the naming convention. Why do we want lookupoptions and not lookup_options? IMO, the latter is more aligned with names of other options in the puppet/hiera world. Also, shouldn't this name be namespaced? It coexists with user defined data. Would perhaps puppet::lookup_options be a better choice?

          Show
          thomas.hallgren Thomas Hallgren added a comment - - edited Henrik Lindberg , forget about my previous comment about module name prefix on the lookupoptions key. I did some refactoring that god rid of the need to do that. Regarding deep merging the lookupoptions . Is that really what we want? In my mind a hash merge is a better fit since it would allow keys to be added but not altered or "knocked out". Another thing that bugs me slightly is the naming convention. Why do we want lookupoptions and not lookup_options ? IMO, the latter is more aligned with names of other options in the puppet/hiera world. Also, shouldn't this name be namespaced? It coexists with user defined data. Would perhaps puppet::lookup_options be a better choice?
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          Agree that:

          • lookup_options is better than lookupoptions (I keep reading that as "lookupop"-tions ).
          • we want a hash merge (not a deep merge)

          Regarding namespacing lookup_options, It is actually more problematic to use the puppet:: namespace as there are several modules that use that name to manage puppet itself. And since they are meta, they may even end up managing the notion of "lookup_option" :-o..
          So, even if far from ideal - the "topscope" is actually the least problematic.

          Show
          henrik.lindberg Henrik Lindberg added a comment - Agree that: lookup_options is better than lookupoptions (I keep reading that as "lookupop"-tions ). we want a hash merge (not a deep merge) Regarding namespacing lookup_options , It is actually more problematic to use the puppet:: namespace as there are several modules that use that name to manage puppet itself. And since they are meta, they may even end up managing the notion of "lookup_option" :-o.. So, even if far from ideal - the "topscope" is actually the least problematic.
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          Just to be sure, hash merge for lookup_options would allow the entire key to be overridden at a higher precedence level - right? That is important since a designer of a module could specify that a particular key (in that module) should use say a deep merge and a user wants to override that in an environment and only use a given data value.

          Show
          henrik.lindberg Henrik Lindberg added a comment - Just to be sure, hash merge for lookup_options would allow the entire key to be overridden at a higher precedence level - right? That is important since a designer of a module could specify that a particular key (in that module) should use say a deep merge and a user wants to override that in an environment and only use a given data value.
          Hide
          thomas.hallgren Thomas Hallgren added a comment -

          Yes, hash merge means that highest priority wins and since the merge is performed on the containing lookup_options hash, the key will essentially be overridden (the higher prio key is found first and then retained during merge).

          Show
          thomas.hallgren Thomas Hallgren added a comment - Yes, hash merge means that highest priority wins and since the merge is performed on the containing lookup_options hash, the key will essentially be overridden (the higher prio key is found first and then retained during merge).
          Hide
          thomas.hallgren Thomas Hallgren added a comment -

          PR-4378 replaces PR-4358.

          Show
          thomas.hallgren Thomas Hallgren added a comment - PR-4378 replaces PR-4358.
          Hide
          thomas.hallgren Thomas Hallgren added a comment -

          One thing that isn't covered in this ticket is how to integrate the lookup of the lookup_options with the lookup explain capability. The current explanation will show how entries are merged but not the origin of the merge options. Perhaps we need some ways to control such output so that clutter can be avoided when it's not desired.

          Since it doesn't affect the lookup functionality I think it deserves a ticket of it's own.

          Show
          thomas.hallgren Thomas Hallgren added a comment - One thing that isn't covered in this ticket is how to integrate the lookup of the lookup_options with the lookup explain capability. The current explanation will show how entries are merged but not the origin of the merge options. Perhaps we need some ways to control such output so that clutter can be avoided when it's not desired. Since it doesn't affect the lookup functionality I think it deserves a ticket of it's own.
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          Thomas Hallgren Yes, please file a separate ticket for 'lookup explain lookup_options'.

          Show
          henrik.lindberg Henrik Lindberg added a comment - Thomas Hallgren Yes, please file a separate ticket for 'lookup explain lookup_options'.
          Hide
          rnelson0@gmail.com Rob Nelson added a comment -

          Thomas Hallgren Awesome, that looks like quite a lot of changes but the resulting work for the user looks much better. Is the lookup_options key itself looked up using a deep merge, so you can define your parameter key wherever you want? I believe so from the diffs but best to verify.

          Show
          rnelson0@gmail.com Rob Nelson added a comment - Thomas Hallgren Awesome, that looks like quite a lot of changes but the resulting work for the user looks much better. Is the lookup_options key itself looked up using a deep merge, so you can define your parameter key wherever you want? I believe so from the diffs but best to verify.
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          Rob Nelson that is what we decided - it will be a hash lookup (not deep) since we think it would be confusing if the options per key also got merged - that means that the overall options are merged, but it is "priority" between what is stated per key. Does that make sense?

          Show
          henrik.lindberg Henrik Lindberg added a comment - Rob Nelson that is what we decided - it will be a hash lookup (not deep) since we think it would be confusing if the options per key also got merged - that means that the overall options are merged, but it is "priority" between what is stated per key. Does that make sense?
          Hide
          rnelson0@gmail.com Rob Nelson added a comment -

          I could put lookup_options: modulex::paramy in one yaml and lookup_options: modulea::paramb in another yaml. Sounds great to me! If my understanding is correct I can submit a new docs PR for review.

          Show
          rnelson0@gmail.com Rob Nelson added a comment - I could put lookup_options: modulex::paramy in one yaml and lookup_options: modulea::paramb in another yaml. Sounds great to me! If my understanding is correct I can submit a new docs PR for review.
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          Rob Nelson Yes it merges those into one `lookup_options`. It does not merge what is under the modulex::parmay or modulea::paramb keys though.

          It means you can define the default data with merge behavior options in each module's "data in modules" data and they will compose nicely. You can then simply bind what will be merged into those values in the environment (or globally). If you need to override the behavior (and say, not merge, you can override the lookup_options: modulex::paramx).

          Show
          henrik.lindberg Henrik Lindberg added a comment - Rob Nelson Yes it merges those into one `lookup_options`. It does not merge what is under the modulex::parmay or modulea::paramb keys though. It means you can define the default data with merge behavior options in each module's "data in modules" data and they will compose nicely. You can then simply bind what will be merged into those values in the environment (or globally). If you need to override the behavior (and say, not merge, you can override the lookup_options: modulex::paramx).
          Hide
          thomas.hallgren Thomas Hallgren added a comment -

          Added PUP-5437 for the explain functionality.

          Show
          thomas.hallgren Thomas Hallgren added a comment - Added PUP-5437 for the explain functionality.
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          Merged to master at: cfe8d3e

          Show
          henrik.lindberg Henrik Lindberg added a comment - Merged to master at: cfe8d3e
          Hide
          rnelson0@gmail.com Rob Nelson added a comment -
          Show
          rnelson0@gmail.com Rob Nelson added a comment - Documentation PR https://github.com/puppetlabs/puppet-docs/pull/565
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          Thomas Hallgren Can you make sure the documentation for the lookup function gets updated as well.

          Show
          henrik.lindberg Henrik Lindberg added a comment - Thomas Hallgren Can you make sure the documentation for the lookup function gets updated as well.
          Hide
          thomas.hallgren Thomas Hallgren added a comment -

          Found a bug in the already merged implementation causing lookup of lookup_options to always fail when using the module provider. Will provide a separate PR with a fix for that.

          Show
          thomas.hallgren Thomas Hallgren added a comment - Found a bug in the already merged implementation causing lookup of lookup_options to always fail when using the module provider. Will provide a separate PR with a fix for that.
          Hide
          thomas.hallgren Thomas Hallgren added a comment -

          Added PR-4397 to address the problem that I found.

          Show
          thomas.hallgren Thomas Hallgren added a comment - Added PR-4397 to address the problem that I found.
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          PR 4397 merged to master at: af57b50

          Show
          henrik.lindberg Henrik Lindberg added a comment - PR 4397 merged to master at: af57b50
          Hide
          henrik.lindberg Henrik Lindberg added a comment -

          Thomas Hallgren please ping when you have reviewed (or possibly edited) the updated description in this ticket?

          Show
          henrik.lindberg Henrik Lindberg added a comment - Thomas Hallgren please ping when you have reviewed (or possibly edited) the updated description in this ticket?
          Hide
          thomas.hallgren Thomas Hallgren added a comment -

          Henrik Lindberg, description reviewed and edited.

          Show
          thomas.hallgren Thomas Hallgren added a comment - Henrik Lindberg , description reviewed and edited.
          Hide
          erict Eric Thompson added a comment - - edited

          validated on redhat7 at stable SHA: 3af913f51fe786b8a08ad4a2b400bb301245280b
          using the some manifests from the lookup function test in lookup.rb

          [root@ivcig3f3n4xejso production]# cat /etc/puppetlabs/code/environments/production/manifests/site.pp
              node default {
                include data_module
              }
          [root@ivcig3f3n4xejso production]# cat /etc/puppetlabs/code/environments/production/
          environment.conf  hieradata/        lib/              manifests/        modules/
          [root@ivcig3f3n4xejso production]# cat /etc/puppetlabs/code/environments/production/hieradata/common.yaml
          ---
          lookup_options:
            data_module::hash_name:
              merge: deep
          data_module::hash_name:
            module_hash_key: hiera_value
          [root@ivcig3f3n4xejso production]# cat /etc/puppetlabs/code/environments/production/
          environment.conf  hieradata/        lib/              manifests/        modules/
          [root@ivcig3f3n4xejso production]# cat /etc/puppetlabs/code/environments/production/modules/
          data_module/  other_module/
          [root@ivcig3f3n4xejso production]# cat /etc/puppetlabs/code/environments/production/modules/data_module/
          lib/           manifests/     metadata.json
          [root@ivcig3f3n4xejso production]# cat /etc/puppetlabs/code/environments/production/modules/data_module/manifests/init.pp
           
              class data_module($env_data_implied,
                                   $module_data_implied,
                                   $env_data_override_implied,
                                   $automatic_data_key=$automatic_default_value) {
                # should be able to merge hashes across sources
                #   this mimicks/covers behavior for including classes
                $lookup_hash = lookup("data_module::hash_name")
                notify { "hash_name hiera lookup_options $lookup_hash": }
                $lookup_hash2 = lookup("data_module::hash_name",Hash[String,String],'hash')
                notify { "hash_name $lookup_hash2": }
           
              }
          [root@ivcig3f3n4xejso production]# cat /etc/puppetlabs/code/environments/production/modules/data_module/lib/puppet/functions/data_module/data.rb
           
                    Puppet::Functions.create_function(:'data_module::data') do
                      def data()
                        { 'data_module::module_data_implied' => 'module_implied_b',
                          'data_module::module_data' => 'module_b',
                          'data_module::hash_name' => {'module_hash_key' => 'module_class_b'},
                          'data_module::array_key' => ['module_array_a', 'module_array_b']
                        }
                      end
                    end
           
           
           
          [root@ivcig3f3n4xejso production]# puppet agent -t --server $(hostname -f)
          Info: Using configured environment 'production'
          Info: Retrieving pluginfacts
          Info: Retrieving plugin
          Info: Caching catalog for ivcig3f3n4xejso.delivery.puppetlabs.net
          Info: Applying configuration version '1447368944'
          Notice: hash_name hiera lookup_options {module_hash_key => hiera_value, env_hash_key => env_class_a}
          Notice: /Stage[main]/Data_module/Notify[hash_name hiera lookup_options {module_hash_key => hiera_value, env_hash_key => env_class_a}]/message: defined 'message' as 'hash_name hiera lookup_options {module_hash_key => hiera_value, env_hash_key => env_class_a}'
          Notice: hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a}
          Notice: /Stage[main]/Data_module/Notify[hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a}]/message: defined 'message' as 'hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a}'
          Notice: Applied catalog in 0.03 seconds
          

          Show
          erict Eric Thompson added a comment - - edited validated on redhat7 at stable SHA: 3af913f51fe786b8a08ad4a2b400bb301245280b using the some manifests from the lookup function test in lookup.rb [root@ivcig3f3n4xejso production]# cat /etc/puppetlabs/code/environments/production/manifests/site.pp node default { include data_module } [root@ivcig3f3n4xejso production]# cat /etc/puppetlabs/code/environments/production/ environment.conf hieradata/ lib/ manifests/ modules/ [root@ivcig3f3n4xejso production]# cat /etc/puppetlabs/code/environments/production/hieradata/common.yaml --- lookup_options: data_module::hash_name: merge: deep data_module::hash_name: module_hash_key: hiera_value [root@ivcig3f3n4xejso production]# cat /etc/puppetlabs/code/environments/production/ environment.conf hieradata/ lib/ manifests/ modules/ [root@ivcig3f3n4xejso production]# cat /etc/puppetlabs/code/environments/production/modules/ data_module/ other_module/ [root@ivcig3f3n4xejso production]# cat /etc/puppetlabs/code/environments/production/modules/data_module/ lib/ manifests/ metadata.json [root@ivcig3f3n4xejso production]# cat /etc/puppetlabs/code/environments/production/modules/data_module/manifests/init.pp   class data_module($env_data_implied, $module_data_implied, $env_data_override_implied, $automatic_data_key=$automatic_default_value) { # should be able to merge hashes across sources # this mimicks/covers behavior for including classes $lookup_hash = lookup("data_module::hash_name") notify { "hash_name hiera lookup_options $lookup_hash": } $lookup_hash2 = lookup("data_module::hash_name",Hash[String,String],'hash') notify { "hash_name $lookup_hash2": }   } [root@ivcig3f3n4xejso production]# cat /etc/puppetlabs/code/environments/production/modules/data_module/lib/puppet/functions/data_module/data.rb   Puppet::Functions.create_function(:'data_module::data') do def data() { 'data_module::module_data_implied' => 'module_implied_b', 'data_module::module_data' => 'module_b', 'data_module::hash_name' => {'module_hash_key' => 'module_class_b'}, 'data_module::array_key' => ['module_array_a', 'module_array_b'] } end end       [root@ivcig3f3n4xejso production]# puppet agent -t --server $(hostname -f) Info: Using configured environment 'production' Info: Retrieving pluginfacts Info: Retrieving plugin Info: Caching catalog for ivcig3f3n4xejso.delivery.puppetlabs.net Info: Applying configuration version '1447368944' Notice: hash_name hiera lookup_options {module_hash_key => hiera_value, env_hash_key => env_class_a} Notice: /Stage[main]/Data_module/Notify[hash_name hiera lookup_options {module_hash_key => hiera_value, env_hash_key => env_class_a}]/message: defined 'message' as 'hash_name hiera lookup_options {module_hash_key => hiera_value, env_hash_key => env_class_a}' Notice: hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a} Notice: /Stage[main]/Data_module/Notify[hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a}]/message: defined 'message' as 'hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a}' Notice: Applied catalog in 0.03 seconds
          Hide
          erict Eric Thompson added a comment -

          without the lookup_options hash:
          (note the second lookup provides the lookup merge method in the call to lookup())

          [root@ivcig3f3n4xejso production]# puppet agent -t --server $(hostname -f)
          Info: Using configured environment 'production'
          Info: Retrieving pluginfacts
          Info: Retrieving plugin
          Info: Caching catalog for ivcig3f3n4xejso.delivery.puppetlabs.net
          Info: Applying configuration version '1447369188'
          Notice: hash_name hiera lookup_options {module_hash_key => hiera_value}
          Notice: /Stage[main]/Data_module/Notify[hash_name hiera lookup_options {module_hash_key => hiera_value}]/message: defined 'message' as 'hash_name hiera lookup_options {module_hash_key => hiera_value}'
          Notice: hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a}
          Notice: /Stage[main]/Data_module/Notify[hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a}]/message: defined 'message' as 'hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a}'
          Notice: Applied catalog in 0.04 seconds
          

          Show
          erict Eric Thompson added a comment - without the lookup_options hash: (note the second lookup provides the lookup merge method in the call to lookup()) [root@ivcig3f3n4xejso production]# puppet agent -t --server $(hostname -f) Info: Using configured environment 'production' Info: Retrieving pluginfacts Info: Retrieving plugin Info: Caching catalog for ivcig3f3n4xejso.delivery.puppetlabs.net Info: Applying configuration version '1447369188' Notice: hash_name hiera lookup_options {module_hash_key => hiera_value} Notice: /Stage[main]/Data_module/Notify[hash_name hiera lookup_options {module_hash_key => hiera_value}]/message: defined 'message' as 'hash_name hiera lookup_options {module_hash_key => hiera_value}' Notice: hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a} Notice: /Stage[main]/Data_module/Notify[hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a}]/message: defined 'message' as 'hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a}' Notice: Applied catalog in 0.04 seconds
          Hide
          erict Eric Thompson added a comment -

          and with the lookup_options hash in the data binded function:

          Info: Caching catalog for ivcig3f3n4xejso.delivery.puppetlabs.net
          Info: Applying configuration version '1447369759'
          Notice: hash_name hiera lookup_options {module_hash_key => hiera_value, env_hash_key => env_class_a}
          Notice: /Stage[main]/Data_module/Notify[hash_name hiera lookup_options {module_hash_key => hiera_value, env_hash_key => env_class_a}]/message: defined 'message' as 'hash_name hiera lookup_options {module_hash_key => hiera_value, env_hash_key => env_class_a}'
          Notice: hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a}
          Notice: /Stage[main]/Data_module/Notify[hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a}]/message: defined 'message' as 'hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a}'
          Notice: Applied catalog in 0.03 seconds
          [root@ivcig3f3n4xejso production]# cat modules/data_module/lib/puppet/functions/data_module/data.rb
           
                    Puppet::Functions.create_function(:'data_module::data') do
                      def data()
                        { 'lookup_options' => {'data_module::hash_name' => {'merge' => 'deep'}},
                          'data_module::hash_name' => {'module_hash_key' => 'module_class_b'},
                          'data_module::module_data_implied' => 'module_implied_b',
                          'data_module::array_key' => ['module_array_a', 'module_array_b']
                        }
                      end
                    end
          

          Show
          erict Eric Thompson added a comment - and with the lookup_options hash in the data binded function: Info: Caching catalog for ivcig3f3n4xejso.delivery.puppetlabs.net Info: Applying configuration version '1447369759' Notice: hash_name hiera lookup_options {module_hash_key => hiera_value, env_hash_key => env_class_a} Notice: /Stage[main]/Data_module/Notify[hash_name hiera lookup_options {module_hash_key => hiera_value, env_hash_key => env_class_a}]/message: defined 'message' as 'hash_name hiera lookup_options {module_hash_key => hiera_value, env_hash_key => env_class_a}' Notice: hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a} Notice: /Stage[main]/Data_module/Notify[hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a}]/message: defined 'message' as 'hash_name {module_hash_key => hiera_value, env_hash_key => env_class_a}' Notice: Applied catalog in 0.03 seconds [root@ivcig3f3n4xejso production]# cat modules/data_module/lib/puppet/functions/data_module/data.rb   Puppet::Functions.create_function(:'data_module::data') do def data() { 'lookup_options' => {'data_module::hash_name' => {'merge' => 'deep'}}, 'data_module::hash_name' => {'module_hash_key' => 'module_class_b'}, 'data_module::module_data_implied' => 'module_implied_b', 'data_module::array_key' => ['module_array_a', 'module_array_b'] } end end
          Hide
          nick.fagerlund Nicholas Fagerlund added a comment -

          This ticket description is still insufficient.

          • Module data appears to only be accessed for keys of the form <THIS MODULE>::<SOMETHING>. Does lookup_options get an exception to this? It appears to, based on my preliminary testing. What is the nature of this exception? Can a module set hash keys governing lookup keys outside its namespace, or is it limited to those within its namespace? Has this behavior been proven in the testing that's taken place so far? Where?
          • By "hash merge," do you mean the naïve hash merge where the higher-level sources will just knock out the entire value of the "merge" key? Or a deep merge?
          Show
          nick.fagerlund Nicholas Fagerlund added a comment - This ticket description is still insufficient. Module data appears to only be accessed for keys of the form <THIS MODULE>::<SOMETHING>. Does lookup_options get an exception to this? It appears to, based on my preliminary testing. What is the nature of this exception? Can a module set hash keys governing lookup keys outside its namespace, or is it limited to those within its namespace? Has this behavior been proven in the testing that's taken place so far? Where? By "hash merge," do you mean the naïve hash merge where the higher-level sources will just knock out the entire value of the "merge" key? Or a deep merge?
          Hide
          thomas.hallgren Thomas Hallgren added a comment -

          The "hash" is a distinct merge type. A "hash" merge refers to that type (i.e. a naïve merge of hashes). A "deep" merge can be performed on both hashes and arrays so the term "hash" isn't really related to "deep", nor does one imply the other.

          Show
          thomas.hallgren Thomas Hallgren added a comment - The "hash" is a distinct merge type. A "hash" merge refers to that type (i.e. a naïve merge of hashes). A "deep" merge can be performed on both hashes and arrays so the term "hash" isn't really related to "deep", nor does one imply the other.
          Hide
          thomas.hallgren Thomas Hallgren added a comment -

          I updated the description.

          Show
          thomas.hallgren Thomas Hallgren added a comment - I updated the description.

            People

            • Assignee:
              Unassigned
              Reporter:
              redmine.exporter redmine.exporter
              QA Contact:
              Eric Thompson
            • Votes:
              17 Vote for this issue
              Watchers:
              37 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Agile