Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Disallowing 1-primitive groups in cs_group is pointless #152

Open
fghaas opened this issue Jul 14, 2015 · 11 comments
Open

Disallowing 1-primitive groups in cs_group is pointless #152

fghaas opened this issue Jul 14, 2015 · 11 comments
Labels
bug Something isn't working needs-feedback Further information is requested
Milestone
5.x

Comments

@fghaas
Copy link
Contributor

fghaas commented Jul 14, 2015

The validation check in https://github.com/puppet-community/puppet-corosync/blob/master/lib/puppet/type/cs_group.rb#L30 is overzealous. It is a perfectly valid use case to have groups with just 1 primitive. For example, you might want to define a set of 3, 2 or even just 1 resource that you then reference from a constraint. The easiest way to that is to use a group, and to always have the constraint point to the group. If there is only one primitive in the group, so be it.

The alternative is to have manifests full of ifs and unlesses to either point to a group or to a standalone primitive, and that's just silly.
fghaas added a commit to fghaas/puppet-corosync that referenced this issue Jul 14, 2015
@fghaas
Allow 1-element arrays for primitives in cs_group
93315bb 
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.
@fghaas
Copy link
Contributor Author

fghaas commented Jul 14, 2015

For the stab I took at this that GitHub has duly cross-referenced, I of course forgot about Puppet's oh-so-smart single-element array munging as outlined in https://tickets.puppetlabs.com/browse/PUP-1299.

fghaas added a commit to fghaas/puppet-corosync that referenced this issue Jul 14, 2015
@fghaas
Allow 1-element arrays for primitives in cs_group
412767b 
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.
fghaas added a commit to fghaas/puppet-corosync that referenced this issue Jul 14, 2015
@fghaas
Allow 1-element arrays for primitives in cs_group
01eea7d 
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.
fghaas added a commit to fghaas/puppet-iscsi-rbd-ha that referenced this issue Jul 15, 2015
@fghaas
Add dummy resource to group if operating without a VIP
e5ad55b 
As seen in
voxpupuli/puppet-corosync#152,
cs_group enforces a primitives array with a length >1. As a silly
workaround, add a Dummy resource just so that the provider is happy.
fghaas added a commit to fghaas/puppet-iscsi-rbd-ha that referenced this issue Jul 15, 2015
@fghaas
Add dummy resource to group if operating without a VIP
a35f9ba 
As seen in
voxpupuli/puppet-corosync#152,
cs_group enforces a primitives array with a length >1. As a silly
workaround, add a Dummy resource just so that the provider is happy.
fghaas added a commit to fghaas/puppet-corosync that referenced this issue Jul 23, 2015
@fghaas
Allow 1-element arrays for primitives in cs_group
c78f6c4 
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.
@igalic
Copy link
Contributor

igalic commented Oct 14, 2015

@fghaas are you interested in providing your patch as pull request?

@fghaas
Copy link
Contributor Author

fghaas commented Oct 14, 2015

@igalic, do you have a workaround for PUP-1299 that works pre-4.0?

@igalic
Copy link
Contributor

igalic commented Oct 14, 2015

no :(

@jyaworski
Copy link
Member

Is this still an issue?

@jyaworski jyaworski added bug Something isn't working needs-feedback Further information is requested labels Feb 29, 2016
@ffrank
Copy link
Contributor

ffrank commented Sep 14, 2016

I believe so, yes.

@roidelapluie roidelapluie modified the milestone: 5.x Sep 14, 2016
roidelapluie pushed a commit to roidelapluie/puppet-corosync that referenced this issue Sep 15, 2016
@fghaas @roidelapluie
Allow 1-element arrays for primitives in cs_group
209f889 
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.
roidelapluie pushed a commit to roidelapluie/puppet-corosync that referenced this issue Sep 15, 2016
@fghaas @roidelapluie
Allow 1-element arrays for primitives in cs_group
3b43295 
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.
roidelapluie pushed a commit to roidelapluie/puppet-corosync that referenced this issue Sep 15, 2016
@fghaas @roidelapluie
Allow 1-element arrays for primitives in cs_group
bd4268e 
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.
@roidelapluie
Copy link
Member

closed by #368

@ffrank
Copy link
Contributor

ffrank commented Sep 15, 2016

Agreed. 👍

I'll leave this open for a time because I think I can produce a follow-up PR that saves a small amount of leg-work.

@fghaas
Copy link
Contributor Author

fghaas commented Sep 16, 2016

Hmm, this should have auto-closed when #368 was merged. After all there is a comment to that effect in bd4268e.

It looks like fixes is a valid keyword, while fixes issue is not. Sorry about that!

@roidelapluie
Copy link
Member

roidelapluie commented Sep 16, 2016 •
edited
Loading

@fghaas regarding @ffrank's comment we will let this open

@roidelapluie roidelapluie mentioned this issue Sep 22, 2016
Closed
@btravouillon
Copy link
Member

@ffrank, do you still want to produce a follow-up PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working needs-feedback Further information is requested
Projects
None yet
Milestone
5.x
Development

No branches or pull requests
6 participants
@roidelapluie @ffrank @jyaworski @igalic @fghaas @btravouillon