Skip to content

Commit

Permalink
Allow 1-element arrays for primitives in cs_group
Browse files Browse the repository at this point in the history
Having only one primitive in a group is a perfectly valid use
case. For example, one might have constraints related to an arbitrary
number of primitives created with create_resources, and the simplest
way of doing that is to place them all of them in a group, and have
the constraint point to that. Disallowing the possibility of ending up
with just one primitive in the group does not make much sense.

Update the check to allow 1-element arrays. Also, remove a misleading
comment that also looks like it was copied from some other code that
talked about colocations, not groups.

Fixes issue voxpupuli#152.
  • Loading branch information
fghaas committed Jul 14, 2015
1 parent b161978 commit 01eea7d
Showing 1 changed file with 11 additions and 3 deletions.
14 changes: 11 additions & 3 deletions lib/puppet/type/cs_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,21 @@ module Puppet
desc "An array of primitives to have in this group. Must be listed in the
order that you wish them to start."

# We want this to be an array, even if it has only one
# value. Prior to 4.x (and unless using the future parser in
# 3.x), Puppet would munge single-parameter arrays into scalar
# values. See also
# https://tickets.puppetlabs.com/browse/PUP-1299.
munge do |value|
value = [value] unless value.is_a?(Array)
end

# Have to redefine should= here so we can sort the array that is given to
# us by the manifest. While were checking on the class of our value we

This comment has been minimized.

Copy link
@ffrank

ffrank Jul 20, 2015

Wait, what?

  1. There is no sorting going on in this overridden hook.
  2. Even if so, I feel that this should fitfully be the job of the munge hook
  3. This override seems to be there only to raise errors if we don't like the value. That's what validate is for, though.

Something is weird. This was changed in ead27ab but the motivation eludes me. Remember that one, @hunner?

This comment has been minimized.

Copy link
@fghaas

fghaas Jul 20, 2015

Author Owner

On your item 1., how does the issue of sorting apply here?

This comment has been minimized.

Copy link
@ffrank

ffrank Jul 20, 2015

It's just confusing because the comment does not really fit the code.

This comment has been minimized.

Copy link
@fghaas

fghaas Jul 20, 2015

Author Owner

Oh, so you're referring to

       # Have to redefine should= here so we can sort the array that is given to

Yeah, I agree that's weird. But like I said, insufficient Ruby-fu on my end to determine whether this has merit or not.

# are going to go ahead and do some validation too. The way Corosync
# colocation works we need to only accept two value arrays.
# are going to go ahead and do some validation too.
def should=(value)
super
raise Puppet::Error, "Puppet::Type::Cs_Group: primitives property must be at least a 2-element array." unless value.is_a? Array and value.length > 1
raise Puppet::Error, "Puppet::Type::Cs_Group: primitives property must be at least a 1-element array." unless value.is_a? Array and value.length >= 1
@should
end
end
Expand Down

3 comments on commit 01eea7d

@fghaas
Copy link
Owner Author

@fghaas fghaas commented on 01eea7d Jul 15, 2015

Choose a reason for hiding this comment

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

Ok, clearly my Ruby-fu is insufficient here. If someone could explain to this Ruby-newbie why the error message still pops up after inserting the munge bit (which I cargo-culted from https://tickets.puppetlabs.com/browse/PUP-1299), I'd be grateful.

@ffrank
Copy link

@ffrank ffrank commented on 01eea7d Jul 20, 2015

Choose a reason for hiding this comment

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

Barring feedback from @hunner, here are some assumptions.

  1. Overriding should= to raise this exception was done to work around some weird issue.
  2. This is not quite a clean approach though, because a validate hook should do that instead (and used to!)
  3. Since this validation hack evades the usual type instance lifecycle, it may raise before your munge ever has a chance to take effect.

TL;DR, try reverting ead27ab. If this doesn't help, the validation might have to take into account that non-array values are acceptable (since they will get munged eventually).

I keep forgetting which happens first - munge or validate.

@fghaas
Copy link
Owner Author

@fghaas fghaas commented on 01eea7d Jul 20, 2015

Choose a reason for hiding this comment

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

Sorry, suddenly just adding an ocf:heartbeat:Dummy resource to the group looks that much more appealing.

Please sign in to comment.