Basic support for windows platform #89

skylerto · 2018-04-23T12:39:12Z

Description

Adds support at a very basic level to get things working on windows.

Issues Resolved

#88

Check List

All tests pass. See https://github.com/chef-cookbooks/community_cookbook_documentation/blob/master/TESTING.MD
New functionality includes testing.
New functionality has been documented in the README if applicable
All commits have been signed for the Developer Certificate of Origin. See https://github.com/chef-cookbooks/community_cookbook_documentation/blob/master/CONTRIBUTING.MD

clean_array not able to create a valid input for shell_out on windows Signed-off-by: skylerto <skylerclayne@gmail.com>

Signed-off-by: skylerto <skylerclayne@gmail.com>

skylerto · 2018-04-23T12:43:56Z

Not sure I'd like to see windows support in the documentation quiet yet as it's barely featured.

skylerto · 2018-04-23T17:18:19Z

can I get a comment @jtimberman ? 😄

Signed-off-by: skylerto <skylerclayne@gmail.com>

jtimberman

See comments in-line. We should be preserving the methods that are used; as far as I know, they are the Best API To Use from previous discussions with @lamont-granquist about changes in this cookbook, in particular.

I'd rather we add Windows-specific testing rather than a conditional that largely just indents the recipes by two spaces (which creates diff noise that make it hard to track down changes later).

jtimberman · 2018-04-25T22:59:17Z

libraries/provider_hab_package.rb

@@ -87,7 +87,7 @@ def remove_package(_name, _version)
        private

        def hab(*command)
-          shell_out_with_timeout!(clean_array('hab', *command))
+          shell_out_with_timeout!(['hab', *command].flatten.compact.join(' '))


Please preserve clean_array - this was previously discussed awhile back with regard to this cookbook: #11 (comment)

instead of passing a single string to shell_out that winds up passing an array which automatically fixes files-with-spaces-needing-surrounding-quotes errors (typically windows) without having to think about it or use quotes.

I totally agree that preserving the API's makes the most sense however, the API is broken for this use case; this is a band-aid solution to get things working on windows.

Passing the array to shell_out_with_timeout! was the problem, on windows when you try to execute this you get the following error:

NoMethodError ------------- undefined method `each_char' for ["hab", "pkg", "path", "skylerto/splunkforwarder"]:Array Did you mean? each each_cons

and debug stacktrace:

C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/mixlib-shellout-2.2.7-universal-mingw32/lib/mixlib/shellout/windows.rb:266:in `should_run_under_cmd?' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/mixlib-shellout-2.2.7-universal-mingw32/lib/mixlib/shellout/windows.rb:191:in `command_to_run' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/mixlib-shellout-2.2.7-universal-mingw32/lib/mixlib/shellout/windows.rb:59:in `run_command' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/mixlib-shellout-2.2.7-universal-mingw32/lib/mixlib/shellout.rb:259:in `run_command' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/mixin/shell_out.rb:191:in `shell_out_command' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/mixin/shell_out.rb:112:in `shell_out' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/mixin/shell_out.rb:117:in `shell_out!' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/provider/package.rb:637:in `shell_out_with_timeout!' C:/Users/vagrant/AppData/Local/Temp/kitchen/cache/cookbooks/habitat/libraries/provider_hab_package.rb:90:in `hab' C:/Users/vagrant/AppData/Local/Temp/kitchen/cache/cookbooks/habitat/libraries/provider_hab_package.rb:144:in `installed_version' C:/Users/vagrant/AppData/Local/Temp/kitchen/cache/cookbooks/habitat/libraries/provider_hab_package.rb:139:in `block in current_versions' C:/Users/vagrant/AppData/Local/Temp/kitchen/cache/cookbooks/habitat/libraries/provider_hab_package.rb:138:in `map' C:/Users/vagrant/AppData/Local/Temp/kitchen/cache/cookbooks/habitat/libraries/provider_hab_package.rb:138:in `current_versions' C:/Users/vagrant/AppData/Local/Temp/kitchen/cache/cookbooks/habitat/libraries/provider_hab_package.rb:59:in `load_current_resource' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/provider.rb:128:in `run_action' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/resource.rb:622:in `run_action' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/runner.rb:69:in `run_action' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/runner.rb:97:in `block (2 levels) in converge' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/runner.rb:97:in `each' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/runner.rb:97:in `block in converge' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/resource_collection/resource_list.rb:94:in `block in execute_each_resource' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/resource_collection/stepable_iterator.rb:114:in `call_iterator_block' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/resource_collection/stepable_iterator.rb:85:in `step' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/resource_collection/stepable_iterator.rb:103:in `iterate' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/resource_collection/stepable_iterator.rb:55:in `each_with_index' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/resource_collection/resource_list.rb:92:in `execute_each_resource' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/runner.rb:96:in `converge' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/client.rb:715:in `block in converge' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/client.rb:710:in `catch' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/client.rb:710:in `converge' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/client.rb:749:in `converge_and_save' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/client.rb:286:in `run' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/application.rb:277:in `run_with_graceful_exit_option' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/application.rb:253:in `block in run_chef_client' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/local_mode.rb:44:in `with_server_connectivity' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/application.rb:236:in `run_chef_client' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/application/client.rb:464:in `sleep_then_run_chef_client' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/application/client.rb:451:in `block in interval_run_chef_client' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/application/client.rb:450:in `loop' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/application/client.rb:450:in `interval_run_chef_client' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/application/client.rb:434:in `run_application' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/lib/chef/application.rb:59:in `run' C:/opscode/chef/embedded/lib/ruby/gems/2.3.0/gems/chef-12.20.3-universal-mingw32/bin/chef-client:26:in `<top (required)>' C:/opscode/chef/bin/chef-client:68:in `load' C:/opscode/chef/bin/chef-client:68:in `<main>'

Mixlib::ShellOut::Windows#should_run_under_cmd?(command) expects command to respond to each_char.

https://github.com/chef/mixlib-shellout/blob/master/lib/mixlib/shellout/windows.rb#L270

This is why I figured it was less intrusive and more time efficient than submitting a PR to mixlib-shellout and waiting for a new release to make that into chef.

I see Chef 12.20.3 in the stack trace. Chef 12 is EOL - have you tried the latest Chef 14?

Same thing with system into:

System Info: ------------ chef_version=14.0.202 platform=windows platform_version=10.0.14393 ruby=ruby 2.5.1p57 (2018-03-29 revision 63029) [x64-mingw32] program_name=C:/opscode/chef/bin/chef-client executable=C:/opscode/chef/bin/chef-client

jtimberman · 2018-04-25T23:00:32Z

resources/config.rb

+              new_resource.service_group, incarnation,
+              tempfile.path].flatten.compact.join(' ')
+
+      shell_out(args)


Please preserve use of shell_out_compact! - via Chef source where this method is defined:

# generally its best to use shell_out_compact! in code and setup expectations on shell_out! in tests

This method is used with the clean_array method, and was intentionally done when this was all originally written.

Similar to my above comment clean_array is also in the call stack for the shell_out_compact! method.

jtimberman · 2018-04-25T23:01:30Z

test/fixtures/cookbooks/test/recipes/config.rb

+    )
+  end
+else
+  apt_update


I'm going to comment here, rather than across the rest of this fixture's changes.

We should use a platform specific suite for test kitchen, and configure .kitchen.yml, rather than make a bunch of conditional changes in the recipes. A lot of lines get changed in these recipes which make a lot of diff noise.

You're totally right, that would absolutely be the right way. Kind of embarrassed this got committed.

Signed-off-by: skylerto <skylerclayne@gmail.com>

skylerto · 2018-08-09T14:56:22Z

@jonlives updated as requested. :)

jonlives · 2018-08-09T15:05:46Z

resources/config.rb

    begin
      tempfile.write(TOML::Generator.new(new_resource.config).body)
      tempfile.close
      if Gem::Requirement.new('>= 14.3.20').satisfied_by?(Gem::Version.new(Chef::VERSION))
-        shell_out!(*command)
+        shell_out!(command)


What's the rationale behind removing the splat operator here? Is it not needed anymore?

the rationale is the same as the above issue for adding windows support.

shell_out! for windows doesn't support array args. We're joining the command array into one string command:

command = [ 'hab', 'config', 'apply', opts, new_resource.service_group, incarnation, tempfile.path ].flatten.compact.join(' ')

jonlives · 2018-08-09T15:05:54Z

resources/config.rb

      else
-        shell_out_compact!(*command)
+        shell_out_compact!(command)


What's the rationale behind removing the splat operator here? Is it not needed anymore?

see above. :)

jonlives

Approving changes, waiting for a couple of other PRs to finalise review before merge.

jonlives · 2018-09-04T13:30:43Z

Merging this now - will take me a few days to work through the list of outstanding pull requests and cut a new release, so please bear with me!

Obvious fix; these changes are the result of automation not creative thinking.

skylerto added 3 commits April 19, 2018 15:15

clean_array failing on windows

e8bc1b4

clean_array not able to create a valid input for shell_out on windows Signed-off-by: skylerto <skylerclayne@gmail.com>

flatten and compact explicitly

573febd

Signed-off-by: skylerto <skylerclayne@gmail.com>

windows tests

8da09ab

Signed-off-by: skylerto <skylerclayne@gmail.com>

rubocop cleanups

c554dcb

Signed-off-by: skylerto <skylerclayne@gmail.com>

jtimberman suggested changes Apr 25, 2018

View reviewed changes

skylerto added 2 commits April 26, 2018 08:58

clean up test cookbook, add windows recipes

69cd9b9

Signed-off-by: skylerto <skylerclayne@gmail.com>

preserve clean_array

1140d5b

Signed-off-by: skylerto <skylerclayne@gmail.com>

skylerto mentioned this pull request Apr 26, 2018

should_run_under_cmd? fails on windows when argument is an array chef/mixlib-shellout#154

Closed

Merge branch 'master' into master

a06dfe3

jonlives suggested changes Aug 9, 2018

View reviewed changes

jonlives approved these changes Aug 9, 2018

View reviewed changes

jonlives merged commit 9351db7 into chef-boneyard:master Sep 4, 2018

chef-ci pushed a commit that referenced this pull request Sep 4, 2018

Update CHANGELOG.md with details from pull request #89

95401d2

Obvious fix; these changes are the result of automation not creative thinking.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic support for windows platform #89

Basic support for windows platform #89

skylerto commented Apr 23, 2018

skylerto commented Apr 23, 2018

skylerto commented Apr 23, 2018

jtimberman left a comment

jtimberman Apr 25, 2018

skylerto Apr 26, 2018

skylerto Apr 26, 2018 •

edited

jtimberman Apr 26, 2018

skylerto Apr 26, 2018 •

edited

jtimberman Apr 25, 2018

skylerto Apr 26, 2018

jtimberman Apr 25, 2018

skylerto Apr 26, 2018

skylerto commented Aug 9, 2018

jonlives Aug 9, 2018

skylerto Aug 9, 2018

jonlives Aug 9, 2018

skylerto Aug 9, 2018

jonlives left a comment

jonlives commented Sep 4, 2018

Basic support for windows platform #89

Basic support for windows platform #89

Conversation

skylerto commented Apr 23, 2018

Description

Issues Resolved

Check List

skylerto commented Apr 23, 2018

skylerto commented Apr 23, 2018

jtimberman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skylerto Apr 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skylerto Apr 26, 2018 • edited

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

skylerto commented Aug 9, 2018

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

jonlives left a comment

Choose a reason for hiding this comment

jonlives commented Sep 4, 2018

skylerto Apr 26, 2018 •

edited

skylerto Apr 26, 2018 •

edited