Cab_Package: dh/url support cab #5732

dheerajd-msys · 2017-01-18T15:03:40Z

This PR is created to fix issues which I committed on #5700

lamont-granquist · 2017-01-20T01:36:55Z

lib/chef/provider/package/cab.rb

          new_resource.version(package_version)
          current_resource.version(installed_version)
          current_resource
        end

+        def cab_file_source
+          @cab_file_source ||= uri_scheme?(new_resource.source) ? download_source_file : @new_resource.source


tiniest nitpick: use new_resource.source instead of @new_resource.source

lamont-granquist · 2017-01-20T01:38:59Z

spec/unit/provider/package/cab_spec.rb

@@ -18,7 +18,7 @@

 require "spec_helper"

-describe Chef::Provider::Package::Cab do
+describe Chef::Provider::Package::Cab, :windows_only do


unit tests shouldn't need :windows_only -- if you need this it indicates you're probably missing some kind of a stub/mock

I unit tested that on my local without adding filter :windows_only and it ran properly however Travis was failing since it was using linux environment and file-path was mismatched.
See https://travis-ci.org/chef/chef/jobs/192942094#L2093 .

You might consider using platform agnostic paths in the tests like: "#{ENV['TEMP']}/Test6.1-KB2664825-v3-x64.cab" instead of C:\\Temp\\Test6.1-KB2664825-v3-x64.cab.

lamont-granquist · 2017-01-20T01:40:49Z

spec/unit/provider/package/cab_spec.rb

@@ -54,6 +54,7 @@
    allow(provider).to receive(:dism_command).with("/Get-Packages").and_return(installed_package_list_obj)
    package_version_obj = double(stdout: package_version_stdout)
    allow(provider).to receive(:dism_command).with("/Get-PackageInfo /PackagePath:\"#{new_resource.source}\"").and_return(package_version_obj)
+    allow_any_instance_of(Chef::Provider::Package::Cab).to receive(:dism_command).with("/Get-PackageInfo /PackagePath:\"#{new_resource.source}\"").and_return(package_version_obj)


allow_any_instance_of is usually codesmell for tests -- can this just be allow(provider).to receive....? although that seems to be the prior line.... i'm somewhat confused as to why this is necessary...

Thanks @lamont-granquist . I will remove this last line.

lamont-granquist · 2017-01-20T01:42:18Z

spec/unit/provider/package/cab_spec.rb

+    end
+
+    it "returns a clean cache path where the cab file is downloaded" do
+      allow(Chef::FileCache).to receive(:create_cache_path).and_return("C:\\chef\\abc\\package")


should probably expect instead of allow here

Thanks @lamont-granquist . Replacing allow with expect works.

mwrock

This looks great! Before merging I would attend to @lamont-granquist 's comments

Signed-off-by: dheerajd-msys <dheeraj.dubey@msystechnologies.com>

btm · 2017-01-27T13:35:22Z

@mwrock @lamont-granquist re-review?

NimishaS · 2017-01-25T07:04:24Z

spec/unit/provider/package/cab_spec.rb

@@ -126,6 +127,49 @@ def allow_get_packages
    end
  end

+  describe "#source_resource" do
+    before do
+      cab_file = "C:\\Temp\\Test6.1-KB2664825-v3-x64.cab"


source_resource method is called for URLs. I am a bit doubtful if cab_file should be given a local path here.

NimishaS · 2017-01-27T13:38:30Z

spec/unit/provider/package/cab_spec.rb

+      allow(provider).to receive(:cab_file_source).and_return(new_resource.source)
+      provider.load_current_resource
+      if windows?
+        expect(provider.cab_file_source).to be == File.join("#{ENV['TEMP'].downcase}", "\\", "test6.1-kb2664825-v3-x64.cab")


cab_file_source is stubbed at line#164 and then we are expecting it's value. This spec needs modification.

NimishaS · 2017-01-27T13:40:24Z

spec/unit/provider/package/cab_spec.rb

+    it "returns local cab file source path if same is set" do
+      new_resource.source = File.join("#{ENV['TEMP']}", "test6.1-kb2664825-v3-x64.cab")
+      allow(provider).to receive(:cab_file_source).and_return(new_resource.source)
+      provider.load_current_resource


Please check if it's required to call load_current_resource here? Can we directly call cab_file_source method since the spec belongs to that method?

NimishaS · 2017-01-27T13:41:38Z

spec/unit/provider/package/cab_spec.rb

+      else
+        expect(provider).to receive(:download_source_file).and_return(File.join("#{ENV['TEMP']}", "test6.1-kb2664825-v3-x64.cab"))
+      end
+      provider.load_current_resource


The spec is for cab_file_source method. It should ideally be verifying the output of that method only. Please check if load_current_resource needs to be called.

Signed-off-by: dheerajd-msys <dheeraj.dubey@msystechnologies.com>

btm mentioned this pull request Jan 20, 2017

Added URL support for cab_package #5700

Closed

btm requested review from lamont-granquist and mwrock January 20, 2017 01:35

btm added Enhancement labels Jan 20, 2017

lamont-granquist reviewed Jan 20, 2017

View reviewed changes

mwrock requested changes Jan 20, 2017

View reviewed changes

thommay added Type: Enhancement Adds new functionality. and removed Enhancement labels Jan 25, 2017

piyushawasthi force-pushed the dh/url_support_cab branch 5 times, most recently from 27d9953 to dcdc183 Compare January 27, 2017 11:46

dheerajd-msys added 8 commits January 27, 2017 18:16

Added URL support for cab_package

cb2bfab

Signed-off-by: dheerajd-msys <dheeraj.dubey@msystechnologies.com>

specs are added and code refactored

dde1b81

Signed-off-by: dheerajd-msys <dheeraj.dubey@msystechnologies.com>

specs are added and code refactored

f8a5faf

Signed-off-by: dheerajd-msys <dheeraj.dubey@msystechnologies.com>

specs are added and code refactored

d0d68eb

Signed-off-by: dheerajd-msys <dheeraj.dubey@msystechnologies.com>

specs are added and code refactored

22c164c

Signed-off-by: dheerajd-msys <dheeraj.dubey@msystechnologies.com>

specs are added and code refactored

a3c8b93

Signed-off-by: dheerajd-msys <dheeraj.dubey@msystechnologies.com>

removed bundle/config

ca2f7cc

Signed-off-by: dheerajd-msys <dheeraj.dubey@msystechnologies.com>

Fixed review comments

0cb3aba

Signed-off-by: dheerajd-msys <dheeraj.dubey@msystechnologies.com>

piyushawasthi force-pushed the dh/url_support_cab branch from dcdc183 to 0cb3aba Compare January 27, 2017 12:47

btm approved these changes Jan 27, 2017

View reviewed changes

NimishaS reviewed Jan 27, 2017

View reviewed changes

Fixed review comments on #cab_file_source

aa12124

Signed-off-by: dheerajd-msys <dheeraj.dubey@msystechnologies.com>

comments and context added for #cab_file_source

558759b

Signed-off-by: dheerajd-msys <dheeraj.dubey@msystechnologies.com>

dheerajd-msys force-pushed the dh/url_support_cab branch from 16b6c61 to 558759b Compare January 30, 2017 13:13

mwrock approved these changes Jan 30, 2017

View reviewed changes

mwrock merged commit 3862a7d into chef:master Jan 30, 2017

btm mentioned this pull request Jan 31, 2017

Doc update for URL support in cab_package chef/chef-web-docs#432

Merged

lamont-granquist changed the title ~~dh/url support cab~~ Cab_Package: dh/url support cab Feb 17, 2017

thommay added Area: Windows and removed Windows labels Sep 22, 2017

chef locked and limited conversation to collaborators Nov 16, 2017

tas50 added Platform: Windows and removed Area: Windows labels Jan 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cab_Package: dh/url support cab #5732

Cab_Package: dh/url support cab #5732

dheerajd-msys commented Jan 18, 2017 •

edited

Loading

lamont-granquist Jan 20, 2017

lamont-granquist Jan 20, 2017

dheerajd-msys Jan 20, 2017

mwrock Jan 20, 2017

lamont-granquist Jan 20, 2017

dheerajd-msys Jan 20, 2017

lamont-granquist Jan 20, 2017

dheerajd-msys Jan 20, 2017

mwrock left a comment

btm commented Jan 27, 2017

NimishaS Jan 25, 2017

NimishaS Jan 27, 2017

NimishaS Jan 27, 2017

NimishaS Jan 27, 2017

Cab_Package: dh/url support cab #5732

Cab_Package: dh/url support cab #5732

Conversation

dheerajd-msys commented Jan 18, 2017 • edited Loading

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

mwrock left a comment

Choose a reason for hiding this comment

btm commented Jan 27, 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

dheerajd-msys commented Jan 18, 2017 •

edited

Loading