From 8ee12203f3e31d6b616d7ed2d205fa85cb540c15 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Wed, 12 Jun 2019 14:53:55 +0100 Subject: [PATCH 1/5] (maint) Fixup Gemfile for JRuby 1.7 installs --- .travis.yml | 2 +- Gemfile | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/.travis.yml b/.travis.yml index 3c7a2284..391e4425 100644 --- a/.travis.yml +++ b/.travis.yml @@ -20,7 +20,7 @@ matrix: cache: bundler: true directories: ~/.rvm - before_install: rvm use jruby-1.7.26 --install --binary --fuzzy + before_install: rvm use jruby-1.7.26 --install --binary --fuzzy && gem install bundler -v '~> 1.7.0' # disable coverage on jruby9k as this confuses codecov - env: RVM="jruby-9.1.9.0" PUPPET_GEM_VERSION='~> 5' JRUBY_OPTS="--debug" before_cache: pushd ~/.rvm && rm -rf archives rubies/ruby-2.2.7 rubies/ruby-2.3.4 && popd diff --git a/Gemfile b/Gemfile index 922fb4aa..4fac43ca 100644 --- a/Gemfile +++ b/Gemfile @@ -7,16 +7,14 @@ gemspec group :tests do gem 'codecov' - # license_finder does not install on windows using older versions of rubygems. - # ruby 2.4 is confirmed working on appveyor. - gem 'license_finder' if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.4.0') gem 'rake', '~> 10.0' gem 'rspec', '~> 3.0' # rubocop 0.58 throws when testing against ruby 2.1, so pin to the latest, # unless we're dealing with jruby... if RUBY_PLATFORM == 'java' - # load any rubocop version that works on java for the Rakefile - gem 'rubocop' + # load a rubocop version that works on java for the Rakefile + gem 'parser', '2.3.3.1' + gem 'rubocop', '0.41.2' elsif Gem::Version.new(RUBY_VERSION) < Gem::Version.new('2.2.0') gem 'rubocop', '0.57.2' # the last version of parallel to support ruby 2.1 @@ -27,6 +25,9 @@ group :tests do # This needs to be removed once we drop puppet4 support. gem 'rubocop', '~> 0.57.0' gem 'rubocop-rspec' + # license_finder does not install on windows using older versions of rubygems. + # ruby 2.4 is confirmed working on appveyor. + gem 'license_finder' if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.4.0') end gem 'simplecov-console' # the test gems required for module testing From c6d1b3a97c2099edd28799a36c4de2eab6883a64 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Wed, 12 Jun 2019 14:57:23 +0100 Subject: [PATCH 2/5] (FM-7839) Implement 'to_json' method for ResourceShim for bolt bolt's `get_resources` uses the `libexec/query_resources.rb` to retrieve resources as json from the target nodes. Especially https://github.com/puppetlabs/bolt/blob/7dec6e49d66c7f9ae06088d46ee3330de0ed2fed/libexec/query_resources.rb#L71 causes `ResourceShims` to fail as they did not implement `to_json`. This integration test models what `libexec/query_resources.rb` is doing. The added `to_json(*)` method now passes this test, and - shown in manual testing - works for bolt. --- lib/puppet/resource_api/glue.rb | 7 +++++++ spec/integration/resource_api_spec.rb | 4 ++++ spec/puppet/resource_api/glue_spec.rb | 17 +++++++++++++++++ 3 files changed, 28 insertions(+) diff --git a/lib/puppet/resource_api/glue.rb b/lib/puppet/resource_api/glue.rb index 8eee448d..30773a08 100644 --- a/lib/puppet/resource_api/glue.rb +++ b/lib/puppet/resource_api/glue.rb @@ -45,6 +45,13 @@ def to_hash values end + def to_json(*) + attrs = filtered_keys.map { |k| [k.to_s, values[k]] unless values[k].nil? } + attributes = Hash[*attrs.compact.flatten] + resource = { title => attributes } + resource.to_json + end + # attribute names that are not title or namevars def filtered_keys values.keys.reject { |k| k == :title || !attr_def[k] || (attr_def[k][:behaviour] == :namevar && @namevars.size == 1) } diff --git a/spec/integration/resource_api_spec.rb b/spec/integration/resource_api_spec.rb index c4ff82ba..b8d1cdf8 100644 --- a/spec/integration/resource_api_spec.rb +++ b/spec/integration/resource_api_spec.rb @@ -223,6 +223,10 @@ def get(_context) describe 'its title' do it { expect(instance.to_resource.title).to eq 'somename' } end + + it 'can serialize to json' do + expect({ 'resources' => [instance.to_resource] }.to_json).to eq '{"resources":[{"somename":{"ensure":"absent","boolean_param":false,"integer_param":99,"float_param":3.21,"ensure_param":"present","variant_pattern_param":"1234ABCD","url_param":"http://www.puppet.com","string_array_param":"d","e":"f","string_param":"default value"}}]}' # rubocop:disable Metrics/LineLength + end end it('ensure is reported as a symbol') { expect(instance[:ensure]).to be_a Symbol } diff --git a/spec/puppet/resource_api/glue_spec.rb b/spec/puppet/resource_api/glue_spec.rb index c31b8c00..b8b656ea 100644 --- a/spec/puppet/resource_api/glue_spec.rb +++ b/spec/puppet/resource_api/glue_spec.rb @@ -47,6 +47,23 @@ end end + describe '.to_json' do + it { expect(instance.to_json).to eq '{"title":{"attr":"value","attr_ro":"fixed"}}' } + + context 'with nil values' do + subject(:instance) do + described_class.new({ namevarname: title, attr: nil, attr_ro: 'fixed' }, 'typename', [:namevarname], + namevarname: { type: 'String', behaviour: :namevar, desc: 'the title' }, + attr: { type: 'String', desc: 'a string parameter' }, + attr_ro: { type: 'String', desc: 'a string readonly', behaviour: :read_only }) + end + + it 'doesn\'t output them' do + expect(instance.to_json).to eq '{"title":{"attr_ro":"fixed"}}' + end + end + end + describe '.to_hierayaml' do it { expect(instance.to_hierayaml).to eq " title:\n attr: value\n attr_ro: fixed\n" } From 89c8808b46dddce0fdcdf3b6e806d2780d633233 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 30 May 2019 11:05:37 +0100 Subject: [PATCH 3/5] (maint) implement `desc`/`docs` fallback This implements the changes specified in https://github.com/puppetlabs/puppet-specifications/pull/141 > The text and examples have been inconsistent with how `desc` vs `docs` > has been handled. This change fixes the text and examples to all show > and require `desc`, but accept `docs` in places where we showed it > previously. --- lib/puppet/resource_api.rb | 12 +++++- spec/puppet/resource_api/base_context_spec.rb | 2 +- .../resource_api/puppet_context_spec.rb | 2 +- spec/puppet/resource_api_spec.rb | 37 ++++++++++++++++++- 4 files changed, 48 insertions(+), 5 deletions(-) diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb index 0f9ba788..f36b3604 100644 --- a/lib/puppet/resource_api.rb +++ b/lib/puppet/resource_api.rb @@ -37,6 +37,16 @@ def register_type(definition) unknown_features = definition[:features] - supported_features Puppet.warning("Unknown feature detected: #{unknown_features.inspect}") unless unknown_features.empty? + # fixup desc/docs backwards compatibility + if definition.key? :docs + if definition[:desc] + raise Puppet::DevError, '`%{name}` has both `desc` and `docs`, prefer using `desc`' % { name: definition[:name] } + end + definition[:desc] = definition[:docs] + definition.delete(:docs) + end + Puppet.warning('`%{name}` has no documentation, add it using a `desc` key' % { name: definition[:name] }) unless definition.key? :desc + # fixup any weird behavior ;-) definition[:attributes].each do |name, attr| next unless attr[:behavior] @@ -56,7 +66,7 @@ def register_type(definition) end Puppet::Type.newtype(definition[:name].to_sym) do - @docs = definition[:docs] + @docs = definition[:desc] # Keeps a copy of the provider around. Weird naming to avoid clashes with puppet's own `provider` member define_singleton_method(:my_provider) do diff --git a/spec/puppet/resource_api/base_context_spec.rb b/spec/puppet/resource_api/base_context_spec.rb index b7f4f724..3d42d10a 100644 --- a/spec/puppet/resource_api/base_context_spec.rb +++ b/spec/puppet/resource_api/base_context_spec.rb @@ -13,7 +13,7 @@ def send_log(log, msg) TestContext.new(definition) end - let(:definition) { { name: 'some_resource', attributes: { name: 'some_resource' }, features: feature_support } } + let(:definition) { { name: 'some_resource', desc: 'a test resource', attributes: { name: { type: 'String', desc: 'message' } }, features: feature_support } } let(:feature_support) { [] } it { expect { described_class.new(nil) }.to raise_error ArgumentError, %r{BaseContext requires definition to be a Hash} } diff --git a/spec/puppet/resource_api/puppet_context_spec.rb b/spec/puppet/resource_api/puppet_context_spec.rb index f64b0119..7b614733 100644 --- a/spec/puppet/resource_api/puppet_context_spec.rb +++ b/spec/puppet/resource_api/puppet_context_spec.rb @@ -3,7 +3,7 @@ RSpec.describe Puppet::ResourceApi::PuppetContext do subject(:context) { described_class.new(definition) } - let(:definition) { { name: 'some_resource' } } + let(:definition) { { name: 'some_resource', desc: 'a test resource', attributes: {} } } describe '#device' do context 'when a NetworkDevice is configured' do diff --git a/spec/puppet/resource_api_spec.rb b/spec/puppet/resource_api_spec.rb index f383e72d..29446642 100644 --- a/spec/puppet/resource_api_spec.rb +++ b/spec/puppet/resource_api_spec.rb @@ -47,7 +47,10 @@ context 'when registering a minimal type' do let(:definition) { { name: 'minimal', attributes: {} } } - it { expect { described_class.register_type(definition) }.not_to raise_error } + it { + expect(Puppet).to receive(:warning).with('`minimal` has no documentation, add it using a `desc` key') + described_class.register_type(definition) + } describe 'the registered type' do subject(:type) { Puppet::Type.type(:minimal) } @@ -62,10 +65,33 @@ end end + context 'when registering a type with both desc and docs key' do + let(:definition) { { name: 'both', desc: 'the desc', docs: 'the docs', attributes: {} } } + + it { + expect { described_class.register_type(definition) }.to raise_error Puppet::DevError, '`both` has both `desc` and `docs`, prefer using `desc`' + } + end + + context 'when registering a type with a docs key' do + let(:definition) { { name: 'both', docs: 'the docs', attributes: {} } } + + it { expect { described_class.register_type(definition) }.not_to raise_error } + + describe 'the registered type' do + subject(:type) { Puppet::Type.type(:both) } + + it { is_expected.not_to be_nil } + it { is_expected.to be_respond_to :instances } + it { expect(type.instance_variable_get(:@docs)).to eq 'the docs' } + end + end + context 'when registering a type with multiple attributes' do let(:definition) do { name: type_name, + desc: 'a test resource', attributes: { name: { type: 'String', @@ -769,6 +795,7 @@ def set(_context, _changes); end let(:definition) do { name: 'init_behaviour', + desc: 'a test resource', attributes: { ensure: { type: 'Enum[present, absent]', @@ -1253,7 +1280,7 @@ def set(_context, _changes); end context 'when loading a provider that doesn\'t create the correct class' do let(:definition) { { name: 'no_class', attributes: {} } } - it { expect { described_class.load_provider('no_class') }.to raise_error Puppet::DevError, %r{Puppet::Provider::NoClass::NoClass} } + it { expect { described_class.load_provider('no_class') }.to raise_error Puppet::DevError, %r{provider class Puppet::Provider::NoClass::NoClass not found} } end context 'when loading a provider that creates the correct class' do @@ -1810,6 +1837,7 @@ def set(_context, changes) end let(:definition) do { name: 'test_noop_support', + desc: 'a test resource', features: ['no such feature'], attributes: {}, } @@ -1826,6 +1854,7 @@ def set(_context, changes) end let(:definition) do { name: 'test_behaviour', + desc: 'a test resource', attributes: { id: { type: 'String', @@ -1842,6 +1871,7 @@ def set(_context, changes) end let(:definition) do { name: 'test_behaviour', + desc: 'a test resource', attributes: { param: { type: 'String', @@ -1858,6 +1888,7 @@ def set(_context, changes) end let(:definition) do { name: 'test_behaviour', + desc: 'a test resource', attributes: { param_ro: { type: 'String', @@ -1874,6 +1905,7 @@ def set(_context, changes) end let(:definition) do { name: 'test_behaviour', + desc: 'a test resource', attributes: { param_ro: { type: 'String', @@ -1890,6 +1922,7 @@ def set(_context, changes) end let(:definition) do { name: 'test_behaviour', + desc: 'a test resource', attributes: { source: { type: 'String', From c80380853691ac951b753aca7f2534ca8315a6bc Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Tue, 21 May 2019 14:27:49 +0100 Subject: [PATCH 4/5] (maint) reset provider cache between tests Without this the `provider` mock would leak between tests. --- spec/puppet/resource_api_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/puppet/resource_api_spec.rb b/spec/puppet/resource_api_spec.rb index 5939cc1a..4d78cb84 100644 --- a/spec/puppet/resource_api_spec.rb +++ b/spec/puppet/resource_api_spec.rb @@ -1794,6 +1794,11 @@ def set(_context, changes) end allow(provider_class).to receive(:new).and_return(provider) end + after(:each) do + # reset cached provider between tests + type.instance_variable_set(:@my_provider, nil) + end + it { expect { described_class.register_type(definition) }.not_to raise_error } it 'is seen as a supported feature' do From b85f0cb003eeab6ef1ee19d811ee3c35104ebabc Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 13 Jun 2019 15:33:47 +0100 Subject: [PATCH 5/5] (maint) have the #inject_device test clean up after itself Before the fix: ``` david@davids:~/git/puppet-resource_api$ SPEC_OPTS="--exclude-pattern spec/\{fixtures/\*\*/\*.rb,fixtures/modules/\*/\*\*/\*.rb\}" bundle exec rspec ./spec/puppet/resource_api/transport_spec.rb[1:4:2:1] ./spec/puppet/resource_api_spec.rb[1:16:2:1] --seed 40589 -fd Run options: include {:ids=>{"./spec/puppet/resource_api/transport_spec.rb"=>["1:4:2:1"], "./spec/puppet/resource_api_spec.rb"=>["1:16:2:1"]}} Randomized with seed 40589 Puppet::ResourceApi::Transport #inject_device(name, transport) when puppet does not have set_device wraps the transport and sets it as current in NetworkDevice Puppet::ResourceApi #load_provider when loading a provider that doesn't create the correct class should raise Puppet::DevError with message matching /provider class Puppet::Provider::NoClass::NoClass not found/ (FAILED - 1) Failures: 1) Puppet::ResourceApi#load_provider when loading a provider that doesn't create the correct class should raise Puppet::DevError with message matching /provider class Puppet::Provider::NoClass::NoClass not found/ Failure/Error: it { expect { described_class.load_provider('no_class') }.to raise_error Puppet::DevError, %r{provider class Puppet::Provider::NoClass::NoClass not found} } expected Puppet::DevError with message matching /provider class Puppet::Provider::NoClass::NoClass not found/, got # with backtrace: # ./lib/puppet/resource_api.rb:437:in `rescue in load_provider' # ./lib/puppet/resource_api.rb:416:in `load_provider' # ./spec/puppet/resource_api_spec.rb:1202:in `block (5 levels) in ' # ./spec/puppet/resource_api_spec.rb:1202:in `block (4 levels) in ' # ./spec/puppet/resource_api_spec.rb:1202:in `block (4 levels) in ' Finished in 0.05427 seconds (files took 0.94482 seconds to load) 2 examples, 1 failure Failed examples: rspec ./spec/puppet/resource_api_spec.rb:1202 # Puppet::ResourceApi#load_provider when loading a provider that doesn't create the correct class should raise Puppet::DevError with message matching /provider class Puppet::Provider::NoClass::NoClass not found/ Randomized with seed 40589 david@davids:~/git/puppet-resource_api$ ``` After the fix: ``` david@davids:~/git/puppet-resource_api$ SPEC_OPTS="--exclude-pattern spec/\{fixtures/\*\*/\*.rb,fixtures/modules/\*/\*\*/\*.rb\}" bundle exec rspec ./spec/puppet/resource_api/transport_spec.rb[1:4:2:1] ./spec/puppet/resource_api_spec.rb[1:16:2:1] --seed 40589 -fd Run options: include {:ids=>{"./spec/puppet/resource_api/transport_spec.rb"=>["1:4:2:1"], "./spec/puppet/resource_api_spec.rb"=>["1:16:2:1"]}} Randomized with seed 40589 Puppet::ResourceApi::Transport #inject_device(name, transport) when puppet does not have set_device wraps the transport and sets it as current in NetworkDevice Puppet::ResourceApi #load_provider when loading a provider that doesn't create the correct class should raise Puppet::DevError with message matching /provider class Puppet::Provider::NoClass::NoClass not found/ Finished in 0.03734 seconds (files took 0.95637 seconds to load) 2 examples, 0 failures Randomized with seed 40589 david@davids:~/git/puppet-resource_api$ ``` --- spec/puppet/resource_api/transport_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index ccb02997..db1eaa3f 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -260,6 +260,10 @@ class Wibble; end end end + after(:each) do + Puppet::Util::NetworkDevice.instance_variable_set(:@current, nil) + end + context 'when puppet has set_device' do it 'wraps the transport and calls set_device within NetworkDevice' do expect(Puppet::ResourceApi::Transport::Wrapper).to receive(:new).with(device_name, transport).and_return(wrapper)