MODULES-5690: Implementing Rubocop in the module #418

david22swan · 2017-10-09T16:03:06Z

No description provided.

tphoney · 2017-10-10T13:49:29Z

spec/acceptance/ntp_service_spec.rb

+when 'AIX'
+  servicename = 'xntpd'
+else
+  servicename = if fact('operatingsystem') == 'SLES' && fact('operatingsystemmajrelease') == '12'


tphoney · 2017-10-10T13:53:12Z

spec/acceptance/ntp_install_spec.rb

  when '5.11'
    packagename = 'service/network/ntp'
  end
 else
-  if fact('operatingsystem') == 'SLES' and fact('operatingsystemmajrelease') == '12'
-    servicename = 'ntpd'
+  if fact('operatingsystem') == 'SLES' && fact('operatingsystemmajrelease') == '12'


it looks like servicename will not be set for solaris

pmcmaw

2 minor changes but class work @david22swan!

pmcmaw · 2017-10-10T14:12:47Z

.sync.yml

+.travis.yml:
+  extras:
+  - rvm: 2.1.9
+    script: bundle exec rake rubocop


Would it be possible to remove this line break.

Understand now, have resolved

Have you pushed this change? I can still see it.

pmcmaw · 2017-10-10T14:18:04Z

spec/spec_helper.rb

@@ -13,4 +13,5 @@
 begin
  require 'spec_helper_local'
 rescue LoadError
+  puts 'Rescue Load Error'
 end


All changes in this file should probably be done in Module Sync rather than in the actual file.

Didn't know that.
Was brought up as a rubocop error so I changed it.

Fix has been made to modulesync

tphoney · 2017-10-11T13:55:04Z

@david22swan i am happy with this, due to the large number of changes it would be great if @hunner @HAIL9000 or @eputnam could look as well.

david22swan · 2017-10-11T14:12:07Z

Ok

hunner · 2017-10-11T15:36:51Z

spec/acceptance/ntp_install_spec.rb

  when '5.11'
    packagename = 'service/network/ntp'
  end
-else
-  if fact('operatingsystem') == 'SLES' and fact('operatingsystemmajrelease') == '12'


Did the sles conditional get re-added somewhere?

To my knowledge this peace of code did not seem to do anything at all, and seemed to be a remnant of something from earlier.

hunner · 2017-10-11T15:38:12Z

spec/acceptance/ntp_parameters_spec.rb

    end
  end

  describe 'keys' do
-    it 'enables the key parameters' do
-      pp = <<-EOS
+    pp = <<-EOS


I think this should be a let(:pp) block if it's outside of the it but used inside of the it. See http://www.betterspecs.org/#let and referenced stackoverflow link.

Have made the change, though a little unsure of why it was necesary

hunner · 2017-10-11T15:39:20Z

spec/acceptance/ntp_parameters_spec.rb

    end
  end

  describe 'package' do
-    it 'installs the right package' do
-      pp = <<-EOS
+    pp = <<-EOS


hunner · 2017-10-11T15:41:12Z

spec/acceptance/class_spec.rb

-describe 'ntp class:', :unless => UNSUPPORTED_PLATFORMS.include?(fact('osfamily')) do
-  it 'should run successfully' do
+describe 'ntp class:', unless: UNSUPPORTED_PLATFORMS.include?(fact('osfamily')) do
+  context 'ntp' do
    pp = "class { 'ntp': }"


There's no actual explanation to ditto?

Whoops weird... github was missing some comments and put them out of order, but I guess I didn't fix it right... this is what the first was supposed to say:

I think this should be a let(:pp) block if it's outside of the it but used inside of the it. See http://www.betterspecs.org/#let and referenced stackoverflow link.

hunner · 2017-10-11T15:43:07Z

spec/acceptance/disable_monitoring_spec.rb


-describe "ntp class with disable_monitor:", :unless => UNSUPPORTED_PLATFORMS.include?(fact('osfamily')) do
+describe 'ntp class with disable_monitor:', unless: UNSUPPORTED_PLATFORMS.include?(fact('osfamily')) do
  context 'should disable' do
    pp = "class { 'ntp': disable_monitor => true }"


Ditto, even though this was pre-existing.

hunner · 2017-10-11T15:49:33Z

spec/acceptance/ntp_service_spec.rb


-describe 'service is unmanaged' do
-  it 'shouldnt stop the service' do
+  describe 'service is unmanaged' do
    pp = <<-EOS


hunner · 2017-10-11T15:51:19Z

spec/acceptance/ntp_service_spec.rb

-      it 'should be stopped' do
-        expect(output.stdout).to match(/Active\:\s+inactive/)
+        # HACK: until we either update SpecInfra or come up with alternative
+        output = shell('service ntpd status', acceptable_exit_codes: [0, 3])


Ugh. I think this should be a let block as well. That way the execution is delayed until it's actually referenced. I understand that this isn't part of rubocop/style changes though.

hunner · 2017-10-11T15:51:31Z

spec/acceptance/preferred_servers_spec.rb


-describe 'preferred servers', :unless => UNSUPPORTED_PLATFORMS.include?(fact('osfamily')) do
+describe 'preferred servers', unless: UNSUPPORTED_PLATFORMS.include?(fact('osfamily')) do
  pp = <<-EOS


Ditto, even though this was pre-existing.

hunner · 2017-10-11T15:52:16Z

spec/acceptance/restrict_spec.rb

  context 'should run successfully' do
+    pp = "class { 'ntp': restrict => ['test restrict']}"


hunner · 2017-10-11T15:52:33Z

spec/acceptance/statistics_spec.rb

  context 'should run successfully' do
+    pp = "class { 'ntp': statistics => ['loopstats'], disable_monitor => false}"


david22swan · 2017-10-17T16:32:45Z

Ok.
Have put let blocks around majority of variables that were requested by @hunner.
Had to remove blocks around '<<-EOS EOS' blocks as it seemed to confuse Rubocop

MODULES-5690: Implementing Rubocop in the module

david22swan requested review from hunner and pmcmaw October 10, 2017 13:07

tphoney reviewed Oct 10, 2017

View reviewed changes

pmcmaw reviewed Oct 10, 2017

View reviewed changes

pmcmaw approved these changes Oct 11, 2017

View reviewed changes

hunner suggested changes Oct 11, 2017

View reviewed changes

MODULES-5690: Implementing Rubocop in the module

13069d5

hunner approved these changes Oct 17, 2017

View reviewed changes

hunner merged commit 13069d5 into puppetlabs:master Oct 17, 2017

hunner added a commit that referenced this pull request Oct 17, 2017

Merge pull request #418 from david22swan/MODULES-5690

30da622

MODULES-5690: Implementing Rubocop in the module

david22swan deleted the MODULES-5690 branch October 18, 2017 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MODULES-5690: Implementing Rubocop in the module #418

MODULES-5690: Implementing Rubocop in the module #418

david22swan commented Oct 9, 2017

tphoney Oct 10, 2017

tphoney Oct 10, 2017

pmcmaw left a comment

pmcmaw Oct 10, 2017

david22swan Oct 11, 2017

pmcmaw Oct 11, 2017

david22swan Oct 11, 2017

pmcmaw Oct 11, 2017

pmcmaw Oct 10, 2017

david22swan Oct 10, 2017

david22swan Oct 11, 2017

tphoney commented Oct 11, 2017

david22swan commented Oct 11, 2017

hunner Oct 11, 2017

david22swan Oct 16, 2017

hunner Oct 11, 2017 •

edited

Loading

david22swan Oct 16, 2017

hunner Oct 11, 2017

david22swan Oct 16, 2017

hunner Oct 11, 2017 •

edited

Loading

david22swan Oct 16, 2017

hunner Oct 17, 2017

hunner Oct 11, 2017

hunner Oct 11, 2017

hunner Oct 11, 2017

david22swan Oct 16, 2017

hunner Oct 11, 2017

hunner Oct 11, 2017

david22swan Oct 16, 2017

hunner Oct 11, 2017

david22swan Oct 16, 2017

david22swan commented Oct 17, 2017

		context 'should run successfully' do
		pp = "class { 'ntp': restrict => ['test restrict']}"

		context 'should run successfully' do
		pp = "class { 'ntp': statistics => ['loopstats'], disable_monitor => false}"

MODULES-5690: Implementing Rubocop in the module #418

MODULES-5690: Implementing Rubocop in the module #418

Conversation

david22swan commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmcmaw 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

tphoney commented Oct 11, 2017

david22swan commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hunner Oct 11, 2017 • edited Loading

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

hunner Oct 11, 2017 • edited Loading

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

david22swan commented Oct 17, 2017

hunner Oct 11, 2017 •

edited

Loading

hunner Oct 11, 2017 •

edited

Loading