smbambling / hiera-test

Heira Testing Playground
0 stars 0 forks source link

Incorrect/Inconstant knockout merge behavior #2

Open rnelson0 opened 8 years ago

rnelson0 commented 8 years ago

You have a knockout_prefix of --, but I don't see any keys prepended with '--'. This seems to work fine given that. Can you whip up an example of something you want knocked out, specifically? I'll try playing with it myself but want to make sure I'm envisioning it the way you are.

rnelson0 commented 8 years ago

Based on my 'tweaks' branch, I created two more:

https://github.com/rnelson0/hiera-test/tree/knockout - With sample knockout data that is not working. https://github.com/rnelson0/hiera-test/tree/proof - Same as knockout but :merge_behavior is set to deep, which shows that deep_merge! is being called before erroring out.

rnelson0 commented 8 years ago

Here's the IRB output:

$ bundle exec irb
1.9.1 :001 > require 'deep_merge'
 => true
1.9.1 :002 > source = {:x => ['--1', '2']}
 => {:x=>["--1", "2"]}
1.9.1 :003 > dest   = {:x => ['1', '3']}
 => {:x=>["1", "3"]}
1.9.1 :004 > dest.ko_deep_merge!(source)
 => {:x=>["3", "2"]}

Perhaps it's an ordering thing?

$ bundle exec irb
1.9.1 :001 > require 'deep_merge'
 => true

1.9.1 :002 > source = {:managed_groups => ['group1', 'group2', 'group3']}
 => {:managed_groups=>["group1", "group2", "group3"]}
1.9.1 :003 > dest   = {:managed_groups => ['--group3']}
 => {:managed_groups=>["--group3"]}
1.9.1 :004 > dest.ko_deep_merge!(source)
 => {:managed_groups=>["--group3", "group1", "group2", "group3"]}

1.9.1 :005 > source = {:managed_groups => ['--group3']}
 => {:managed_groups=>["--group3"]}
1.9.1 :006 > dest   = {:managed_groups => ['group1', 'group2', 'group3']}
 => {:managed_groups=>["group1", "group2", "group3"]}
1.9.1 :007 > dest.ko_deep_merge!(source)
 => {:managed_groups=>["group1", "group2"]}
smbambling commented 8 years ago

The really odd behavior is that arrays within a hash have the knockout correctly applied while direct arrays do not.

In the output below "--group3" is added to the array, yet the user1 hash has had group3 removed from its groups => [group1, group2] array

accounts::users:
  user1:
    ensure: present
    comment: 'User One Updated'
    groups:
      - --group3
bundle exec puppet apply --certname=test.example.dev --hiera_config=hiera.yaml test.pp
Notice: Scope(Class[main]): [--group3, group1, group2, group3]
Notice: Scope(Class[main]): Processing --group3
Notice: Scope(Class[main]): {}
Notice: Scope(Class[main]): Processing group1
Notice: Scope(Class[main]): {group1 => {ensure => present, gid => 9999}}
Notice: Scope(Class[main]): Processing group2
Notice: Scope(Class[main]): {group2 => {ensure => present, gid => 9998}}
Notice: Scope(Class[main]): Processing group3
Notice: Scope(Class[main]): {group3 => {ensure => present, gid => 9997}}
Notice: Scope(Class[main]): Processing user1
Notice: Scope(Class[main]): {user1 => {ensure => present, comment => User One Updated, uid => 700, gid => 700, groups => [group1, group2]}}
Notice: Scope(Class[main]): Processing user2
Notice: Scope(Class[main]): {user2 => {ensure => present, comment => User Two Updated, uid => 701, gid => 701}}
Notice: Compiled catalog for test.example.dev in environment production in 0.28 seconds
Notice: Applied catalog in 0.01 seconds
rnelson0 commented 8 years ago

@hel From twitter "Probably (no knock out for arrays)" - clearly that works in deep_merge itself. Sounds like you are saying arrays are not passing to deep_merge with the knockout_prefix, or maybe not at all? /cc @thallgren

smbambling commented 8 years ago

@hlindberg (hel) is someone else :) @ffrank

hlindberg commented 8 years ago

We need to wait for @thallgren to get clarity - he is the expert in this area (and he is out at the moment).

thallgren commented 8 years ago

I need some context. What version of puppet/hiera are you using? Do you use 'deep' or 'deeper'? Can you provide the hiera config?

smbambling commented 8 years ago

Puppet 4.2.1, Hiera 3.0.1 -- The hiera config and datadir are included in this repo

smbambling commented 8 years ago

Doing some more searching in combination with the ordering that @rnelson0 found. I think this issue might apply or I could be totally wrong :)

https://tickets.puppetlabs.com/browse/HI-223 https://github.com/pkill/deep_merge/commit/4967c0f49016690c208146d581f6f58d5a7254dc

thallgren commented 8 years ago

That sounds very plausible and it begs the question, is the current behavior all wrong? It feels a bit scary if less specific entries were allowed to knock out more specific entries (and the possibly re-add them).

smbambling commented 8 years ago

I think @ffrank might be correct (via twitter) that this only applies to hashes and not arrays with the deep_merge.

If I move managed_groups to a hash with a key,val that is an array it works

common.yaml

---
managed_groups:
  groups:
   - group1
   - group2
   - group3
   - group4

nodes/test.example.dev.yaml

---
managed_groups:
  groups:
    - --group3

Results

bundle exec puppet apply --certname=test.example.dev --hiera_config=hiera.yaml test.pp
Notice: Scope(Class[main]): [group1, group2, group4]
Notice: Scope(Class[main]): Processing group1
Notice: Scope(Class[main]): {group1 => {ensure => present, gid => 9999}}
Notice: Scope(Class[main]): Processing group2
Notice: Scope(Class[main]): {group2 => {ensure => present, gid => 9998}}
Notice: Scope(Class[main]): Processing group4
Notice: Scope(Class[main]): {}
Notice: Compiled catalog for test.example.dev in environment production in 0.29 seconds
Notice: Applied catalog in 0.01 seconds
smbambling commented 8 years ago

Pushed up a branch with the 'fixes' for testing https://github.com/smbambling/hiera-test/tree/managed_groups_as_hash

smbambling commented 8 years ago

@thallgren: Do you know if the deep merge knockout only applies to Hash and not Array ?