Replace staging module with archive module #128

TraGicCode · 2017-09-13T21:51:27Z

This will get rid of the staging module being used and replace it with the newer recommended archive module.

Closes #127

@wyardley Can you review this?

TraGicCode · 2017-09-13T22:00:03Z

I Still need to test this on windows and other nodes besides ubuntu. Just need a set of eyes to look at this.

wyardley · 2017-09-13T22:26:41Z

Can you suppress the merge (rebasing is better), and squash it down into either 1 commit or a few commits that are broken up into logical functions.

Did you run the acceptance tests yet?

wyardley · 2017-09-13T23:00:10Z

manifests/params.pp

@@ -80,7 +80,7 @@
  # Based on the small number of inputs above, we can construct sane defaults
  # for pretty much everything else.

-  # Settings common to everything
+  # # Settings common to everything


Is the extra comment mark a mistake here?

Yes. Was when i was commenting out staging to get everything to fail and then replace it

wyardley

Looks Ok to me, other than the extra comment mark. I'd like it if someone who's more familiar with this module could also review, though.

alexjfisher · 2017-09-14T07:36:12Z

manifests/platform/solaris.pp

  include ::splunk::params
  include ::splunk::platform::posix

-  $path         = $staging::path
+  $path         = $archive::path


There's no such variable as $archive::path

Hey @alexjfisher It does work but i might be accessing this the wrong way from the puppet archive module

My mistake. I missed the inherits https://github.com/voxpupuli/puppet-archive/blob/master/manifests/init.pp#L28

The staging module created $staging::path for you. With archive, I think you'll need to include ::archive::staging instead of just ::archive ??

Also needed in forwarder.pp ?

alexjfisher · 2017-09-14T07:40:00Z

spec/spec_helper_system.rb

@@ -19,7 +19,7 @@

    # Install modules and dependencies
    puppet_module_install(source: proj_root, module_name: 'splunk')
-    shell('puppet module install nanliu-staging --version 0.3.1')
+    shell('puppet module install puppet-archive --version 0.3.1')


There's no such version of puppet/archive.

alexjfisher · 2017-09-14T12:03:46Z

manifests/forwarder.pp

@@ -73,16 +73,16 @@
  $path_delimiter  = $splunk::params::path_delimiter
  #no need for staging the source if we have yum or apt
  if $pkg_provider != undef and $pkg_provider != 'yum' and $pkg_provider != 'apt' and $pkg_provider != 'chocolatey' {
-    include ::staging
+    include ::archive


include ::archive::staging here too?

TraGicCode · 2017-09-14T12:05:12Z

Before this can be merged there is one thing that will break all existing windows users that don't have chocolatey. Briefly looking, by default, it looks like the archive class in the archive module assumes the user has 7zip installed and if not it will install it via chocolatey.

Therefore i need to do the following:

See if puppet-archive will allow downloading a file without requiring 7zip and chocolatey

alexjfisher · 2017-09-14T12:10:51Z

If you're not needing to extract anything you download, I don't see why you'd need to include ::archive. (You would then need to create and use your own 'staging' path though).

wyardley

I think spec_helper_system.rb and any related plumbing can probably be removed outright (in a separate PR, if not here)?

wyardley · 2017-09-14T19:05:40Z

spec/spec_helper_system.rb

@@ -19,7 +19,7 @@

    # Install modules and dependencies
    puppet_module_install(source: proj_root, module_name: 'splunk')
-    shell('puppet module install nanliu-staging --version 0.3.1')
+    shell('puppet module install puppet-archive --version 2.0.0')


btw, it seems like this is for https://github.com/puppetlabs/rspec-system, which is deprecated and presumably not in use now that the module has beaker based acceptance tests

No Longer applicable

@alexjfisher

dismissing in favor of @alexjfisher's comments

TraGicCode · 2017-09-15T02:06:29Z

I tested on windows and it looks like that block never executes that uses the archive module. If you pass in a folder location where the msi exists it will use that existing file to install the forwarder. If you pass in an HTTP/S url the package resource will stream the msi and perform the install without ever saving the file on the windows system. @alexjfisher From what i can tell this should be good. Tested on

Ubuntu 16.04 ( server and forwarder )
Windows ( only forwarder..looks like server has never worked )

bastelfreak · 2017-09-20T20:22:09Z

spec/spec_helper_system.rb

+
+    # Install modules and dependencies
+    puppet_module_install(source: proj_root, module_name: 'splunk')
+    shell('puppet module install puppet-archive --version 2.0.0')


please take a look at the spec_helper_acceptance here:
voxpupuli/puppet-grafana@d09835a

install_module_dependencies installs all modules from the metadata.json, install_module_from_forge is for additional dependencies.

After #136 is merged i can do this.

@bastelfreak Done.

…e dependencies.

ekohl · 2017-09-21T12:14:50Z

spec/spec_helper_system.rb

@@ -0,0 +1,27 @@
+require 'rspec-system/spec_helper'


I believe spec_helper_system.rb is no longer used. Nowadays we only use spec_helper_acceptance.rb

I'll get that as well.

TraGicCode · 2017-09-21T13:26:00Z

I believe i've cleared up all of the raised issues

bastelfreak · 2017-09-23T13:32:08Z

@TraGicCode looks good to me. @alexjfisher can you check this PR again?

wyardley reviewed Sep 13, 2017

View reviewed changes

wyardley previously approved these changes Sep 13, 2017

View reviewed changes

wyardley requested a review from bastelfreak September 13, 2017 23:03

alexjfisher requested changes Sep 14, 2017

View reviewed changes

alexjfisher reviewed Sep 14, 2017

View reviewed changes

wyardley reviewed Sep 14, 2017

View reviewed changes

bastelfreak requested changes Sep 20, 2017

View reviewed changes

TraGicCode added 2 commits September 21, 2017 07:06

Replaced staging module with archive module

47e7757

Update spec_helper to utilize new recommended way of installing modul…

6f5bee3

…e dependencies.

ekohl reviewed Sep 21, 2017

View reviewed changes

Removing old rspec-system stuff

a1f82b6

bastelfreak approved these changes Sep 23, 2017

View reviewed changes

alexjfisher approved these changes Sep 23, 2017

View reviewed changes

alexjfisher merged commit c732f38 into voxpupuli:master Sep 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace staging module with archive module #128

Replace staging module with archive module #128

TraGicCode commented Sep 13, 2017 •

edited

Loading

TraGicCode commented Sep 13, 2017

wyardley commented Sep 13, 2017

wyardley Sep 13, 2017

TraGicCode Sep 13, 2017

TraGicCode Sep 14, 2017

wyardley left a comment

alexjfisher Sep 14, 2017

TraGicCode Sep 14, 2017

alexjfisher Sep 14, 2017

alexjfisher Sep 14, 2017

TraGicCode Sep 14, 2017

alexjfisher Sep 14, 2017

TraGicCode Sep 15, 2017

alexjfisher Sep 14, 2017

TraGicCode Sep 14, 2017

alexjfisher Sep 14, 2017

TraGicCode Sep 14, 2017

TraGicCode commented Sep 14, 2017 •

edited

Loading

alexjfisher commented Sep 14, 2017

wyardley left a comment

wyardley Sep 14, 2017

TraGicCode Sep 21, 2017

TraGicCode commented Sep 15, 2017

bastelfreak Sep 20, 2017

TraGicCode Sep 21, 2017

TraGicCode Sep 21, 2017

ekohl Sep 21, 2017

TraGicCode Sep 21, 2017

TraGicCode Sep 21, 2017

TraGicCode commented Sep 21, 2017

bastelfreak commented Sep 23, 2017

Replace staging module with archive module #128

Replace staging module with archive module #128

Conversation

TraGicCode commented Sep 13, 2017 • edited Loading

TraGicCode commented Sep 13, 2017

wyardley commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyardley 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

Choose a reason for hiding this comment

TraGicCode commented Sep 14, 2017 • edited Loading

alexjfisher commented Sep 14, 2017

wyardley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TraGicCode commented Sep 15, 2017

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

TraGicCode commented Sep 21, 2017

bastelfreak commented Sep 23, 2017

TraGicCode commented Sep 13, 2017 •

edited

Loading

TraGicCode commented Sep 14, 2017 •

edited

Loading