search

redhat-openstack / puppet-pacemaker

Puppet modules to manage pacemaker with corosync
9 stars 25 forks source link

Initial pcs property support #71

Closed mbaldessari closed 8 years ago

mbaldessari commented 8 years ago

Add a function in order to support pcs properties natively. Support is per cluster-wide properties or per-node properties. Example:

pacemaker::property {"global-bar": property => "bar", value => "baz", force => true, tries => 1, try_sleep => 1, }

jguiditta commented 8 years ago

I have to say that I am on the fence with this. I like the idea to add this support, but on the other hand, I think this module should probably be trying to move away from execs, and instead use more puppet provider code. That said, if nobody else strenuously objects, perhaps this could be a good interim step, and the impl under pacemaker::property could later change to call a provider. Any thoughts on this @EmilienM (aside from 'this needs to get under jenkins and have more/better tests', which I agree with)?

EmilienM commented 8 years ago

Of course, unit tests are more than welcome.

mbaldessari commented 8 years ago

Thanks for the feedback @EmilienM, much appreciated. I addressed all your comments (not sure why it is not showing as a new commit, but the existing one is definitely updated). I did test all scenarios before (with --force and without) but I agree with the risks of passing "" to pcs, so I amended that as well.

Regarding adding this to CI and unit tests, I am happy to help but might need a kick or two in the right direction.

EmilienM commented 8 years ago

@mbaldessari a last minor comment and code looks good to me. As a note for later, we might want to setup some acceptance tests (beaker) and unit tests for this modules. Merging code without any tests really makes me sick.

mbaldessari commented 8 years ago

Thanks @EmilienM . I have amended the commit and fixed a couple of other puppet-lint complaints.

Fully agreed on the unit tests. I will tackle them in a separate pull request. If you have a puppet module that you consider a good example regarding unit testing, I am all ears. Otherwise I'll just read up on the topic and wade my way through ;)

EmilienM commented 8 years ago

I'm blindly merging this patch that won't hopefully not break anything. Thanks for your contribution and your patience, very appreciated here.