Boolean properties and misc #346
Conversation
puppetlabs_spec_helper-0.10.2 not compatible with puppet 4.0
Not passing the arguments to the super constructors has nasty side effects, including the default values specified for the parameters being ignored.
The :boolean option is ignored for newproperty. It only has an effect with newparam, where is creates a method called <parameter name>? in the type class.
| @@ -30,7 +30,7 @@ end | |||
| if puppetversion = ENV['PUPPET_GEM_VERSION'] | |||
| gem 'puppet', puppetversion, :require => false | |||
| else | |||
| gem 'puppet', :require => false | |||
| gem 'puppet', '< 4.0.0', :require => false | |||
There was a problem hiding this comment.
puppetlabs_spec_helper-0.10.2 seems to be incompatible with puppet 4.0. I get this error:
/usr/bin/ruby1.9.1 -S rspec spec/classes/sensu_api_spec.rb spec/classes/sensu_client_spec.rb spec/classes/sensu_init_spec.rb spec/classes/sensu_package_spec.rb spec/classes/sensu_rabbitmq_spec.rb spec/classes/sensu_redis_spec.rb spec/classes/sensu_server_spec.rb spec/defines/sensu_check_spec.rb spec/defines/sensu_extension_spec.rb spec/defines/sensu_filter_spec.rb spec/defines/sensu_handler_spec.rb spec/defines/sensu_plugin_spec.rb spec/defines/sensu_subscription_spec.rb spec/unit/puppet_x_sensu_totype_spec.rb spec/unit/sensu_config_spec.rb --color
/home/glopes/.gems/gems/puppetlabs_spec_helper-0.10.2/lib/puppetlabs_spec_helper/module_spec_helper.rb:23:in `block in <top (required)>': undefined method `environmentpath=' for #<RSpec::Core::Configuration:0x000000012d2ae8> (NoMethodError)
from /var/lib/gems/1.9.1/gems/rspec-core-2.99.2/lib/rspec/core.rb:154:in `configure'
from /home/glopes/.gems/gems/puppetlabs_spec_helper-0.10.2/lib/puppetlabs_spec_helper/module_spec_helper.rb:22:in `<top (required)>'
from /home/glopes/repos/sensu-puppet/spec/spec_helper.rb:2:in `require'
from /home/glopes/repos/sensu-puppet/spec/spec_helper.rb:2:in `<top (required)>'
from /home/glopes/repos/sensu-puppet/spec/classes/sensu_api_spec.rb:1:in `require'
from /home/glopes/repos/sensu-puppet/spec/classes/sensu_api_spec.rb:1:in `<top (required)>'
from /var/lib/gems/1.9.1/gems/rspec-core-2.99.2/lib/rspec/core/configuration.rb:1065:in `load'
from /var/lib/gems/1.9.1/gems/rspec-core-2.99.2/lib/rspec/core/configuration.rb:1065:in `block in load_spec_files'
from /var/lib/gems/1.9.1/gems/rspec-core-2.99.2/lib/rspec/core/configuration.rb:1065:in `each'
from /var/lib/gems/1.9.1/gems/rspec-core-2.99.2/lib/rspec/core/configuration.rb:1065:in `load_spec_files'
from /var/lib/gems/1.9.1/gems/rspec-core-2.99.2/lib/rspec/core/command_line.rb:18:in `run'
from /var/lib/gems/1.9.1/gems/rspec-core-2.99.2/lib/rspec/core/runner.rb:103:in `run'
from /var/lib/gems/1.9.1/gems/rspec-core-2.99.2/lib/rspec/core/runner.rb:17:in `block in autorun'
There was a problem hiding this comment.
Actually, the odd thing is that that line is c.environmentpath = spec_path if Puppet.version.to_f >= 4.0, so it's there specifically for puppet 4.
cataphract
force-pushed
the
boolean_and_misc
branch
from
db0268d to
6307f65
Compare
|
I've been re-evaluating what versions of puppet I support with my modules and was thinking about this module as well. The release of 4.0 makes me think we should drop all 2.7 support, and I believe dropping everything pre-3.3 still keeps one version of PE older than current support. Not to muddy the waters on this, but I'm wondering if it makes sense to just use |
|
I guess I am not adverse to that - we'll go a major release to go with it? |
|
That would probably make sense if we remove older supported versions. If there's other things we want to change with that we should probably have that discussion outside this PR. |
|
Yeah I don't have anything else - unless you do - there's probably a few types/providers folks would like to replace but I don't have any bandwidth. |
|
So would you prefer a pull request using |
|
Yes please! |
cataphract
force-pushed
the
boolean_and_misc
branch
from
6307f65 to
d73412f
Compare
|
Done. I also added another commit for using the full GPG fingerprint for the apt repository, as this generates a warning in the latest puppetlabs-apt (I can submit a separate PR if you prefer). |
cataphract
force-pushed
the
boolean_and_misc
branch
from
d73412f to
e83db17
Compare
As to the default, my choice was to only give defaults where create on the provider did not should whether the should value was null.
Missing caller the port= setter on sensu_client_config's provider's create method. When the resource is first created (more precisely, when the ensure property is out of sync) the only synched property is ensure, which delegates to create on the provider. Because writing to the json object of the provider that serves as the data for the json object is only done in the provider setters, we must call them explicitly from create.
This avoids a warning with recent versions of puppetlabs-apt, which warn you that using a shortened fingerprint is insecure.
|
Thanks so much for this - excellent refactor! |
|
Unfortunately, there is a problem. Once a property is true, you cannot change it back to false. This is because on Arguable this should test |
Fix problem introduced in #346 and simplification of create
As mentioned in 05999a6#commitcomment-10864529 , implement the boolean properties through a superclass.
Unfortunately,
Puppet::Parameter::Booleanwas only introduced in 3.3, so we must implement our own superclass. In the process, fixed some other issues.