From bc54be78eb76bf453873be93f0ee565286bdbd5b Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Tue, 22 Jan 2019 16:14:51 +0000 Subject: [PATCH 01/16] (FM-7691) start refactoring definition handling in contexts --- lib/puppet/resource_api/base_context.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/puppet/resource_api/base_context.rb b/lib/puppet/resource_api/base_context.rb index bf877680..220ee8f6 100644 --- a/lib/puppet/resource_api/base_context.rb +++ b/lib/puppet/resource_api/base_context.rb @@ -7,7 +7,6 @@ class Puppet::ResourceApi::BaseContext def initialize(definition) raise ArgumentError, 'BaseContext requires definition to be a Hash' unless definition.is_a?(Hash) - @typename = definition[:name] @type = Puppet::ResourceApi::TypeDefinition.new(definition) end @@ -27,7 +26,7 @@ def feature_support?(feature) [:debug, :info, :notice, :warning, :err].each do |level| define_method(level) do |*args| if args.length == 1 - message = "#{@context || @typename}: #{args.last}" + message = "#{@context || @type.name}: #{args.last}" elsif args.length == 2 resources = format_titles(args.first) message = "#{resources}: #{args.last}" @@ -137,9 +136,9 @@ def send_log(_level, _message) def format_titles(titles) if titles.length.zero? && !titles.is_a?(String) - @typename + @type.name else - "#{@typename}[#{[titles].flatten.compact.join(', ')}]" + "#{@type.name}[#{[titles].flatten.compact.join(', ')}]" end end From 5ecffccc3382a790cc4bdbbe000f22ee4b2b4796 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Tue, 22 Jan 2019 16:47:12 +0000 Subject: [PATCH 02/16] (FM-7691) Allow pre-fabbed BaseTypeDefinition as argument to BaseContext --- lib/puppet/resource_api/base_context.rb | 10 ++++++++-- spec/puppet/resource_api/base_context_spec.rb | 11 +++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/lib/puppet/resource_api/base_context.rb b/lib/puppet/resource_api/base_context.rb index 220ee8f6..fa995648 100644 --- a/lib/puppet/resource_api/base_context.rb +++ b/lib/puppet/resource_api/base_context.rb @@ -6,8 +6,14 @@ class Puppet::ResourceApi::BaseContext attr_reader :type def initialize(definition) - raise ArgumentError, 'BaseContext requires definition to be a Hash' unless definition.is_a?(Hash) - @type = Puppet::ResourceApi::TypeDefinition.new(definition) + if definition.is_a?(Hash) + # this is only for backwards compatibility + @type = Puppet::ResourceApi::TypeDefinition.new(definition) + elsif definition.is_a? Puppet::ResourceApi::BaseTypeDefinition + @type = definition + else + raise ArgumentError, 'BaseContext requires definition to be a child of Puppet::ResourceApi::BaseTypeDefinition, not <%{actual_type}>' % { actual_type: definition.class } + end end def device diff --git a/spec/puppet/resource_api/base_context_spec.rb b/spec/puppet/resource_api/base_context_spec.rb index 34869f75..1c85129d 100644 --- a/spec/puppet/resource_api/base_context_spec.rb +++ b/spec/puppet/resource_api/base_context_spec.rb @@ -13,10 +13,17 @@ def send_log(log, msg) TestContext.new(definition) end - let(:definition) { { name: 'some_resource', attributes: { name: { type: 'String', desc: 'message' } }, features: feature_support } } + let(:definition_hash) { { name: 'some_resource', attributes: { name: { type: 'String', desc: 'message' } }, features: feature_support } } + let(:definition) { Puppet::ResourceApi::TypeDefinition.new(definition_hash) } let(:feature_support) { [] } - it { expect { described_class.new(nil) }.to raise_error ArgumentError, %r{BaseContext requires definition to be a Hash} } + it { expect { described_class.new(nil) }.to raise_error ArgumentError, %r{BaseContext requires definition to be a child of Puppet::ResourceApi::BaseTypeDefinition} } + describe 'legacy hash definition support' do + let(:definition) { definition_hash } + + it { expect { context }.not_to raise_error } + it { expect(context.type.name).to eq 'some_resource' } + end describe '#failed?' do it('defaults to false') { is_expected.not_to be_failed } From 9d549c05a109b121e123211e8e71fe411da70e16 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Tue, 22 Jan 2019 17:16:23 +0000 Subject: [PATCH 03/16] (FM-7691) use the new capability in resource_api.rb --- lib/puppet/resource_api.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb index 1d69ac0a..16623f6d 100644 --- a/lib/puppet/resource_api.rb +++ b/lib/puppet/resource_api.rb @@ -353,7 +353,7 @@ def cache_current_state(resource_hash) end define_singleton_method(:context) do - @context ||= PuppetContext.new(definition) + @context ||= PuppetContext.new(TypeDefinition.new(definition)) end def context From 76c6eeb6b936ca413d81ab57ee8471b9566b746a Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Wed, 23 Jan 2019 17:34:29 +0000 Subject: [PATCH 04/16] (FM-7691) Load the schema before trying to validate connection_info --- lib/puppet/resource_api/transport.rb | 1 + spec/puppet/resource_api/transport_spec.rb | 17 ++++++++++------- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index 5858cd91..4586ed14 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -33,6 +33,7 @@ def connect(name, connection_info) def self.validate(name, connection_info) @transports ||= {} + require "puppet/transport/schema/#{name}" unless @transports.key? name transport_schema = @transports[name] raise Puppet::DevError, 'Transport for `%{target}` not registered' % { target: name } if transport_schema.nil? diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index 3208900d..bc6fe0c2 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -129,19 +129,22 @@ describe '#self.validate' do context 'when the transport being validated has not be registered' do - it { expect { described_class.validate('wibble', {}) }.to raise_error Puppet::DevError, %r{Transport for `wibble` not registered} } + it { expect { described_class.validate('wibble', {}) }.to raise_error LoadError, %r{(no such file to load|cannot load such file) -- puppet/transport/schema/wibble} } end context 'when the transport being validated has been registered' do let(:schema) { { name: 'validate', desc: 'a minimal connection', connection_info: {} } } + let(:schema_def) { instance_double('Puppet::ResourceApi::TransportSchemaDef', 'schema_def') } + + it 'validates the connection_info' do + allow(Puppet::ResourceApi::TransportSchemaDef).to receive(:new).with(schema).and_return(schema_def) - it 'continues to validate the connection_info' do - # rubocop:disable RSpec/AnyInstance - expect_any_instance_of(Puppet::ResourceApi::TransportSchemaDef).to receive(:check_schema).with({}) - expect_any_instance_of(Puppet::ResourceApi::TransportSchemaDef).to receive(:validate).with({}) - # rubocop:enable RSpec/AnyInstance described_class.register(schema) - described_class.validate('validate', {}) + + expect(schema_def).to receive(:check_schema).with('connection_info').and_return(nil) + expect(schema_def).to receive(:validate).with('connection_info').and_return(nil) + + described_class.validate('validate', 'connection_info') end end end From b6cbdcf57f9be727543300c9ade3afa9815eb505 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Wed, 23 Jan 2019 17:38:02 +0000 Subject: [PATCH 05/16] (FM-7691) Fix tests to not rely on previously registered transports --- spec/puppet/resource_api/transport_spec.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index bc6fe0c2..d79f9016 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -11,6 +11,11 @@ Puppet.debug = true end + after(:each) do + # reset registered transports between tests to reduce cross-test poisoning + described_class.instance_variable_set(:@transports, nil) + end + context 'when registering a schema with missing keys' do it { expect { described_class.register([]) }.to raise_error(Puppet::DevError, %r{requires a hash as schema}) } it { expect { described_class.register({}) }.to raise_error(Puppet::DevError, %r{requires a `:name`}) } @@ -25,7 +30,10 @@ it { expect { described_class.register(schema) }.not_to raise_error } context 'when re-registering a transport' do - it { expect { described_class.register(schema) }.to raise_error(Puppet::DevError, %r{`minimal` is already registered}) } + it { + described_class.register(schema) + expect { described_class.register(schema) }.to raise_error(Puppet::DevError, %r{`minimal` is already registered}) + } end end From c7f320374a3da2b20b3335f853c7c05c55e41008 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 10:50:30 +0000 Subject: [PATCH 06/16] (FM-7696) remove overzealous exception handling --- lib/puppet/resource_api/transport.rb | 10 +++------- spec/puppet/resource_api/transport_spec.rb | 9 ++++----- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index 4586ed14..e1225d1b 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -21,13 +21,9 @@ def register(schema) def connect(name, connection_info) validate(name, connection_info) - begin - require "puppet/transport/#{name}" - class_name = name.split('_').map { |e| e.capitalize }.join - Puppet::Transport.const_get(class_name).new(connection_info) - rescue LoadError, NameError => detail - raise Puppet::DevError, 'Cannot load transport for `%{target}`: %{detail}' % { target: name, detail: detail } - end + require "puppet/transport/#{name}" + class_name = name.split('_').map { |e| e.capitalize }.join + Puppet::Transport.const_get(class_name).new(connection_info) end module_function :connect # rubocop:disable Style/AccessModifierDeclarations diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index d79f9016..ba1c8e82 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -102,19 +102,18 @@ end context 'when the transport file does not exist' do - it 'throws a DevError' do + it 'throws a LoadError' do expect(described_class).to receive(:validate).with(name, connection_info) - expect { described_class.connect(name, connection_info) }.to raise_error Puppet::DevError, - %r{Cannot load transport for `test_target`} + expect { described_class.connect(name, connection_info) }.to raise_error LoadError, %r{(no such file to load|cannot load such file) -- puppet/transport/test_target} end end context 'when the transport file does exist' do context 'with an incorrectly defined transport' do - it 'throws a DevError' do + it 'throws a NameError' do expect(described_class).to receive(:validate).with(name, connection_info) expect(described_class).to receive(:require).with('puppet/transport/test_target') - expect { described_class.connect(name, connection_info) }.to raise_error Puppet::DevError, + expect { described_class.connect(name, connection_info) }.to raise_error NameError, %r{uninitialized constant (Puppet::Transport|TestTarget)} end end From c46f50bb3a4b2e8de39aebac52c83cbfdd58fe5f Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 11:03:31 +0000 Subject: [PATCH 07/16] (FM-7691) fix connection_info/schema misnomer in tests --- spec/puppet/resource_api/transport_spec.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index ba1c8e82..0bd2fafa 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -88,7 +88,7 @@ context 'when connecting to a transport' do let(:name) { 'test_target' } - let(:connection_info) do + let(:schema) do { name: 'test_target', desc: 'a basic transport', @@ -103,18 +103,18 @@ context 'when the transport file does not exist' do it 'throws a LoadError' do - expect(described_class).to receive(:validate).with(name, connection_info) - expect { described_class.connect(name, connection_info) }.to raise_error LoadError, %r{(no such file to load|cannot load such file) -- puppet/transport/test_target} + expect(described_class).to receive(:validate).with(name, host: 'example.com') + expect { described_class.connect(name, host: 'example.com') }.to raise_error LoadError, %r{(no such file to load|cannot load such file) -- puppet/transport/test_target} end end context 'when the transport file does exist' do context 'with an incorrectly defined transport' do it 'throws a NameError' do - expect(described_class).to receive(:validate).with(name, connection_info) + expect(described_class).to receive(:validate).with(name, host: 'example.com') expect(described_class).to receive(:require).with('puppet/transport/test_target') - expect { described_class.connect(name, connection_info) }.to raise_error NameError, - %r{uninitialized constant (Puppet::Transport|TestTarget)} + expect { described_class.connect(name, host: 'example.com') }.to raise_error NameError, + %r{uninitialized constant (Puppet::Transport|TestTarget)} end end @@ -123,12 +123,12 @@ it 'loads initiates the class successfully' do expect(described_class).to receive(:require).with('puppet/transport/test_target') - expect(described_class).to receive(:validate).with(name, connection_info) + expect(described_class).to receive(:validate).with(name, host: 'example.com') stub_const('Puppet::Transport::TestTarget', test_target) - expect(test_target).to receive(:new).with(connection_info) + expect(test_target).to receive(:new).with(host: 'example.com') - described_class.connect(name, connection_info) + described_class.connect(name, host: 'example.com') end end end From 9535fe36c58afdef0d7428a60339a85fceeb09e6 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 11:08:52 +0000 Subject: [PATCH 08/16] (FM-7691) create and pass through context when connecting a transport --- lib/puppet/resource_api/transport.rb | 9 +++------ spec/puppet/resource_api/transport_spec.rb | 8 +++++++- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index e1225d1b..d31038a4 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -1,8 +1,4 @@ -# rubocop:disable Style/Documentation -module Puppet; end -module Puppet::ResourceApi; end -module Puppet::ResourceApi::Transport; end -# rubocop:enable Style/Documentation +require 'puppet/resource_api/puppet_context' # Remote target transport API module Puppet::ResourceApi::Transport @@ -23,7 +19,8 @@ def connect(name, connection_info) validate(name, connection_info) require "puppet/transport/#{name}" class_name = name.split('_').map { |e| e.capitalize }.join - Puppet::Transport.const_get(class_name).new(connection_info) + context = Puppet::ResourceApi::PuppetContext.new(@transports[name]) + Puppet::Transport.const_get(class_name).new(context, connection_info) end module_function :connect # rubocop:disable Style/AccessModifierDeclarations diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index 0bd2fafa..ed7d3953 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -111,6 +111,8 @@ context 'when the transport file does exist' do context 'with an incorrectly defined transport' do it 'throws a NameError' do + described_class.register(schema) + expect(described_class).to receive(:validate).with(name, host: 'example.com') expect(described_class).to receive(:require).with('puppet/transport/test_target') expect { described_class.connect(name, host: 'example.com') }.to raise_error NameError, @@ -120,13 +122,17 @@ context 'with a correctly defined transport' do let(:test_target) { double('Puppet::Transport::TestTarget') } # rubocop:disable RSpec/VerifiedDoubles + let(:context) { instance_double(Puppet::ResourceApi::PuppetContext, 'context') } it 'loads initiates the class successfully' do + described_class.register(schema) + expect(described_class).to receive(:require).with('puppet/transport/test_target') expect(described_class).to receive(:validate).with(name, host: 'example.com') + expect(Puppet::ResourceApi::PuppetContext).to receive(:new).with(kind_of(Puppet::ResourceApi::TransportSchemaDef)).and_return(context) stub_const('Puppet::Transport::TestTarget', test_target) - expect(test_target).to receive(:new).with(host: 'example.com') + expect(test_target).to receive(:new).with(context, host: 'example.com') described_class.connect(name, host: 'example.com') end From f8282d215cf6e8dfda940b2d9ca2d820405264ab Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 11:27:12 +0000 Subject: [PATCH 09/16] (FM-7691) clean up wrapper to not expose or remember config Neither the Device API, nor the wrapper need access to the config, so we can immediately load the config, pass it on and never store it ourselves here. --- lib/puppet/resource_api/transport/wrapper.rb | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/lib/puppet/resource_api/transport/wrapper.rb b/lib/puppet/resource_api/transport/wrapper.rb index 97fe4fe0..df4f7ba3 100644 --- a/lib/puppet/resource_api/transport/wrapper.rb +++ b/lib/puppet/resource_api/transport/wrapper.rb @@ -8,21 +8,17 @@ class Puppet::ResourceApi::Transport::Wrapper def initialize(name, url_or_config) if url_or_config.is_a? String - @url = URI.parse(url_or_config) - raise "Unexpected url '#{url_or_config}' found. Only file:/// URLs for configuration supported at the moment." unless @url.scheme == 'file' + url = URI.parse(url_or_config) + raise "Unexpected url '#{url_or_config}' found. Only file:/// URLs for configuration supported at the moment." unless url.scheme == 'file' + raise "Trying to load config from '#{url.path}, but file does not exist." if url && !File.exist?(url.path) + config = (Hocon.load(url.path, syntax: Hocon::ConfigSyntax::HOCON) || {}).map { |k, v| [k.to_sym, v] }.to_h else - @config = url_or_config + config = url_or_config end @transport = Puppet::ResourceApi::Transport.connect(name, config) end - def config - raise "Trying to load config from '#{@url.path}, but file does not exist." if @url && !File.exist?(@url.path) - # symbolize top-level keys to match up with expected provider/resource data conventions - @config ||= (Hocon.load(@url.path, syntax: Hocon::ConfigSyntax::HOCON) || {}).map { |k, v| [k.to_sym, v] }.to_h - end - def facts # @transport.facts + custom_facts # look into custom facts work by TP @transport.facts From f8fcf793dc06cc50d12c2e86905c4f019e59c065 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 13:15:23 +0000 Subject: [PATCH 10/16] (FM-7691) hide loading puppet_context from jruby the JRuby 1.7 only implements ruby language 1.9 which chokes on the logging named arguments. Hiding the require as shown here will avoid the file being loaded for the server-side tests. --- lib/puppet/resource_api/transport.rb | 10 ++++++---- spec/puppet/resource_api/transport_spec.rb | 1 + 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index d31038a4..88aa44e5 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -1,5 +1,3 @@ -require 'puppet/resource_api/puppet_context' - # Remote target transport API module Puppet::ResourceApi::Transport def register(schema) @@ -19,8 +17,7 @@ def connect(name, connection_info) validate(name, connection_info) require "puppet/transport/#{name}" class_name = name.split('_').map { |e| e.capitalize }.join - context = Puppet::ResourceApi::PuppetContext.new(@transports[name]) - Puppet::Transport.const_get(class_name).new(context, connection_info) + Puppet::Transport.const_get(class_name).new(get_context(name), connection_info) end module_function :connect # rubocop:disable Style/AccessModifierDeclarations @@ -33,4 +30,9 @@ def self.validate(name, connection_info) transport_schema.check_schema(connection_info) transport_schema.validate(connection_info) end + + def self.get_context(name) + require 'puppet/resource_api/puppet_context' + Puppet::ResourceApi::PuppetContext.new(@transports[name]) + end end diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index ed7d3953..aca163ed 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -127,6 +127,7 @@ it 'loads initiates the class successfully' do described_class.register(schema) + allow(described_class).to receive(:require).with('puppet/resource_api/puppet_context').and_call_original expect(described_class).to receive(:require).with('puppet/transport/test_target') expect(described_class).to receive(:validate).with(name, host: 'example.com') expect(Puppet::ResourceApi::PuppetContext).to receive(:new).with(kind_of(Puppet::ResourceApi::TransportSchemaDef)).and_return(context) From fc4f585c4fef64d186bd578f17a995534753044a Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 13:25:56 +0000 Subject: [PATCH 11/16] (maint) update notifications to talk to slack --- .travis.yml | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/.travis.yml b/.travis.yml index b2619621..978c4106 100644 --- a/.travis.yml +++ b/.travis.yml @@ -65,10 +65,5 @@ matrix: - rvm: 2.5.1 env: PUPPET_GEM_VERSION='https://github.com/puppetlabs/puppet.git#6.0.x' notifications: - hipchat: - rooms: - secure: 10a49kkZcghKHNnef8x7eBG+KjScL3i1VpygFg6DPAOK2YNbEoyEx1Kv9KLC7GSRYov/SQZOsZrvHZtDhEtFSKhhiAjOwxl1jV1t6aAMGMnN1IoZBOvdAJKrZsm54/bBeYp+je2wqnnoFNtLVFSoOX0LkFaDEWT+zGZ5xKJIH25GpeQEZf1eDxs/d8YX/m+RwbGXHVA//hOpvZo0ntvznh2EbW5OPODKSeUXbWZ+W4ndODTsKWFc/WLMSSgFDzW/Y2/9V40D4IC8lvSx6eKFryMfAQy6pO/d1aTB468awzyVcdYAMMCOITm7hlKGRKxNgq6NkOsXs5KLg6ifpn+a/Rhapbz6Qxbpjjho/7Wxngl4B3T+i35ap/mFrS/fOfKCq3gEQlYn29its9bEFArNGbr+/sXKABb+sRpgW4RTPWYDHJyHJendbevd5tZ+fd0JUBOi0Cb4PcXxQxM8IQrbuu2zso0K5MV05kL0S1DE/VsuUrPaK0RsF+b1+i6NfvtN8kgbYs1eiVku+guIG2ec3xIefQ1hsEOFPFNqSDfHp7nANnRVIbBCt8qw8DhmNEczsfN5Dp21euJUsO9qpau++NzD3jRhkE5Zki5cwsakU7hIQzw82BIb0eSQJCHygieExeEboWRqtDgy/IKIWPgIvEuU68ppR2bl2reKCHLCnWc= - template: - - '%{repository}#%{build_number} (%{branch} - %{commit} : %{author}): %{message} - (Details | PR/Diff)' - format: html + slack: + secure: aPXZYNow8LsmmlS8PQU3FjL0bc7FqUUA95d++wZfIu7YAjGboIUiekxYouQ0XnY+Aig8InvbTOIgBHgGNheyr/YFbFS90/jtulbF8oW7BitW+imgjeAHDCwlQZTCc4FFYde/2pI7QTT8H5NpLR9mKxlTU77Sqr8gFAIybuPdHcKMYQZdEZS07ma2pUp7+GyKS6PDQpzW2+mDCz/wfi3/JdsUvc0mclCZ8vxySc66j5P1E6nFDMzuakBOjwJHpgeDpreapbmSUQLAX0a3ZsFP+N+SNduLotlV2BWnJK2gcO6rGFP4Fz1D0bGXuBnYYdIiB+9OgI3wtXg9y1SifNHUG3IrOBAA8CGNyrebTGKtH0TS2O+HZLbaNX2g6udD5e3156vys9wScmJuQ/rSkVtQfXf1qUm5eijvlXI+DIbssbZHqm6QQGyM4p3NoULmNmF1C85bQoZ4GF7b1P/8mstsVE/HUfnzRPNbwD0r6j1aE/ck3PKMi7ZAhIi0Ja9RnAgP3wi0t62uERYcJGGYEycWohMWnrf2w6GFwGeuoiwAkASdHOLX0/AOMPc4mBOjlc621o8uYMrrZqfF5CrOAvJ151USSsWn2AhXaibIvnHo6X91paNvvNpU/GYu3CUAl6q8OhYovvjtRVPVnhs2DrpgoRB+6NWHnzjRG/wr6Z9U+vA= From 5d89caf5c1f1e1fbca9e80366116b7bc88d04fbf Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 13:31:12 +0000 Subject: [PATCH 12/16] (FM-7691) organise tests, and mark agent tests as such Loading a PuppetContext is not necessary on (the JRuby 1.7) puppet server, so stop trying to run the tests there. --- spec/puppet/resource_api/transport_spec.rb | 118 +++++++++++---------- 1 file changed, 60 insertions(+), 58 deletions(-) diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index aca163ed..7ad393ef 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -16,77 +16,79 @@ described_class.instance_variable_set(:@transports, nil) end - context 'when registering a schema with missing keys' do - it { expect { described_class.register([]) }.to raise_error(Puppet::DevError, %r{requires a hash as schema}) } - it { expect { described_class.register({}) }.to raise_error(Puppet::DevError, %r{requires a `:name`}) } - it { expect { described_class.register(name: 'no connection info', desc: 'some description') }.to raise_error(Puppet::DevError, %r{requires `:connection_info`}) } - it { expect { described_class.register(name: 'no description') }.to raise_error(Puppet::DevError, %r{requires `:desc`}) } - it { expect { described_class.register(name: 'no hash attributes', desc: 'some description', connection_info: []) }.to raise_error(Puppet::DevError, %r{`:connection_info` must be a hash, not}) } - end + describe '#register(schema)' do + context 'when registering a schema with missing keys' do + it { expect { described_class.register([]) }.to raise_error(Puppet::DevError, %r{requires a hash as schema}) } + it { expect { described_class.register({}) }.to raise_error(Puppet::DevError, %r{requires a `:name`}) } + it { expect { described_class.register(name: 'no connection info', desc: 'some description') }.to raise_error(Puppet::DevError, %r{requires `:connection_info`}) } + it { expect { described_class.register(name: 'no description') }.to raise_error(Puppet::DevError, %r{requires `:desc`}) } + it { expect { described_class.register(name: 'no hash attributes', desc: 'some description', connection_info: []) }.to raise_error(Puppet::DevError, %r{`:connection_info` must be a hash, not}) } + end - context 'when registering a minimal transport' do - let(:schema) { { name: 'minimal', desc: 'a minimal connection', connection_info: {} } } + context 'when registering a minimal transport' do + let(:schema) { { name: 'minimal', desc: 'a minimal connection', connection_info: {} } } - it { expect { described_class.register(schema) }.not_to raise_error } + it { expect { described_class.register(schema) }.not_to raise_error } - context 'when re-registering a transport' do - it { - described_class.register(schema) - expect { described_class.register(schema) }.to raise_error(Puppet::DevError, %r{`minimal` is already registered}) - } + context 'when re-registering a transport' do + it { + described_class.register(schema) + expect { described_class.register(schema) }.to raise_error(Puppet::DevError, %r{`minimal` is already registered}) + } + end end - end - context 'when registering a transport' do - let(:schema) do - { - name: 'a_remote_thing', - desc: 'basic transport', - connection_info: { - host: { - type: 'String', - desc: 'the host ip address or hostname', - }, - user: { - type: 'String', - desc: 'the user to connect as', - }, - password: { - type: 'String', - sensitive: true, - desc: 'the password to make the connection', + context 'when registering a transport' do + let(:schema) do + { + name: 'a_remote_thing', + desc: 'basic transport', + connection_info: { + host: { + type: 'String', + desc: 'the host ip address or hostname', + }, + user: { + type: 'String', + desc: 'the user to connect as', + }, + password: { + type: 'String', + sensitive: true, + desc: 'the password to make the connection', + }, }, - }, - } - end + } + end - it 'adds to the transports register' do - expect { described_class.register(schema) }.not_to raise_error + it 'adds to the transports register' do + expect { described_class.register(schema) }.not_to raise_error + end end - end - context 'when registering a transport with a bad type' do - let(:schema) do - { - name: 'a_bad_thing', - desc: 'basic transport', - connection_info: { - host: { - type: 'garbage', - desc: 'the host ip address or hostname', + context 'when registering a transport with a bad type' do + let(:schema) do + { + name: 'a_bad_thing', + desc: 'basic transport', + connection_info: { + host: { + type: 'garbage', + desc: 'the host ip address or hostname', + }, }, - }, + } + end + + it { + expect { described_class.register(schema) }.to raise_error( + Puppet::DevError, %r{ is not a valid type specification} + ) } end - - it { - expect { described_class.register(schema) }.to raise_error( - Puppet::DevError, %r{ is not a valid type specification} - ) - } end - context 'when connecting to a transport' do + describe '#connect(name, connection_info)', agent_test: true do let(:name) { 'test_target' } let(:schema) do { @@ -141,7 +143,7 @@ end end - describe '#self.validate' do + describe '#validate(name, connection_info)', agent_test: true do context 'when the transport being validated has not be registered' do it { expect { described_class.validate('wibble', {}) }.to raise_error LoadError, %r{(no such file to load|cannot load such file) -- puppet/transport/schema/wibble} } end From 8aaedab16babca938b9016bf7e5416a98eedbc91 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 15:56:17 +0000 Subject: [PATCH 13/16] (maint) Add some docs and remove the Style/Documentation exclusions --- .rubocop.yml | 3 +-- .rubocop_todo.yml | 17 ----------------- lib/puppet/resource_api.rb | 1 + lib/puppet/resource_api/base_context.rb | 5 +++++ lib/puppet/resource_api/io_context.rb | 2 ++ lib/puppet/resource_api/puppet_context.rb | 2 ++ lib/puppet/resource_api/transport.rb | 5 +++++ 7 files changed, 16 insertions(+), 19 deletions(-) delete mode 100644 .rubocop_todo.yml diff --git a/.rubocop.yml b/.rubocop.yml index 6d831a74..5457a9ea 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,6 +1,5 @@ --- require: rubocop-rspec -inherit_from: .rubocop_todo.yml AllCops: TargetRubyVersion: '2.1' Include: @@ -157,4 +156,4 @@ Naming/UncommunicativeMethodParamName: # This cop breaks syntax highlighting in VSCode Layout/ClosingHeredocIndentation: - Enabled: false \ No newline at end of file + Enabled: false diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml deleted file mode 100644 index 63295575..00000000 --- a/.rubocop_todo.yml +++ /dev/null @@ -1,17 +0,0 @@ -# This configuration was generated by -# `rubocop --auto-gen-config` -# on 2017-09-05 11:31:11 +0100 using RuboCop version 0.49.1. -# The point is for the user to remove these configuration records -# one by one as the offenses are removed from the code base. -# Note that changes in the inspected code, or installation of new -# versions of RuboCop, may require this file to be generated again. - -# Offense count: 6 -Style/Documentation: - Exclude: - - 'spec/**/*' - - 'test/**/*' - - 'lib/puppet/resource_api.rb' - - 'lib/puppet/resource_api/base_context.rb' - - 'lib/puppet/resource_api/io_context.rb' - - 'lib/puppet/resource_api/puppet_context.rb' diff --git a/lib/puppet/resource_api.rb b/lib/puppet/resource_api.rb index 16623f6d..870eb952 100644 --- a/lib/puppet/resource_api.rb +++ b/lib/puppet/resource_api.rb @@ -12,6 +12,7 @@ require 'puppet/type' require 'puppet/util/network_device' +# This module contains the main API to register and access types, providers and transports. module Puppet::ResourceApi @warning_count = 0 diff --git a/lib/puppet/resource_api/base_context.rb b/lib/puppet/resource_api/base_context.rb index fa995648..910a7d81 100644 --- a/lib/puppet/resource_api/base_context.rb +++ b/lib/puppet/resource_api/base_context.rb @@ -1,7 +1,12 @@ require 'puppet/resource_api/type_definition' +# rubocop:disable Style/Documentation module Puppet; end module Puppet::ResourceApi; end +# rubocop:enable Style/Documentation + +# This class provides access to all common external dependencies of providers and transports. +# The runtime environment will inject an appropriate implementation. class Puppet::ResourceApi::BaseContext attr_reader :type diff --git a/lib/puppet/resource_api/io_context.rb b/lib/puppet/resource_api/io_context.rb index 3cfb4afb..d31143c4 100644 --- a/lib/puppet/resource_api/io_context.rb +++ b/lib/puppet/resource_api/io_context.rb @@ -1,5 +1,7 @@ require 'puppet/resource_api/base_context' +# Implement Resource API Conext to log through an IO object, defaulting to `$stderr`. +# There is no access to a device here. class Puppet::ResourceApi::IOContext < Puppet::ResourceApi::BaseContext def initialize(definition, target = $stderr) super(definition) diff --git a/lib/puppet/resource_api/puppet_context.rb b/lib/puppet/resource_api/puppet_context.rb index 770c7692..6dd23a51 100644 --- a/lib/puppet/resource_api/puppet_context.rb +++ b/lib/puppet/resource_api/puppet_context.rb @@ -1,6 +1,8 @@ require 'puppet/resource_api/base_context' require 'puppet/util/logging' +# Implement Resource API Context to log through Puppet facilities +# and access/expose the puppet process' current device class Puppet::ResourceApi::PuppetContext < Puppet::ResourceApi::BaseContext def device # TODO: evaluate facter_url setting for loading config if there is no `current` NetworkDevice diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index 88aa44e5..f50412e9 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -1,3 +1,8 @@ +# rubocop:disable Style/Documentation +module Puppet; end +module Puppet::ResourceApi; end +# rubocop:enable Style/Documentation + # Remote target transport API module Puppet::ResourceApi::Transport def register(schema) From bd3a21e0abd825f55ef928189d5ebb8a256b7c6c Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Thu, 24 Jan 2019 16:32:34 +0000 Subject: [PATCH 14/16] (FM-7700) Add a `list` operation returning registered transports --- lib/puppet/resource_api/transport.rb | 10 +++++++ spec/puppet/resource_api/transport_spec.rb | 32 ++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/lib/puppet/resource_api/transport.rb b/lib/puppet/resource_api/transport.rb index f50412e9..107d002a 100644 --- a/lib/puppet/resource_api/transport.rb +++ b/lib/puppet/resource_api/transport.rb @@ -18,6 +18,16 @@ def register(schema) end module_function :register # rubocop:disable Style/AccessModifierDeclarations + # retrieve a Hash of transport schemas, keyed by their name. + def list + if @transports + Marshal.load(Marshal.dump(@transports)) + else + {} + end + end + module_function :list # rubocop:disable Style/AccessModifierDeclarations + def connect(name, connection_info) validate(name, connection_info) require "puppet/transport/#{name}" diff --git a/spec/puppet/resource_api/transport_spec.rb b/spec/puppet/resource_api/transport_spec.rb index 7ad393ef..c9a748a9 100644 --- a/spec/puppet/resource_api/transport_spec.rb +++ b/spec/puppet/resource_api/transport_spec.rb @@ -88,6 +88,38 @@ end end + describe '#list' do + subject { described_class.list } + + context 'with no transports registered' do + it { is_expected.to eq({}) } + end + + context 'with a transport registered' do + let(:schema) do + { + name: 'test_target', + desc: 'a basic transport', + connection_info: { + host: { + type: 'String', + desc: 'the host ip address or hostname', + }, + }, + } + end + + before(:each) do + described_class.register(schema) + end + + it { expect(described_class.list['test_target'].definition).to eq schema } + it 'returns a new object' do + expect(described_class.list['test_target'].definition.object_id).not_to eq schema.object_id + end + end + end + describe '#connect(name, connection_info)', agent_test: true do let(:name) { 'test_target' } let(:schema) do From ac037b2945ded3837229a386685428c32939b62b Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Fri, 25 Jan 2019 09:10:33 +0000 Subject: [PATCH 15/16] (FM-7691) Add `context` parameter for test transport This find other places where we need to handle passing a context --- lib/puppet/resource_api/transport/wrapper.rb | 2 +- .../test_module/lib/puppet/transport/test_device.rb | 8 ++++---- spec/puppet/resource_api/transport/wrapper_spec.rb | 5 +++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/lib/puppet/resource_api/transport/wrapper.rb b/lib/puppet/resource_api/transport/wrapper.rb index df4f7ba3..caabc5af 100644 --- a/lib/puppet/resource_api/transport/wrapper.rb +++ b/lib/puppet/resource_api/transport/wrapper.rb @@ -21,7 +21,7 @@ def initialize(name, url_or_config) def facts # @transport.facts + custom_facts # look into custom facts work by TP - @transport.facts + @transport.facts(nil) end def respond_to_missing?(name, _include_private) diff --git a/spec/fixtures/test_module/lib/puppet/transport/test_device.rb b/spec/fixtures/test_module/lib/puppet/transport/test_device.rb index ad3a04f6..7974f1d4 100644 --- a/spec/fixtures/test_module/lib/puppet/transport/test_device.rb +++ b/spec/fixtures/test_module/lib/puppet/transport/test_device.rb @@ -1,19 +1,19 @@ module Puppet::Transport # a transport for a test_device class TestDevice - def initialize(connection_info); + def initialize(_context, connection_info); puts connection_info end - def facts + def facts(_context) { 'foo' => 'bar' } end - def verify + def verify(_context) return true end - def close + def close(_context) return end end diff --git a/spec/puppet/resource_api/transport/wrapper_spec.rb b/spec/puppet/resource_api/transport/wrapper_spec.rb index f2692b19..b5757a7a 100644 --- a/spec/puppet/resource_api/transport/wrapper_spec.rb +++ b/spec/puppet/resource_api/transport/wrapper_spec.rb @@ -42,7 +42,7 @@ it 'will return the facts provided by the transport' do allow(Puppet::ResourceApi::Transport).to receive(:connect).and_return(transport) - allow(transport).to receive(:facts).and_return(facts) + allow(transport).to receive(:facts).with(nil).and_return(facts) expect(instance.facts).to eq(facts) end @@ -53,12 +53,13 @@ context 'when the transport can handle the method' do let(:instance) { described_class.new('wibble', {}) } let(:transport) { instance_double(Puppet::Transport::TestDevice, 'transport') } + let(:context) { instance_double(Puppet::ResourceApi::PuppetContext, 'context') } it 'will return the facts provided by the transport' do allow(Puppet::ResourceApi::Transport).to receive(:connect).and_return(transport) expect(transport).to receive(:close) - instance.close + instance.close(context) end end From e4e8cbef7b52648d48d707916d4ee1ee2fe8ff00 Mon Sep 17 00:00:00 2001 From: David Schmitt Date: Fri, 25 Jan 2019 09:25:33 +0000 Subject: [PATCH 16/16] (FM-7691) pass a context to transport.facts This uses the initial `list` implementation from the previous commit to access the schema to create a context to pass around. --- lib/puppet/resource_api/transport/wrapper.rb | 4 +++- spec/puppet/resource_api/transport/wrapper_spec.rb | 5 ++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/puppet/resource_api/transport/wrapper.rb b/lib/puppet/resource_api/transport/wrapper.rb index caabc5af..81febbcb 100644 --- a/lib/puppet/resource_api/transport/wrapper.rb +++ b/lib/puppet/resource_api/transport/wrapper.rb @@ -17,11 +17,13 @@ def initialize(name, url_or_config) end @transport = Puppet::ResourceApi::Transport.connect(name, config) + @schema = Puppet::ResourceApi::Transport.list[name] end def facts + context = Puppet::ResourceApi::PuppetContext.new(@schema) # @transport.facts + custom_facts # look into custom facts work by TP - @transport.facts(nil) + @transport.facts(context) end def respond_to_missing?(name, _include_private) diff --git a/spec/puppet/resource_api/transport/wrapper_spec.rb b/spec/puppet/resource_api/transport/wrapper_spec.rb index b5757a7a..b6652424 100644 --- a/spec/puppet/resource_api/transport/wrapper_spec.rb +++ b/spec/puppet/resource_api/transport/wrapper_spec.rb @@ -37,12 +37,15 @@ describe '#facts' do context 'when called' do let(:instance) { described_class.new('wibble', {}) } + let(:context) { instance_double(Puppet::ResourceApi::PuppetContext, 'context') } let(:facts) { { 'foo' => 'bar' } } let(:transport) { instance_double(Puppet::Transport::TestDevice, 'transport') } it 'will return the facts provided by the transport' do allow(Puppet::ResourceApi::Transport).to receive(:connect).and_return(transport) - allow(transport).to receive(:facts).with(nil).and_return(facts) + allow(Puppet::ResourceApi::Transport).to receive(:list).and_return(schema: :dummy) + allow(Puppet::ResourceApi::PuppetContext).to receive(:new).and_return(context) + allow(transport).to receive(:facts).with(context).and_return(facts) expect(instance.facts).to eq(facts) end