non msi packages must explicitly provide a source attribute on install #4277

mwrock · 2015-12-11T00:46:18Z

Currently one can use a package source location as a package name:

package 'http://mercurial.selenic.com/release/windows/Mercurial-3.6.1-x64.exe' do
  checksum 'febd29578cb6736163d232708b834a2ddd119aa40abc536b2c313fc5e1b5831d'
end

In the case of non-:msi installer_types, this will result in package resources reconverging an :install action even though the package has been previously installed.

The reason is that when the windows_package scans the registry for all current software installations, it cannot search by install path or url. So it searches by the DisplayName property in the registry keys mentioned above. If you use a source path as the name of a non :msi windows_package it will not find any currently installed package and will therefore attempt to reinstall the package. :msi packages get around this because there is a well known API for extracting the id of the installed MSI package from a .msi file.

Requiring an explicit source for non :msi packages will reduce confusion here around idempotency.

chefsalim · 2015-12-11T00:51:33Z

👍

btm · 2015-12-11T20:09:57Z

def uninstall_registry_entries
  @uninstall_registry_entries ||= Chef::Provider::Package::Windows::RegistryUninstallEntry.find_entries(new_resource.name)
end

I think this should look at new_resource.package_name so that the name can be something less crazy, e.g.

package "remove the burning foxes" do
  package_name "FireFox Internet Browser of Webs v11.2.0 sp12"
  action :uninstall
end

It'd actually be way more intuitive if this was registry_name or registry_key or something. hrm.

btm · 2015-12-11T20:23:34Z

DOC_CHANGES.md

@@ -83,11 +83,13 @@ to better align with other ssh related options.

 `windows_package` now supports more than just `MSI`. Most common windows installer types are supported including Inno Setup, Nullsoft, Wise and InstallShield. The new allowed `installer_type` values are: `inno`, `nsis`, `wise`, `installshield`, `custom`, and `msi`.

+Non `:msi` package names should match the softwares `DisplayName` property expected to be found in the registry keys mentioned below. Further, they must explicitly include a package `source` attribute when using the `:install` action. It will not default to the package name if missing. Without the name matching the software display name, non `:msi` packages will always reconverge on `:install`.


DisplayName is a registry key property, not a chef resource property. we should be more clear about that. It's not that we don't say that, but DisplayName sticks out (besides being camel case) to me as something I might set on my resource.

For proper behavior, the package_name property for Windows packages other than MSIs must be set to match ...

On second thought, we should remove/combine this paragraph with the one below that says almost the same thing, lists the registry key locations, but also mentions that the add/remove name might be more accessible.

mwrock · 2015-12-11T21:00:41Z

One thing I noticed just a bit ago is that the cookbook does require an explicit source and does not defaut a missing source to be the name. That was one of the slight differences in the resource that did not copy over. So this PR should better mimic what we had in the cookbook.

mwrock · 2015-12-12T01:39:58Z

I've adjusted to review comments.

btm · 2015-12-12T04:27:19Z

lib/chef/provider/package/windows.rb

@@ -34,6 +34,13 @@ class Windows < Chef::Provider::Package

        require 'chef/provider/package/windows/registry_uninstall_entry.rb'

+        def define_resource_requirements
+          requirements.assert(:install) do |a|
+            a.assertion { @new_resource.source unless package_provider == :msi }


Use the new_resource accessor rather than the instance variable.

btm · 2015-12-12T04:33:43Z

👍 check the use of instance variables vs accessors, otherwise 🏆

mwrock · 2015-12-12T05:25:48Z

argh well thats embarassing. should have caught those. consider them smitten

btm · 2015-12-14T23:45:20Z

👍

chefsalim · 2015-12-15T19:21:16Z

lib/chef/provider/package/windows.rb

+        def define_resource_requirements
+          requirements.assert(:install) do |a|
+            a.assertion { new_resource.source unless package_provider == :msi }
+            a.failure_message Chef::Exceptions::NoWindowsPackageSource, "Source for package #{new_resource.name} must be specified in the resource's source property for package to be installed because the package_name property is used to test for package installation state for this package type."


This message could probably be shortened :)
Also, I think the sentence is missing a 'the' in "source property for package"

Yeah, it's a little tough. MSI doesn't require source, so I think this is an attempt at proactive documentation.

Not seeing the missing "the" but to @btm's point, this is tough to get just right and make it concise. We could just say that the source is required and leave it at that but if the user uses a url/path as the package_name, then they might be confused why they need to specify a source which they don't have to do for MSIs.

I guess you could add a "the" here:

"Source for package #{new_resource.name} must be specified in the resource's source property for package to be installed because the package_name property is used to test for the package installation state for this package type."

chefsalim · 2015-12-15T19:50:26Z

👍 again

chefsalim · 2015-12-15T21:09:39Z

Also Matt, you may need to wait to update the DOC changes for the required source, as those will be for 12.7

mwrock · 2015-12-15T21:11:09Z

do we do doc changes for patch releases?

chefsalim · 2015-12-15T21:19:14Z

Per James, we should update the doc_changes if the change is significant enough.
(Note: the process may change for 12.7)

mwrock · 2015-12-16T00:24:23Z

There was some merge conflict fallout here I had to address with the properties changes. There is still one app veyor test failing but it is related to easy_install and completely unrelated to this PR. So merging.

non msi packages must explicitly provide a source attribute on install

mwrock changed the title ~~add idempotency note regarding non msi installs to doc changes~~ non msi packages must explicitly provide a source attribute on install Dec 11, 2015

btm reviewed Dec 11, 2015
View reviewed changes

btm reviewed Dec 12, 2015
View reviewed changes

mwrock closed this Dec 12, 2015

mwrock reopened this Dec 12, 2015

smurawski added the Ready for Merge label Dec 15, 2015

lamont-granquist added this to the Accepted Minor milestone Dec 15, 2015

chefsalim reviewed Dec 15, 2015
View reviewed changes

mwrock force-pushed the mwrock/package branch from 5b2afe3 to f99c222 Compare December 15, 2015 20:13

non msi packages must explicitly provide a source attribute on install

09df8ee

merge cleanup

41bd0ba

mwrock force-pushed the mwrock/package branch from f99c222 to 41bd0ba Compare December 15, 2015 21:26

fixing post merge test breaks

6e1caca

mwrock added a commit that referenced this pull request Dec 16, 2015

Merge pull request #4277 from chef/mwrock/package

7677d4b

non msi packages must explicitly provide a source attribute on install

mwrock merged commit 7677d4b into master Dec 16, 2015

mwrock deleted the mwrock/package branch December 16, 2015 00:24

chef-supermarket added the Signed CLA label May 25, 2016

chef-supermarket assigned sethvargo May 25, 2016

thommay added Status: Ready for Merge and removed Ready for Merge labels Jan 25, 2017

chef locked and limited conversation to collaborators Nov 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

non msi packages must explicitly provide a source attribute on install #4277

non msi packages must explicitly provide a source attribute on install #4277

mwrock commented Dec 11, 2015

chefsalim commented Dec 11, 2015

btm commented Dec 11, 2015

btm Dec 11, 2015

mwrock commented Dec 11, 2015

mwrock commented Dec 12, 2015

btm Dec 12, 2015

btm commented Dec 12, 2015

mwrock commented Dec 12, 2015

btm commented Dec 14, 2015

chefsalim Dec 15, 2015

btm Dec 15, 2015

mwrock Dec 15, 2015

btm Dec 15, 2015

chefsalim commented Dec 15, 2015

chefsalim commented Dec 15, 2015

mwrock commented Dec 15, 2015

chefsalim commented Dec 15, 2015

mwrock commented Dec 16, 2015

		@@ -83,11 +83,13 @@ to better align with other ssh related options.

		`windows_package` now supports more than just `MSI`. Most common windows installer types are supported including Inno Setup, Nullsoft, Wise and InstallShield. The new allowed `installer_type` values are: `inno`, `nsis`, `wise`, `installshield`, `custom`, and `msi`.

		Non `:msi` package names should match the softwares `DisplayName` property expected to be found in the registry keys mentioned below. Further, they must explicitly include a package `source` attribute when using the `:install` action. It will not default to the package name if missing. Without the name matching the software display name, non `:msi` packages will always reconverge on `:install`.

non msi packages must explicitly provide a source attribute on install #4277

non msi packages must explicitly provide a source attribute on install #4277

Conversation

mwrock commented Dec 11, 2015

chefsalim commented Dec 11, 2015

btm commented Dec 11, 2015

btm Dec 11, 2015

Choose a reason for hiding this comment

mwrock commented Dec 11, 2015

mwrock commented Dec 12, 2015

btm Dec 12, 2015

Choose a reason for hiding this comment

btm commented Dec 12, 2015

mwrock commented Dec 12, 2015

btm commented Dec 14, 2015

chefsalim Dec 15, 2015

Choose a reason for hiding this comment

btm Dec 15, 2015

Choose a reason for hiding this comment

mwrock Dec 15, 2015

Choose a reason for hiding this comment

btm Dec 15, 2015

Choose a reason for hiding this comment

chefsalim commented Dec 15, 2015

chefsalim commented Dec 15, 2015

mwrock commented Dec 15, 2015

chefsalim commented Dec 15, 2015

mwrock commented Dec 16, 2015