Allow multiple rings in nodelist #291

rhamon · 2016-06-21T21:09:54Z

Please check the following items before submitting a PR -- thank you!

Note that this project is released with a Contributor Code of Conduct.
By participating in this project you agree to abide by its terms.
Contributor Code of Conduct.

There is no existing PR that addresses this problem
Mentioned any existing issues in your commit so they get linked and
closed once this PR gets merged, i.e: Closes #1554 in the body of a commit
Followed the instructions in the Contributing document
Ran the unit/spec tests and ensured they still pass
Added tests to cover the new behaviour
Updated the documentation to match the changes
When possible, add an entry to the CHANGELOG file
Squashed your PR down to a single commit. You may forego this if the PR
tries to address multiple issues. Though we prefer one PR per feature/fix,
sometimes that's not feasible. In that case ensure that a single feature/fix
and associated tests and documentation is bundled up in one commit

Optional, but extra points:

Added tests to ensure the old behaviour cannot accidentally be
reintroduced

roidelapluie · 2016-06-21T21:22:42Z

manifests/init.pp

+  or ( is_array($unicast_addresses) and is_array($unicast_addresses[0]) and size($unicast_addresses[0]) > 1 )
+  or ( is_array($quorum_members) and is_array($quorum_members[0]) and size($quorum_members[0]) > 1 )
+  and $rrp_mode == 'none' {
+    fail('You must set rrp_mode to active or passive with multiple rings')


Do you really think that the validation belongs to the Puppet module?

IMO this is correct. the logic is a bit complex but we should keep it.

roidelapluie · 2016-06-21T21:23:17Z

Hello, Thanks for your pull requests.

Could you please add tests to prevent future regressions?

roidelapluie · 2016-06-21T22:17:52Z

Why are you closing this?

rhamon · 2016-06-23T18:06:07Z

Spent 30mins trying to get spec going and started feeling frustrated, so
moved on to other happy thoughts.
On Jun 21, 2016 6:17 PM, "Julien Pivotto" notifications@github.com wrote:

Why are you closing this?

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
#291 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AGq6a66zgPvEcjUPFIhaA3z5QDRm77h2ks5qOGMTgaJpZM4I7LN2
.

roidelapluie · 2016-06-23T18:07:45Z

ok

bastelfreak · 2016-06-23T18:26:32Z

Hi @rhamon, thanks for your work. Please don't delete your branch yet - I would like to pick it up and add missing tests/fix them. This PR should be sufficient to implement #262

rhamon · 2016-06-23T19:25:29Z

templates/corosync.conf.udpu.erb

@@ -96,7 +96,9 @@ quorum {
 nodelist {
 <% [@quorum_members].flatten.each_index do |i| -%>


This is broken, will commit <% Array(@quorum_members).each_index do |i| -%>

rhamon · 2016-06-23T20:22:37Z

The failures from "travis-ci" are about not finding a proper version of puppet-lint... what am I supposed to do about it? Can I still tick the box "Ran the unit/spec tests and ensured they still pass"?

bastelfreak · 2016-06-23T20:23:46Z

yeah we broke that yesterday evening - sorry about that :( I will try to fix that tomorrow

roidelapluie · 2016-06-24T13:38:53Z

manifests/init.pp

+  or ( is_array($unicast_addresses) and is_array($unicast_addresses[0]) and size($unicast_addresses[0]) > 1 )
+  or ( is_array($quorum_members) and is_array($quorum_members[0]) and size($quorum_members[0]) > 1 ) )
+  and $rrp_mode == 'none' {
+    fail('You must set rrp_mode to active or passive with multiple rings')


Please drop this, I think it is best to use:

file { 'corosync.conf': validate_cmd => 'corosync -t' }

Would using validate_cmd from puppetlabs-stdlib be okay?
validate_cmd(template($corosync_conf), 'corosync -t', 'Corosync failed to validate /etc/corosync/corosync.conf')

no, we should use standard file { validate_cmd
}

The approcach in #294 is better than this ?

Much better, yes.
Validate_cmd is only in puppet 3.5+, would be nice to fallback to stdlib for previous versions.

Then we will wait for the 4.0 version of this module before releasing #294. At that time we will remove support for Puppet < 3.6. I do not think that it is useful to get a workaround until then.

#294 is merged, you can remove that block :)

I still need this as our company is not ready to upgrade puppet to 3.6+...

master is no longer compatible with Puppet < 3.6.

roidelapluie · 2016-08-19T10:30:30Z

@rhamon #294 is merged. Can you please rebase and fix conflicts?

roidelapluie · 2016-08-25T14:55:20Z

needs tests

Closes voxpupuli#262

rhamon · 2016-08-25T21:38:15Z

rebased

rhamon · 2016-08-25T21:43:27Z

I don't like the [...].flatten everywhere. I believe using Array() is the way to ensure you get an Array where you expect one. http://docs.ruby-lang.org/en/2.0.0/Kernel.html#method-i-Array

roidelapluie · 2016-08-25T21:50:38Z

You still miss tests. With tests you do not believe, you know :)

rhamon · 2016-08-25T21:55:21Z

unfortunately not going to happen anytime soon... the desire and/or will to learn spec is not on my todo list... maybe just close this PR?
I'm shoving this under the "It works for me" carpet.

rhamon · 2016-08-25T21:58:40Z

Regarding [].flatten, it returned unexpected results for me with array of arrays where Array() did correct thing... would be nice to prove the world using spec tests but see previous comment.

roidelapluie · 2016-08-25T21:59:22Z

You can PR this to the oldpuppet branch, this branch should work with old puppet versions, then we can release a v3.0.1 and backport the changes to 4.0.1

roidelapluie · 2016-08-25T22:00:07Z

Let's keep this PR open and I'll see if I find some time to work on that.

rhamon · 2016-08-25T22:02:58Z

This is free and unencumbered software released into the public domain.

Anyone is free to copy, modify, publish, use, compile, sell, or
distribute this software, either in source code form or as a compiled
binary, for any purpose, commercial or non-commercial, and by any
means.

In jurisdictions that recognize copyright laws, the author or authors
of this software dedicate any and all copyright interest in the
software to the public domain. We make this dedication for the benefit
of the public at large and to the detriment of our heirs and
successors. We intend this dedication to be an overt act of
relinquishment in perpetuity of all present and future rights to this
software under copyright law.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
IN NO EVENT SHALL THE AUTHORS BE LIABLE FOR ANY CLAIM, DAMAGES OR
OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
OTHER DEALINGS IN THE SOFTWARE.

For more information, please refer to http://unlicense.org

rhamon · 2016-08-25T22:08:21Z

Merci RoiDeLapluie 👍

Closes voxpupuli#291 Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>

rhamon force-pushed the nodelist_rings branch from 34b1c6f to 5168884 Compare June 21, 2016 21:17

roidelapluie reviewed Jun 21, 2016
View reviewed changes

rhamon closed this Jun 21, 2016

roidelapluie reopened this Jun 21, 2016

roidelapluie closed this Jun 23, 2016

bastelfreak reopened this Jun 23, 2016

bastelfreak self-assigned this Jun 23, 2016

rhamon reviewed Jun 23, 2016
View reviewed changes

rhamon force-pushed the nodelist_rings branch from 64628a2 to fc9f330 Compare June 23, 2016 20:05

roidelapluie reviewed Jun 24, 2016
View reviewed changes

roidelapluie added the x.y.z: Y version bump label Jun 28, 2016

roidelapluie unassigned bastelfreak Aug 19, 2016

roidelapluie added needs-feedback Further information is requested needs-work not ready to merge just yet labels Aug 19, 2016

roidelapluie modified the milestones: 6.x, 5.x Aug 25, 2016

roidelapluie changed the title ~~Allow multiple rings in nodelist and validate rrp_mode~~ Allow multiple rings in nodelist Aug 25, 2016

Allow multiple rings in nodelist and validate rrp_mode

ed65595

Closes voxpupuli#262

rhamon force-pushed the nodelist_rings branch from fc9f330 to ed65595 Compare August 25, 2016 21:37

roidelapluie added a commit to roidelapluie/puppet-corosync that referenced this pull request Aug 31, 2016

Add spec test for voxpupuli#291

bde58df

Closes voxpupuli#291 Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>

roidelapluie added a commit to roidelapluie/puppet-corosync that referenced this pull request Aug 31, 2016

Add spec test for voxpupuli#291

6f7f3c3

Closes voxpupuli#291 Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>

roidelapluie added a commit to roidelapluie/puppet-corosync that referenced this pull request Aug 31, 2016

Add spec test for voxpupuli#291

94b26a3

Closes voxpupuli#291 Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>

bastelfreak closed this in 4275bc2 Aug 31, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow multiple rings in nodelist #291

Allow multiple rings in nodelist #291

rhamon commented Jun 21, 2016 •

edited

Loading

roidelapluie Jun 21, 2016

bastelfreak Jun 23, 2016

roidelapluie commented Jun 21, 2016

roidelapluie commented Jun 21, 2016

rhamon commented Jun 23, 2016

roidelapluie commented Jun 23, 2016

bastelfreak commented Jun 23, 2016 •

edited

Loading

rhamon Jun 23, 2016

rhamon commented Jun 23, 2016

bastelfreak commented Jun 23, 2016

roidelapluie Jun 24, 2016 •

edited

Loading

rhamon Jun 26, 2016

roidelapluie Jun 27, 2016

roidelapluie Jun 27, 2016

rhamon Jun 27, 2016

roidelapluie Jun 27, 2016 •

edited

Loading

roidelapluie Aug 19, 2016 •

edited

Loading

rhamon Aug 25, 2016

roidelapluie Aug 25, 2016

roidelapluie commented Aug 19, 2016

roidelapluie commented Aug 25, 2016

rhamon commented Aug 25, 2016

rhamon commented Aug 25, 2016

roidelapluie commented Aug 25, 2016

rhamon commented Aug 25, 2016

rhamon commented Aug 25, 2016

roidelapluie commented Aug 25, 2016

roidelapluie commented Aug 25, 2016

rhamon commented Aug 25, 2016

rhamon commented Aug 25, 2016

		@@ -96,7 +96,9 @@ quorum {
		nodelist {
		<% [@quorum_members].flatten.each_index do \|i\| -%>

Allow multiple rings in nodelist #291

Allow multiple rings in nodelist #291

Conversation

rhamon commented Jun 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roidelapluie commented Jun 21, 2016

roidelapluie commented Jun 21, 2016

rhamon commented Jun 23, 2016

roidelapluie commented Jun 23, 2016

bastelfreak commented Jun 23, 2016 • edited Loading

Choose a reason for hiding this comment

rhamon commented Jun 23, 2016

bastelfreak commented Jun 23, 2016

roidelapluie Jun 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roidelapluie Jun 27, 2016 • edited Loading

Choose a reason for hiding this comment

roidelapluie Aug 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roidelapluie commented Aug 19, 2016

roidelapluie commented Aug 25, 2016

rhamon commented Aug 25, 2016

rhamon commented Aug 25, 2016

roidelapluie commented Aug 25, 2016

rhamon commented Aug 25, 2016

rhamon commented Aug 25, 2016

roidelapluie commented Aug 25, 2016

roidelapluie commented Aug 25, 2016

rhamon commented Aug 25, 2016

rhamon commented Aug 25, 2016

rhamon commented Jun 21, 2016 •

edited

Loading

bastelfreak commented Jun 23, 2016 •

edited

Loading

roidelapluie Jun 24, 2016 •

edited

Loading

roidelapluie Jun 27, 2016 •

edited

Loading

roidelapluie Aug 19, 2016 •

edited

Loading