Refactor nssm resource #21

Annih · 2017-07-19T21:24:26Z

Following last nssm bump with the cookbook v3.0.1 the escaping behavior changed and broke contingent cookbooks. It also added extra execution time to check parameters values.

Non goals

This PR does not aim to be backward compatible.

Main changes

[breaking] Handle escaping for the user:
- No need to use """ for parameters with spaces
- What the user set in the property is what he gets in nssm.
Use latest way to define resources (custom resource):
- Smaller code
- Separated Platform implementation
Use version in nssm path to allow upgrade scenario
Speed up the convergence:
- Load current service state at once by implementing the load_current_value using nssm dump
- Requires nssm >= 2.24.94 for the nssm dump command
Introduce start & stop actions to manipulate the service

The README and tests have been updated accordingly. I did many commits to help see how I came to the current result.

Let me kow if you have any question, remark or feedback.

Cc. @aboten

This has been done for install_if_missing but not for the install action. This could be considered as a 'breaking change'. But it fixes a bit the behavior following criteo-cookbooks#7.

Instead of performing a adhoc WIN32OLE WMI query, use a gem written for that. This gem is already packaged with chef.

This requires nssm > 2.24

Recent version of nssm now better handle command line parameters. Passing arguments properly quoted is easier. This commit quotes & escape parameters in a way that the dump output is the same as the input. It helps for idempotency and ease usage of the resource. No need to triple quotes arguments :) But this is a breaking change because it changes the public interface of this resource, even if the behavior was already broken following last nssm bump.

dhoer

Looks great overall. I have not used the new resources syntax, so I had to understand what that was. The build on appveyor is failing do to a food critic saying a resource attribute is incorrect. The build will need to pass before I can merge it. Thanks for looking at this and getting it updated!

dhoer · 2017-07-19T22:27:31Z

Gemfile

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

 gem 'berkshelf'
 gem 'chef', '>= 12.7'


Should the Chef version be bumped based on Chefspec note:
ChefSpec requires Ruby 2.2 or later and Chef 12.14.89 or later!

Yes maybe we should bump chef in the Gemfile.

dhoer · 2017-07-19T22:30:20Z

resources/service.rb

@@ -0,0 +1,96 @@
+provides :nssm_service, platform: 'windows'
+# TODO: migrate to nssm_service with a breaking change notice


Would a major bump in version v4.0.0 be enough to signal breaking change, so is nssm_service is not really needed?

Yes I think a v4.0.0 would be great, but currently I think my PR is not enough. Due to the way nssm binary is installed upgrade is not possible when a service is currently running.
In addition, the implementation I made for the load_current_value does not work with older version of nssm, another blocking point for the upgrade.

The nssm_service would be great if we start having a resource for nssm_install for instance.
I really think we should migrate the install recipe into a resource, to enhance install/upgrade scenarii and allow people to subscribe to that kind of event.

dhoer · 2017-07-19T22:46:04Z

README.md


 #### Attribute Parameters

 - `servicename` - Name attribute. The name of the Windows service.
 - `program` - The program to be run as a service.
 - `args` - String of arguments for the program. Optional
 - `parameters` - Hash of key value pairs where key represents associated registry entry. Optional
- `start` - Start service after installing. Default` -  true
+- `start` - Start service after installing. Default -  true
+- `nssm_binary` - Path to nssm binary. Default - `node['nssm']['install_location']\nssm.exe`

 ## ChefSpec Matchers


This section can go

dhoer · 2017-07-19T23:03:53Z

resources/service.rb

+property :args, kind_of: String
+property :parameters, kind_of: Hash, default: lazy { ::Mash.new }
+property :nssm_binary, kind_of: String, default: lazy { "#{node['nssm']['install_location']}\\nssm.exe" }
+# TODO: migrate this to :start action with a breaking change


Should these just be removed if we do a major rev?

dhoer · 2017-07-19T23:11:58Z

libraries/matchers.rb

-  end
-
-  ChefSpec.define_matcher :nssm
-end


This is cool, didn't know you no longer needed matchers 👍

Resource can now be used via 'nssm' or 'nssm_service'. On windows it works the same. On other platforms it just logs warnings.

Chefspec autogenerate matchers. Also bump chef in Gemfile to match Chefspec 7.1 requirements.

NSSM commands were build by manual string concatenation. Foodcritic was unhappy when this was done inside an execute resource. Refactor the parameter_helper into nssm library with higher level methods.

Use win32-service to determine whether the service exist or not. Then if the `dump` command exist, use it to retrieve all settings at once. This allow backward compatibility with older version of nssm, but idempotency won't be perfect, settings will be set at every run.

This allow nssm upgrade scenario. Each nssm service is bound to a specific version of nssm. If you decide to bump nssm, this'll update the service.

Annih · 2017-07-24T10:20:13Z

@dhoer I managed to get green CI :D
I also updated the code to take into account your comments and try to be as much backward-compatible as possible.

I'm creating smaller reviews for different stuffs.

Annih · 2017-07-24T15:50:07Z

Wow quick merge, thanks!

dhoer · 2017-07-24T16:10:54Z

Getting the following error:
resource execute[Install service name service] is configured to notify resource service[service name] with action start, but service[service name] cannot be found in the resource collection. execute[Install service name service] is defined in C:/Users/TEST_U~1/AppData/Local/Temp/kitchen/cache/cookbooks/nssm/resources/service.rb:31:in `block in class_from_file'

Do you have time to look at this?

Annih · 2017-07-24T16:41:24Z

This is related to #22, I was looking at it but your merged it before I managed to get my test OK :D

I'll try again ;)

Annih added 9 commits July 18, 2017 13:48

Properly escape parameters in install action

4f1e4de

This has been done for install_if_missing but not for the install action. This could be considered as a 'breaking change'. But it fixes a bit the behavior following criteo-cookbooks#7.

Use win32-service to detect if nssm is installed

dfe4407

Instead of performing a adhoc WIN32OLE WMI query, use a gem written for that. This gem is already packaged with chef.

Use findstr instead of grep

b890878

Rewrite nssm resource into a custom resource

d51f0f4

Replace nssm_exe action_class method by a property

e815f19

Implement load_current_value using nssm dump

441ecdd

This requires nssm > 2.24

Call action install in install_if_missing

f58d148

Introduce start & stop actions

da6a965

Annih force-pushed the rewrite_provider branch from 8daa09a to 2add07d Compare July 19, 2017 21:24

dhoer reviewed Jul 19, 2017

View reviewed changes

libraries/matchers.rb

end

ChefSpec.define_matcher :nssm

end

Copy link

Contributor

dhoer Jul 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is cool, didn't know you no longer needed matchers 👍

Annih added 5 commits July 24, 2017 11:24

Rename the resource and separate platform implementation

1836982

Resource can now be used via 'nssm' or 'nssm_service'. On windows it works the same. On other platforms it just logs warnings.

Update README to reflect new behavior

3055255

Update tests to reflect new behavior

106d66e

Bump chefspec and remove matchers

74f97c6

Chefspec autogenerate matchers. Also bump chef in Gemfile to match Chefspec 7.1 requirements.

Improve helper to generate nssm commands

a1db68d

NSSM commands were build by manual string concatenation. Foodcritic was unhappy when this was done inside an execute resource. Refactor the parameter_helper into nssm library with higher level methods.

Annih force-pushed the rewrite_provider branch from 2add07d to f967a80 Compare July 24, 2017 09:33

Annih mentioned this pull request Jul 24, 2017

Version 3.0.1 is missing %WINDIR% #16

Open

Annih force-pushed the rewrite_provider branch 2 times, most recently from 8db4688 to 95df8d0 Compare July 24, 2017 09:58

Version nssm_binary and make it a real property

602043c

This allow nssm upgrade scenario. Each nssm service is bound to a specific version of nssm. If you decide to bump nssm, this'll update the service.

Annih force-pushed the rewrite_provider branch from 95df8d0 to 602043c Compare July 24, 2017 10:10

This was referenced Jul 24, 2017

Stop service on removal #23

Merged

Fix nssm start behavior #22

Merged

dhoer merged commit af675e7 into criteo-cookbooks:master Jul 24, 2017

Annih deleted the rewrite_provider branch July 24, 2017 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor nssm resource #21

Refactor nssm resource #21

Annih commented Jul 19, 2017 •

edited

Loading

dhoer left a comment

dhoer Jul 19, 2017

Annih Jul 20, 2017

dhoer Jul 19, 2017

Annih Jul 20, 2017

dhoer Jul 19, 2017

dhoer Jul 19, 2017

dhoer Jul 19, 2017

Annih commented Jul 24, 2017

Annih commented Jul 24, 2017

dhoer commented Jul 24, 2017

Annih commented Jul 24, 2017

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

		gem 'berkshelf'
		gem 'chef', '>= 12.7'

		@@ -0,0 +1,96 @@
		provides :nssm_service, platform: 'windows'
		# TODO: migrate to nssm_service with a breaking change notice

Refactor nssm resource #21

Refactor nssm resource #21

Conversation

Annih commented Jul 19, 2017 • edited Loading

Non goals

Main changes

dhoer left a comment

Choose a reason for hiding this comment

dhoer Jul 19, 2017

Choose a reason for hiding this comment

Annih Jul 20, 2017

Choose a reason for hiding this comment

dhoer Jul 19, 2017

Choose a reason for hiding this comment

Annih Jul 20, 2017

Choose a reason for hiding this comment

dhoer Jul 19, 2017

Choose a reason for hiding this comment

dhoer Jul 19, 2017

Choose a reason for hiding this comment

dhoer Jul 19, 2017

Choose a reason for hiding this comment

Annih commented Jul 24, 2017

Annih commented Jul 24, 2017

dhoer commented Jul 24, 2017

Annih commented Jul 24, 2017

Annih commented Jul 19, 2017 •

edited

Loading