gocd / go-cookbook

Cookbook that installs and configures the open-source ThoughtWorks Studios GoCD product
http://www.gocd.org/
Apache License 2.0
44 stars 66 forks source link

#63 #67 improve version handling and installing on windows #75

Closed tomzo closed 8 years ago

tomzo commented 8 years ago

Few things here

  1. The way attributes were constructed from one another - it did not work in many cases. Leading to paths, filenames, urls ending up with something like go-agent--setup.exe. I replaced those by library methods. User can still overide versions and urls as before. But it is more bulletproof now.
  2. Changed windows_package and package resources into simple execute with proper nsis arguments - /s to be silent. The windows package resources just don't work too well on windows (meaning I had a few silly errors when trying out the windows recipe @ketan recently added). At least not in all chef version. I think that extra chef-o-cracy is just breaking the cookbook instead of being useful. execute will work, all the time.
  3. I used the updates service #63. On windows, it makes perfect sense to call service to find out what is latest stable.
  4. A few improvements on package names and version handling to make it feel better. Now user can also set node['gocd']['version'] = 'stable' or node['gocd']['version'] = 'experimental' which will be resolved with updates service at compile time. On windows this is resolved always.

All this is unit tested only at this point, so please don't merge yet, but please have a look and review. I'll test it within next few days as I am working on windows now.

Windows agent LWRP will be in another PR. Once I test these changes a bit.

ketan commented 8 years ago

Here's what I'd want as a user of the cookbook (wrt installing GoCD)

I'll push another commit on top of this commit to see what you think.

tomzo commented 8 years ago

All the points you said about version are true, except for handling upgrades.

For apt and yum installations setting up handling upgrades would be more or less easy. Just choose action :upgrade instead of :install in package resource.

How would you image it when there is no repository? In windows that is the only case. How would we get currently installed version? Can we count on some Go binary or script to print out version?

tomzo commented 8 years ago

Ah. I just noticed you wanted separate channel attribute. That could work too. But still, the upgrading part is a mistery to me.

ketan commented 8 years ago

package, rpm_package and deb_package (and optionally windows_package, if we go that route) support about the same attributes, so upgrading things is going to be trivial.

As far as non repository stuff is concerned, all bets are off, users should be expected to specify the full package url and/or version, as needed. That make sense?

tomzo commented 8 years ago

As far as non repository stuff is concerned, all bets are off, users should be expected to specify the full package url and/or version, as needed

OK. But then windows package source is non-repository. So you expect that when user on windows needs an upgrade then he must update package url (directly or indirectly by changing version/channel). Then provisioning on machine where Go is installed already must somehow determine the version that was already installed to tell if package should be installed again or not. windows_package does not magically take care of this, just because it has version attribute.

ketan commented 8 years ago

You are right about the version attribute support in windows_package resource in chef 12.6.

This documentation from the windows cookbook (https://github.com/chef-cookbooks/windows/blob/master/README.md#windows_package) seems to suggest otherwise are you suggesting it is a bug?

tomzo commented 8 years ago

I tried it yesterday on chef 12.4.2 - the whole resource failed with error that it cannot determine installer type. it might have been something I did wrong. I don't know. I'll try again today.

Nonetheless I had a bunch of issues with chef, windows and using version attribute and windows package resource. Now when they finally removed it, we must maintain 2 blocks for 12.6+ or below. While actual command is trivial thanks to nsis. That is was why I wanted to remove it.

ketan commented 8 years ago

I tried it yesterday on chef 12.4.2 - the whole resource failed with error that it cannot determine installer type. it might have been something I did wrong. I don't know. I'll try again today.

If you're seeing this error, I had to upgrade to 12.7 nightly as suggested in the last comment. I think 12.7.2 was out recently, and I've not tested with that version. I recall that windows_package (via windows cookbook) was working fine on 12.4.3 (which is the version I think we use on windows) I'll need to just check it once tomorrow when I'm back to work.

I'm all for removing dependency on windows_package if it's simple to maintain. But easy upgradability of (at-least) the go-agent package is something we may need to consider.

ketan commented 8 years ago

I'm all for removing dependency on windows_package if it's simple to maintain. But easy upgradability of (at-least) the go-agent package is something we may need to consider.

Also — that's not something we need to do right away, but I do think we need an answer to the problem, even if we don't solve it immediately.

ketan commented 8 years ago

@tomzo — I pushed a commit here, https://github.com/ketan/go-cookbook/commit/48b7e04e0a6456ae82db5312444c1261321e73a6 (one test is failing, I can look at it if you are ok with the general idea of the PR)

I've left windows as-is, and just addressed upgrades on linux when using package repos.

tomzo commented 8 years ago

@ketan I like it, but youl'll have to restrain from using if node[... in attributes/default.rb as I have written there. That must be moved to library methods so that it will work in wrapper cookbooks.

tomzo commented 8 years ago

I reverted my windows changes, I managed to catch the error I previously encountered:

Starting Chef Client, version 12.4.2
       Compiling Cookbooks...
       [2016-04-19T17:45:28-07:00] WARN: Go server not found on Chef server or not specifed via node['gocd']['agent']['go_server_host'] attribute, defaulting Go server to 127.0.0.1
       Converging 9 resources
       Recipe: gocd::server_windows_install

           - create new file C:\Users\ADMINI~1\AppData\Local\Temp\kitchen\cache/go-server-stable-setup.exe
           - update content in file C:\Users\ADMINI~1\AppData\Local\Temp\kitchen\cache/go-server-stable-setup.exe from none to f01b1a
           (file sizes exceed 10000000 bytes, diff output suppressed)

           ================================================================================
           Error executing action `install` on resource 'windows_package[Go Server]'
           ================================================================================

           ArgumentError
           -------------
           Installer type for Windows Package 'Go Server' not specified and cannot be determined from file extension 'exe'

           Resource Declaration:
           ---------------------
           # In C:\Users\ADMINI~1\AppData\Local\Temp\kitchen\cookbooks\gocd\recipes\server_windows_install.rb

            13:   package 'Go Server' do
            14:     source package_path
            15:     options opts.join(" ")
            16:   end
            17: else

           Compiled Resource:
           ------------------
           # Declared in C:\Users\ADMINI~1\AppData\Local\Temp\kitchen\cookbooks\gocd\recipes\server_windows_install.rb:13:in `from_file'

           windows_package("Go Server") do
             action [:install]
             retries 0
             retry_delay 2
             default_guard_interpreter :default
             options "/S /D=C:\\GoServer"
             package_name "Go Server"
             source "c:\\users\\admini~1\\appdata\\local\\temp\\kitchen\\cache\\go-server-stable-setup.exe"
             timeout 600
             returns [0]
             declared_type :package
             cookbook_name :gocd
             recipe_name "server_windows_install"
           end
tomzo commented 8 years ago

When I specified windows installer type explicitly with

  package 'Go Server' do
    installer_type :nsis
    source package_path
    options opts.join(" ")
  end

then error is

================================================================================
           Error executing action `install` on resource 'windows_package[Go Server]'
           ================================================================================

           RuntimeError
           ------------
           Unable to find a Chef::Provider::Package::Windows provider for installer_type 'nsis'

           Resource Declaration:
           ---------------------
           # In C:\Users\ADMINI~1\AppData\Local\Temp\kitchen\cookbooks\gocd\recipes\server_windows_install.rb

            13:   package 'Go Server' do
            14:     installer_type :nsis
            15:     source package_path
            16:     options opts.join(" ")
            17:   end
            18: else
tomzo commented 8 years ago

Executed on chef 12.9.38 there is no errors. So obviously this was simply broken in 12.4.2

tomzo commented 8 years ago

Since last time we spoke

I did nothing to improve support of actually upgrading windows agents. I think this should be addressed in other PR.

Tests are passing, I also provisioned windows server + agent with no problem on chef 12.9.38

@ketan let me know if you are OK with this.

ketan commented 8 years ago

I'm ok with this PR. Haven't seen the README, but I'd think it's probably time to update it.

Few observations -

ketan commented 8 years ago

Should we squash and merge (since most of this commit is noise wrt windows)?

tomzo commented 8 years ago

APT package_options are not needed anymore since all binaries are signed, even older ones and experimental builds do you recall why unzip package is need by the server on linux?

I removed apt options. But I don't recall why unzip is needed. Doesn't server need it?

ketan commented 8 years ago

But I don't recall why unzip is needed. Doesn't server need it?

I don't see why it should, but apparently the debian package requires it. Can't see from old history why it's needed. Maybe @arvindsv knows?

tomzo commented 8 years ago

my bet is that one of package scripts (like post-inst) unzips something