add some big FIXMEs

the windows package provider is doing some very strange, and very likely
very buggy things.

i don't have the time or the knowledge right now to unwind these though.

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

1 files changed, 17 insertions, 1 deletions

diff --git a/lib/chef/provider/package/windows.rb b/lib/chef/provider/package/windows.rb
index f011ebf909..85e284faa6 100644
--- a/lib/chef/provider/package/windows.rb
+++ b/lib/chef/provider/package/windows.rb
@@ -1,6 +1,6 @@
 #
 # Author:: Bryan McLellan <btm@loftninjas.org>
-# Copyright:: Copyright 2014-2016, Chef Software, Inc.
+# Copyright:: Copyright 2014-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -54,6 +54,10 @@ class Chef
           @current_resource = Chef::Resource::WindowsPackage.new(new_resource.name)
           if downloadable_file_missing?
             logger.trace("We do not know the version of #{new_resource.source} because the file is not downloaded")
+            # FIXME: this label should not be used.  It could be set to nil.  Probably what should happen is that
+            # if the file hasn't been downloaded then load_current_resource must download the file here, and then
+            # the else clause to set current_resource.version can always be run.  Relying on a side-effect here
+            # produces at least less readable code, if not outright buggy...
             current_resource.version(:unknown.to_s)
           else
             current_resource.version(package_provider.installed_version)
@@ -164,6 +168,18 @@ class Chef
         # this package provider does not support package arrays
         # However, There may be multiple versions for a single
         # package so the first element may be a nested array
+        #
+        # FIXME: this breaks the semantics of the superclass and needs to get unwound.  Since these package
+        # providers don't support multipackage they can't put multiple versions into this array.  The windows
+        # package managers need this in order to uninstall multiple installed version, and they should track
+        # that in something like an `uninstall_version_array` of their own.  The superclass does not implement
+        # this kind of feature.  Doing this here breaks LSP and will create bugs since the superclass will not
+        # expect it at all.  The `current_resource.version` also MUST NOT be an array if the package provider
+        # is not multipackage.  The existing implementation of package_provider.installed_version should probably
+        # be what `uninstall_version_array` is, and then that list should be sorted and last/first'd into the
+        # current_resource.version.  The current_version_array method was not intended to be overwritten by
+        # sublasses (but ruby provides no feature to block doing so -- it is already marked as private).
+        #
         def current_version_array
           [ current_resource.version ]
         end