Bug #12197

Array handling broken in 2.7.10

Added by Stefan Schulte 4 months ago. Updated about 1 month ago.

Status:Closed Start date:01/26/2012
Priority:Normal Due date:
Assignee:Daniel Pittman % Done:

0%
Category:ralsh
Target version:2.7.11
Affected Puppet version:2.7.10 Branch:https://github.com/puppetlabs/puppet/pull/413
Keywords:
Votes: 2

Description

Handling properties with array_matching all is somehow broken in 2.7.10 and ruby 1.8

I have created the following manifest and applied it once, so the resource must be in sync

k5login { '/tmp/test.txt':
  ensure     => present,
  principals => [ 'A', 'B', 'C' ],
}

running puppet apply on 2.7.9

# puppet apply /tmp/test.pp 
notice: Finished catalog run in 0.03 seconds

running puppet apply on 2.7.10

# puppet apply /tmp/test.pp
notice: /Stage[main]//K5login[/tmp/test.txt]/principals: principals changed ['A', 'B', 'C'] to 'A B C'
notice: Finished catalog run in 0.13 seconds

running puppet apply on 2.7.10 and ruby 1.9.3

# puppet apply /tmp/test.pp
/usr/lib64/ruby/site_ruby/1.9.1/rubygems/custom_require.rb:36:in `require': iconv will be deprecated in the future, use String#encode instead.
Could not load confine test 'operatingsystem': cannot load such file -- puppet/provider/confine/operatingsystem
Could not load confine test 'operatingsystem': cannot load such file -- puppet/provider/confine/operatingsystem
Could not load confine test 'operatingsystem': cannot load such file -- puppet/provider/confine/operatingsystem
Could not load confine test 'operatingsystem': cannot load such file -- puppet/provider/confine/operatingsystem
Could not load confine test 'operatingsystem': cannot load such file -- puppet/provider/confine/operatingsystem
Could not load confine test 'operatingsystem': cannot load such file -- puppet/provider/confine/operatingsystem
Could not load confine test 'operatingsystem': cannot load such file -- puppet/provider/confine/operatingsystem
Could not load confine test 'operatingsystem': cannot load such file -- puppet/provider/confine/operatingsystem
Could not load confine test 'operatingsystem': cannot load such file -- puppet/provider/confine/operatingsystem
notice: Finished catalog run in 0.07 seconds

Related issues

related to Puppet - Feature #2927: Symbolic Modes for the File type. Closed 12/14/2009
related to Puppet Labs Modules - Bug #12219: F5_Monitor type : Add insync method for template_destina... Rejected 01/27/2012
duplicated by Puppet - Bug #12223: Puppet 2.7.10 ssh_authorized_key keeps looping when array... Duplicate 01/27/2012

History

#1
Updated by Stefan Schulte 4 months ago

  • Status changed from Unreviewed to Investigating

#2
Updated by Stefan Schulte 4 months ago

If I revert commit 48726b662d2fb5cc3532fca12eb6aa5a3c1622cd it works again.

The commit introduces the following code to test insyc if array_matching is all (reduced to the important stuff)

old = (is == @should or is == @should.collect { |v| v.to_s })
new = Array(is).zip(@should).all? {|is, want| property_matches?(is, want) }
fail "old and new mismatch!" unless old == new

return Array(is).zip(@should).all? {|is, want| property_matches?(is, want) }

So I guess the intention was to be sure that the new way behaves exactly like the old way to determine insync. Unfortunately the the first execution of Array(is).zip(@should).all? {|is, want| property_matches?(is, want) } modifies the is value (blocks dont create a new scope under ruby 1.8 if I am not mistaken.

Short test with irb18

irb(main):001:0> a=['a','b']
=> ["a", "b"]
irb(main):002:0> a.all? { |a| true }
=> true
irb(main):003:0> a
=> "b"

while with irb19

irb(main):001:0> a=['a','b']
=> ["a", "b"]
irb(main):002:0> a.all? { |a| true }
=> true
irb(main):003:0> a
=> ["a", "b"]

#3
Updated by Stefan Schulte 4 months ago

  • Branch set to https://github.com/puppetlabs/puppet/pull/413

I tried fixing the described error but I just found another pitfall:

Array(is).zip(@should).all? {|current, want| property_matches?(current, want) }

This can also return true if is is just a subset of @should because of the way Array.zip works.

irb(main):001:0> is=['a','b']
irb(main):002:0> should=['a','b','c']
irb(main):003:0> is.zip(should)
=> [["a", "a"], ["b", "b"]]
irb(main):004:0> is.zip(should).all? {|x,y| x==y}
=> true

So in the end my pull request just reverts the commit that was causing the error. And if we want to refactor insync? again we should definitly add some specs ;–)

#4
Updated by Stefan Schulte 4 months ago

  • Status changed from Investigating to In Topic Branch Pending Review

#5
Updated by Daniel Pittman 4 months ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release
  • Target version set to 2.7.11

This has been merged – indeed, this broke, and my assessment that other tests (including the acceptance suite) exercised this well enough were obviously unfounded. Thanks for the analysis, and the pull request to fix it. Sorry for breaking your systems. :(

#6
Updated by Matt Robinson 4 months ago

  • Status changed from Merged - Pending Release to Needs More Information

Reverting this commit means that the symbolic file modes #2927 added as part of the same commit series will also have problems. We should either revert that commit also 24f2a65, or fix the issues in the reverted commit very soon.

#7
Updated by Daniel Pittman 4 months ago

  • Status changed from Needs More Information to Code Insufficient

#8
Updated by Daniel Pittman 4 months ago

  • Assignee changed from Stefan Schulte to Daniel Pittman

#9
Updated by Daniel Pittman 4 months ago

  • Status changed from Code Insufficient to In Topic Branch Pending Review

https://github.com/puppetlabs/puppet/pull/420 fixes the problem, and adds appropriate testing to keep it from breaking again. Still pending some internal decisions, but we should have this properly fixed shortly.

#10
Updated by Andreas Ntaflos 3 months ago

Has this been fixed in 2.7.11? The release notes don’t mention this bug.

#11
Updated by Steve Traylen 2 months ago

2.7.11 and this seems to completely fixed.

Many thanks. Steve.

#12
Updated by Daniel Pittman 2 months ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release

#13
Updated by Stefan Schulte about 1 month ago

released in 2.7.11 (commit:2f215460 which seems to have no corresponding feature branch) and 2.7.12 (commit:7485aa6)

I can only toggle between “Code insufficient”, “Tests insufficient”, and “Merged – Pending release” here (which seems odd since I’m the author). Can someone please close this one.

#14
Updated by Justin Stoller about 1 month ago

  • Status changed from Merged - Pending Release to Closed

Also available in: Atom PDF