Added windows support #352

KZachariassen · 2017-07-14T10:06:54Z

This is an updated attempt to add windows support, it is based on the old branch feature/windows_support but has been updates and tested on server 2016.

Could we get this merged and mark windows support as experimental?

Fixes #195

solarkennedy · 2017-07-15T18:03:35Z

Certainly! Can you rebase against current master first to resolve the merge conflicts?

# Conflicts: # manifests/config.pp # manifests/install.pp # manifests/params.pp # manifests/run_service.pp

KZachariassen · 2017-07-17T08:41:15Z

Conflict should be solved now, I have simplified the logic around init_style for windows, so it reuse existing logic around $init_style = 'unmanaged'

Windows needs 775 to create sym-link

solarkennedy · 2017-07-17T14:33:14Z

manifests/install.pp

+  case $::operatingsystem {
+    'windows': {
+      $binary_name = 'consul.exe'
+      $binary_mode = '0775'


why does windows need different permissions here?

If we already know what the group should be, I'm ok with making 775 being the default for both oses. Does 555 not work for running windows binaries?

I just ran a test on the, and the binary_mode can be set to 755, but 555 does not work on windows 2016. I have pushed a version where I set it to 755 for both data_dir and binary

KZachariassen · 2017-07-17T15:04:57Z

Let me do another test tomorrow and I'll let you know.

A domain joined machine needs 775, to allow domain users to create files and folders in the install_dir.

robklg · 2017-09-11T14:24:29Z

Hey, I was just looking into adding windows support to this puppet module. I see this merge request was about to get merged, but hasn't yet?

Is there something I can do?
Does it needs some more testing first? We use Windows Server 2012.

solarkennedy · 2017-09-11T14:40:41Z

The main blocker is that this PR installs nssm on behalf of the user, which I don't think this module should do.

solarkennedy · 2017-09-11T14:41:25Z

OMG it looks like all my review comments are still pending because I didn't hit the "submit review" button... :(

solarkennedy · 2017-07-17T14:36:53Z

manifests/windows_service.pp

+class consul::windows_service (
+  $nssm_version = '2.24',
+  $nssm_download_url = undef,
+  $nssm_download_url_base = 'https://nssm.cc/release',


This is a large new dependency that I didn't expect.

I'm ok with depending on nssm if you think that is the right tool for the job on windows, but I don't think it is appropriate for the consul module to install it on the users behalf.

Please instead assume it is available, add documentation on how to install it via something like
https://github.com/opentable/puppet-nssm

And then change the consul code to do the parameter execing

This is still a blocker for acceptance. The consul module should not presume to install nssm for the user.

However it can require it and assume it is available as long as it is documented in the README

solarkennedy · 2017-07-17T14:44:19Z

manifests/windows_service.pp

+    provider  => 'powershell',
+    notify    => Exec['consul_service_set_parameters']
+  }
+  file { "${consul::bin_dir}/set_service_parameters.ps1":


It does look like are in good company using nssm for this
https://chocolatey.org/packages/consul

Can you "show" tools\chocolateyInstall.ps1 on that page and make sure there isn't anything we missed?

solarkennedy · 2017-07-17T14:44:42Z

manifests/windows_service.pp

+  file { "${consul::bin_dir}/set_service_parameters.ps1":
+    ensure  => 'present',
+    content => template('consul/set_service_parameters.ps1.erb'),
+    notify  => Exec['consul_service_set_parameters']


Just do the ~> shorthand

solarkennedy · 2017-07-17T14:45:43Z

manifests/windows_service.pp

+    command     => "${consul::bin_dir}/set_service_parameters.ps1",
+    refreshonly => true,
+    logoutput   => true,
+    provider    => 'powershell',


Is it safe to assume powershell is installed already? even if not technically safe, please start a section for "Experimental Windows Support" in the readme and note this.

solarkennedy · 2017-07-25T15:36:13Z

manifests/install.pp

-          mode  => '0555';
-        "${consul::bin_dir}/consul":
+        "${install_path}/consul-${consul::version}/${binary_name}":
+          owner =>  $consul::user,


can you remove this extra prepending space for style?

solarkennedy · 2017-07-25T15:36:41Z

manifests/install.pp

-        mode   => '0555';
+        owner  => $consul::user,
+        group  => $consul::group,
+        mode   => $binary_mode;


can you use a , instead of ;

solarkennedy · 2017-07-25T15:36:49Z

manifests/install.pp

+        "${install_path}/consul-${consul::version}/${binary_name}":
+          owner =>  $consul::user,
+          group => $consul::group,
+          mode  => $binary_mode;


can you use a , instead of ;

solarkennedy · 2017-07-25T15:36:54Z

manifests/install.pp

          ensure => link,
          notify => $do_notify_service,
-          target => "${install_path}/consul-${consul::version}/consul";
+          target => "${install_path}/consul-${consul::version}/${binary_name}";


can you use a , instead of ;

robklg · 2017-09-11T14:57:48Z

@solarkennedy yeah I understand your reluctance. I do agree however that NSSM is the best option. But it would also be fine to me if the puppet-consul module would only manage the config and not the installation. Then we can do the NSSM part in our profile.

solarkennedy · 2017-09-16T23:13:10Z

I agree that would be a good compromise. For the same reason this puppet module doesn't install curl or unzip (as much as other people want it to), I don't think it is sustainable. If every puppet module that need to use the unzip binary tried to manage that package there would be lots of conflicts.

If the consul module tries to install NSSM, it would conflict with any other puppet module that thinks it should install NSSM too. NSSM should "just" be its own puppet module, puppet-consul should "just" use that or assume it is available.

iwagnerclgx · 2018-01-23T19:15:12Z

Any thoughts on proceeding? I don't mind taking a stab at rebasing @krjensen's PR into the latest master and cleaning up the concerns raised here.

solarkennedy · 2018-01-24T05:54:19Z

Go for it

solarkennedy · 2018-01-27T04:24:56Z

Closed by #403

Kristian Jensen added 2 commits July 11, 2017 14:17

asd

c44f421

Added windows support

231146c

Kristian Jensen added 2 commits July 17, 2017 10:28

Merge https://github.com/solarkennedy/puppet-consul

efe869c

# Conflicts: # manifests/config.pp # manifests/install.pp # manifests/params.pp # manifests/run_service.pp

Simplified logic around init_style for windows

3dd4faf

Kristian Jensen and others added 5 commits July 17, 2017 10:49

Fixed trailing whitespace

0d5f01c

Fixed indentation of =>

91029fc

Fixed linter

1661be3

Fixed =>

e42f9a5

Fixed permission on windows data_dir

f6d9529

Windows needs 775 to create sym-link

solarkennedy reviewed Jul 17, 2017

View reviewed changes

Kristian Jensen and others added 5 commits July 18, 2017 08:21

lowerd binary mode to 755

084412d

Fixed permissions

d9ebd1b

Windows needs 775 when a computer is domain joined

8a95a87

A domain joined machine needs 775, to allow domain users to create files and folders in the install_dir.

Fixed wrong var

b2bae75

Using binary_mode on consul folder

d99ff71

solarkennedy mentioned this pull request Jul 26, 2017

Ensure we dont mess with windows permissions with Unix ones #321

Closed

solarkennedy requested changes Sep 11, 2017

View reviewed changes

iwagnerclgx mentioned this pull request Jan 25, 2018

Added experimental Windows support #403

Merged

solarkennedy closed this Jan 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added windows support #352

Added windows support #352

KZachariassen commented Jul 14, 2017 •

edited by solarkennedy

solarkennedy commented Jul 15, 2017

KZachariassen commented Jul 17, 2017

solarkennedy Jul 17, 2017

KZachariassen Jul 18, 2017

KZachariassen commented Jul 17, 2017

robklg commented Sep 11, 2017

solarkennedy commented Sep 11, 2017

solarkennedy commented Sep 11, 2017

solarkennedy Jul 17, 2017

solarkennedy Jul 25, 2017

solarkennedy Jul 17, 2017

solarkennedy Jul 17, 2017

solarkennedy Jul 17, 2017

solarkennedy Jul 25, 2017

solarkennedy Jul 25, 2017

solarkennedy Jul 25, 2017

solarkennedy Jul 25, 2017

robklg commented Sep 11, 2017

solarkennedy commented Sep 16, 2017

iwagnerclgx commented Jan 23, 2018

solarkennedy commented Jan 24, 2018

solarkennedy commented Jan 27, 2018

Added windows support #352

Added windows support #352

Conversation

KZachariassen commented Jul 14, 2017 • edited by solarkennedy

solarkennedy commented Jul 15, 2017

KZachariassen commented Jul 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KZachariassen commented Jul 17, 2017

robklg commented Sep 11, 2017

solarkennedy commented Sep 11, 2017

solarkennedy commented Sep 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robklg commented Sep 11, 2017

solarkennedy commented Sep 16, 2017

iwagnerclgx commented Jan 23, 2018

solarkennedy commented Jan 24, 2018

solarkennedy commented Jan 27, 2018

KZachariassen commented Jul 14, 2017 •

edited by solarkennedy