[MODULES-4152] Implement install_module and install_module_on methods #3

wilson208 · 2016-12-05T15:51:17Z

Also implemented accompanying unit tests and readme updates. Several supporting functions implemented and unit tested also.

DavidS

Getting close!

DavidS · 2016-12-06T08:57:23Z

spec/unit/beaker/module_install_helper_spec.rb

+    before do
+      @module_source_dir = '/a/b/c/d'
+      allow(File).to receive(:exist?).and_return(true)
+      allow(File).to receive(:read).and_return('{"name": "puppetlabs-vcsrepo"}')


This needs to be more specific. Add a with() clause like you did below to avoid poisoning the well if the implementation changes later.

DavidS · 2016-12-06T09:00:09Z

spec/unit/beaker/module_install_helper_spec.rb

+    end
+  end
+
+  context 'hosts_to_install_module_on with split master/agent setup' do


The duplicated hosts_to_install_module_on in the context description hints at this and the previous context needing a joint parent. "with" is always a good indicator for denoting a context, and therefore usually should go first:

context "#hosts_to_install_module_on" do context "with split master/agent setup" do ... end context "with solo agent setup" do ... end

PS: also the description here does not describe the reality of the test.

Made changes to rectify this. Had to disable an rspec cop in .rubocop.yml to allow this change.

DavidS · 2016-12-06T09:04:36Z

lib/beaker/module_install_helper.rb

+    # valid metadata.json
+    while module_source_dir.nil? && search_in.length > 1
+      # remove last segment (file or folder, doesn't matter)
+      search_in = search_in.split('/')[0...-1].join('/')


This won't work on Windows. See https://ruby-doc.org/core-2.3.1/File.html#SEPARATOR ,https://ruby-doc.org/core-2.3.1/File.html#ALT_SEPARATOR , https://ruby-doc.org/core-2.3.1/File.html#method-c-join , https://ruby-doc.org/core-2.3.1/File.html#method-c-split , and https://ruby-doc.org/core-2.3.1/File.html#method-c-dirname

Have updated this implementation, let me know what you think!

DavidS · 2016-12-06T09:08:50Z

Gemfile

@@ -3,7 +3,7 @@ source 'https://rubygems.org'
 gemspec

 group :test do
-  gem 'beaker', '2.51.0' if RUBY_VERSION <= '2.1.6'
+  gem 'beaker', '2.52.0' if RUBY_VERSION <= '2.1.6'


Does puppet_install_helper also need this?

Have removed this. It is defined in gemspec.

DavidS · 2016-12-06T11:11:12Z

README.md


-This will install the module under test on the specified host using the local source
+### `install_module_on`
+This will install the module under test on the specified host using the local source. The module name will be derived from the name property of the module's metadata.json file, assuming it is in format author-modulename.


The documentation could also do with examples. People love copy&pasting working code ;-)

Have added usage of install_module and install_module_on. These usage examples are basic and will be more useful when further functionality is implemented.

wilson208 · 2016-12-14T10:06:37Z

@DavidS would you be able to take a look at this if you get a chance? Thanks :)

DavidS · 2016-12-14T11:16:10Z

beaker-module_install_helper.gemspec

@@ -28,5 +28,6 @@ Gem::Specification.new do |spec|
  spec.add_development_dependency 'rspec'

  # Run time dependencies
-  spec.add_runtime_dependency 'beaker', '>= 2.0'
+  spec.add_runtime_dependency 'beaker', '= 2.52' if RUBY_VERSION <= '2.1.6'
+  spec.add_runtime_dependency 'beaker' if RUBY_VERSION > '2.1.6'


This is only evaluated at gem build time, but the RUBY_VERSION constraint depends on the executing runtime.

there is no 2.52 version of beaker, it's 2.52.0

So I should be moving this dependency to the Gemfile and updating the version correctly?

This is still in the wrong place?

DavidS · 2016-12-14T11:19:49Z

lib/beaker/module_install_helper.rb

+    # valid metadata.json
+    while module_source_dir.nil? && search_in.length > 1
+      # remove last segment (file or folder, doesn't matter)
+      search_in = File.join(search_in.split(File::SEPARATOR)[0...-1])


This still looks like https://ruby-doc.org/core-2.3.1/File.html#method-c-dirname ...

Have now fixed this in the latest commits and added an Assumptions section to the README based on what we discussed about requiring a metadata.json in the module. I also included the assumption that for the metadata.json to be found, the bundle install path must be within the module directory, as the bundle install --path option allows the bundle gems to be installed anywhere on the system.

I thought https://github.com/puppetlabs/beaker-module_install_helper/pull/3/files#diff-111e45d2ddd48cd4fc5f8359327e2e81R78 makes the constraint on the install path unnecessary?

Correct. Have fixed this now, sorry!

…nd install_module_on methods, along with some supporting methods and accompanying tests

DavidS · 2016-12-14T16:43:30Z

lib/beaker/module_install_helper.rb

+    module_source_dir = nil
+    # here we go up the file tree and search the directories for a
+    # valid metadata.json
+    while module_source_dir.nil? && search_in.length > 1


we also talked about fixing this here for windows.

DavidS · 2016-12-14T18:19:43Z

lib/beaker/module_install_helper.rb

+    module_source_dir = nil
+    # here we go up the file tree and search the directories for a
+    # valid metadata.json
+    while module_source_dir.nil? && search_in.length > 2


'C:/'.length is bigger than 2. Alternatively try File.dirname(search_in) == search_in, which should work on any filesystem.

Maybe add a testcase with no metadata.json to the unit tests below, to avoid any further dead ends here ;-)

DavidS · 2016-12-14T18:21:22Z

beaker-module_install_helper.gemspec

@@ -26,7 +26,4 @@ Gem::Specification.new do |spec|

  ## Testing dependencies
  spec.add_development_dependency 'rspec'
-
-  # Run time dependencies
-  spec.add_runtime_dependency 'beaker', '>= 2.0'


this is still needed for documentation purposes.

…not found. Add spec test to cover no metadata.json found.

wilson208 force-pushed the initial_functionality branch from bcaf52a to e489736 Compare December 5, 2016 22:14

wilson208 changed the title ~~[WIP] Implement install_module_on method~~ [MODULES-4152] Implement install_module and install_module_on methods Dec 5, 2016

DavidS suggested changes Dec 6, 2016

View reviewed changes

DavidS reviewed Dec 6, 2016

View reviewed changes

wilson208 force-pushed the initial_functionality branch 2 times, most recently from 1e207ba to 348656e Compare December 6, 2016 14:17

DavidS suggested changes Dec 14, 2016

View reviewed changes

wilson208 force-pushed the initial_functionality branch 4 times, most recently from 6f46fe2 to 2235176 Compare December 14, 2016 13:14

Implementation, unit tests and documentation for the install_module a…

dc29c66

…nd install_module_on methods, along with some supporting methods and accompanying tests

wilson208 force-pushed the initial_functionality branch from 2235176 to dc29c66 Compare December 14, 2016 14:05

DavidS reviewed Dec 14, 2016

View reviewed changes

Fix for windows and move beaker dependency to Gemfile

c461d0a

DavidS reviewed Dec 14, 2016

View reviewed changes

Wilson McCoubrey and others added 2 commits December 16, 2016 12:27

Ensure traversing tree does not enter infinite loop if metadata.json …

af1cc1d

…not found. Add spec test to cover no metadata.json found.

Avoid RUBY_VERSION dependency in gemspec

dd17114

DavidS approved these changes Dec 16, 2016

View reviewed changes

wilson208 merged commit 5ff1ccb into voxpupuli:master Dec 16, 2016

wilson208 deleted the initial_functionality branch December 16, 2016 12:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MODULES-4152] Implement install_module and install_module_on methods #3

[MODULES-4152] Implement install_module and install_module_on methods #3

wilson208 commented Dec 5, 2016 •

edited

Loading

DavidS left a comment

DavidS Dec 6, 2016

wilson208 Dec 6, 2016

DavidS Dec 6, 2016

wilson208 Dec 6, 2016

DavidS Dec 6, 2016

wilson208 Dec 6, 2016

DavidS Dec 6, 2016

wilson208 Dec 6, 2016

DavidS Dec 6, 2016

wilson208 Dec 6, 2016

wilson208 commented Dec 14, 2016

DavidS Dec 14, 2016

wilson208 Dec 14, 2016

DavidS Dec 14, 2016

DavidS Dec 14, 2016

wilson208 Dec 14, 2016

DavidS Dec 14, 2016

wilson208 Dec 14, 2016

DavidS Dec 14, 2016

DavidS Dec 14, 2016

DavidS Dec 14, 2016

DavidS Dec 14, 2016

[MODULES-4152] Implement install_module and install_module_on methods #3

[MODULES-4152] Implement install_module and install_module_on methods #3

Conversation

wilson208 commented Dec 5, 2016 • edited Loading

DavidS left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilson208 commented Dec 14, 2016

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

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

Choose a reason for hiding this comment

wilson208 commented Dec 5, 2016 •

edited

Loading