Your commit message needs to be a whole lot longer; explaining why you select '*' from the WMI data source would be a good start, especially because you are going to do that for *every* fact you create, fetching an awful lot of redundant data every time.
Thanks for putting this up, though. It is good to see progress on getting this sort of Windows data available in Facter, as a presage to getting better Win32 support in Puppet. On Mon, May 23, 2011 at 23:49, William Van Hevelingen <[email protected]> wrote: > Signed-off-by: William Van Hevelingen <[email protected]> > --- > Local-branch: feature/master/7621-windows_osrelease > lib/facter/operatingsystemrelease.rb | 17 +++++++++++++++++ > lib/facter/util/mswindows.rb | 16 ++++++++++++++++ > lib/facter/windowsrelease.rb | 18 ++++++++++++++++++ > 3 files changed, 51 insertions(+), 0 deletions(-) > create mode 100644 lib/facter/util/mswindows.rb > create mode 100644 lib/facter/windowsrelease.rb > > diff --git a/lib/facter/operatingsystemrelease.rb > b/lib/facter/operatingsystemrelease.rb > index 347fe7f..2ec7b05 100644 > --- a/lib/facter/operatingsystemrelease.rb > +++ b/lib/facter/operatingsystemrelease.rb > @@ -123,5 +123,22 @@ Facter.add(:operatingsystemrelease) do > end > > Facter.add(:operatingsystemrelease) do > + confine :operatingsystem => :windows > + require 'facter/util/mswindows' > + osrelease = "" > > + # Included spaces to ensure its uniqueness > + @releases = [" XP ", " Vista ", " 7 ", " 2003 ", " 2008 ", " 2008 R2 ", " > 8 "] > + > + @releases.each do |r| > + if Facter::Util::MSWindows.getWindowsRelease.include?(r) > + setcode do > + osrelease = r.gsub(/\s/, "") > + end I don't like this very much. I think we should export the unadorned WMI/CIM value, without interpretation. Otherwise we are going to have very strange behaviours *and* a headache: either we don't export an unknown value, or we force ourselves to have an ABI change when we *do* add it to the list. Neither of those is very nice. > + def self.getWindowsRelease > + caption = "" > + self.Win32_OperatingSystem.each { |x| caption = x["Caption"] } > + caption > + end That seems ... odd. It isn't clear why you don't rewrite it as: Win32_OperatingSystem.last["Caption"] ...since that seems to be the effect of the code. Is there some detail I am missing? If so, a comment explaining it would probably be a good idea. :) > + def self.Win32_OperatingSystem > + require 'win32ole' > + wmi = WIN32OLE.connect("winmgmts://") > + query = wmi.ExecQuery("select * from Win32_OperatingSystem") > + query Ruby, and our, style is that we don't have an explicit return; in this case, assigning to a variable and using it on the next line is extra strange. So, best drop 'query' entirely. (Also, that isn't a query, it is a result. ;) Regards, Daniel -- ⎋ Puppet Labs Developer – http://puppetlabs.com ✉ Daniel Pittman <[email protected]> ✆ Contact me via gtalk, email, or phone: +1 (877) 575-9775 ♲ Made with 100 percent post-consumer electrons -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
