pip resource: support non-default pip locations, such as virtualenvs #2097

tonybaloney · 2017-08-24T08:39:29Z

Fixes #516

Implements optional path to pip executable (like when virtualenv is used) instead of using system PATH.
Tested by installing newer version of Django in virtualenv and testing for it

describe pip('django', '/Users/anthonyshaw/repo/wayfinder/wayfinder/bin/pip') do
  it { should be_installed }
  its('version') { should eq('1.11.4')}
end
describe port(80) do
  it { should_not be_listening }
end

Before:

somepersonslaptop:inspec anthonyshaw$ bundle exec bin/inspec exec test.rb

Profile: tests from test.rb
Version: (not specified)
Target:  local://


  Pip Package
     ∅  django version should eq "1.11.4"

     expected: "1.11.4"
          got: nil

     (compared using ==)

  Port 80
     ✔  should not be listening

Test Summary: 1 successful, 2 failures, 0 skipped

After

somepersonslaptop:inspec anthonyshaw$ bundle exec bin/inspec exec test.rb

Profile: tests from test.rb
Version: (not specified)
Target:  local://


  Pip Package
     ✔  django should be installed
     ✔  django version should eq "1.11.4"
  Port 80
     ✔  should not be listening

Test Summary: 3 successful, 0 failures, 0 skipped

tonybaloney · 2017-08-24T08:42:41Z

sorry. Forgot DCO. I'll amend the commit

Signed-off-by: Anthony Shaw <[email protected]>

srenatus

Looks good. I've got some inline comments, also, an added test case would be awesome, too (https://github.com/chef/inspec/blob/master/test/unit/resources/pip_test.rb)

srenatus · 2017-08-24T08:43:49Z

lib/resources/pip.rb

    "

-    def initialize(package_name)
+    def initialize(package_name, virtualenv_path=nil)


[nit] I think virtualenv_path is a bit confusing here: From looking at this, I'd expect that I was to put /path/to/virtualenv in there, not /path/to/virtualenv/bin/pip. No idea what's the best approach, though.

agreed. pip_path would make sense. OR having it test for the existence of @virtualenv_path + /bin/pip before executing.

srenatus · 2017-08-24T08:47:18Z

lib/resources/pip.rb

@@ -75,7 +81,7 @@ def pip_cmd
          return nil
        end
      end
-      pipcmd || 'pip'
+      pipcmd = @virtualenv_path.nil? ? 'pip' : @virtualenv_path


[nit] I think to keep the logic that was there before -- this would maybe better be something like this:

pipcmd || @virtualenv_path || 'pip'

So, the order of precedence would be: if the pip is found (windows), take that. Otherwise, take @virtualenv_path; fall back to 'pip'.

Makes me wonder if this shouldn't be like this, then:

@virtualenv_path || pipcmd || 'pip'

@srenatus I updated it. let me know if the new one makes more sense? I'm new to this so feel free to [nit]

srenatus · 2017-08-24T08:54:30Z

lib/resources/pip.rb

@@ -75,7 +81,7 @@ def pip_cmd
          return nil
        end
      end
-      pipcmd || 'pip'
+      @virtualenv_path.nil? ? pipcmd || 'pip' : @virtualenv_path


So, if @virtualenv_path is nil, @virtualenv_path.nil? is true. However, @virtualenv_path will also be understood as "falsy" by ruby, so that line is equivalent to

@virtualenv_path || pipcmd || 'pip'

Signed-off-by: Anthony Shaw <[email protected]>

tonybaloney · 2017-08-24T10:05:53Z

@srenatus decided to solve both problems. it now works with a directory (and adds /bin/pip to the path) and then the flow through logic is cleaner as well

describe pip('django') do
  it { should be_installed }
  its('version') { should eq('1.10.5')}
end
describe pip('django', '/Users/anthonyshaw/repo/wayfinder/wayfinder/') do
  it { should be_installed }
  its('version') { should eq('1.11.4')}
end
describe pip('django', '/Users/anthonyshaw/repo/wayfinder/wayfinder/bin/pip') do
  it { should be_installed }
  its('version') { should eq('1.11.4')}
end

Gives

somepersonslaptop:inspec anthonyshaw$ bundle exec bin/inspec exec test.rb

Profile: tests from test.rb
Version: (not specified)
Target:  local://


  Pip Package
     ✔  django should be installed
     ✔  django version should eq "1.10.5"
  Pip Package
     ✔  django should be installed
     ✔  django version should eq "1.11.4"
  Pip Package
     ✔  django should be installed
     ✔  django version should eq "1.11.4"

Test Summary: 6 successful, 0 failures, 0 skipped

srenatus · 2017-08-24T10:34:12Z

@tonybaloney I'm not one of @chef/inspec-maintainers, but I'm sure they'd appreciate an added testcase or two? 😉

adamleff

@tonybaloney welcome to the InSpec community and congratulations on your first contribution!

I've left you some suggestions in an effort to streamline the resource and provide better error messaging to our users.

As @srenatus mentioned, we should definitely add a test for when a user provides a custom pip path. The process would look something like this:

Add a test that invokes a resource with a custom pip path - maybe test for django this time instead of jinja2 currently tested for? It would go around here: https://github.com/chef/inspec/blob/master/test/unit/resources/pip_test.rb#L14
Add a mock for the non-standard pip location (see https://github.com/chef/inspec/blob/master/test/helper.rb#L205 for an example)
Add the mock data for the non-standard pip call. Make it different than the existing pip mock so we're 100% certain the right thing is being tested (see https://github.com/chef/inspec/blob/master/test/unit/mock/cmd/pip-show-jinja2 for an example, maybe name it "non-standard-pip-show-django"

I'd love to help you with the tests if you have any questions or concerns about them. Just let me know.

Again, THANK YOU for contributing to InSpec! It's great to have you.

adamleff · 2017-08-24T11:53:49Z

lib/resources/pip.rb

    "

-    def initialize(package_name)
+    def initialize(package_name, virtualenv_path = nil)


Let's change this variable to pip_path since you can technically install pip in a "non-PATH" location without using a virtualenv, and the examples given are actually paths to an actual pip command, not the top-level directory of the virtualenv.

adamleff · 2017-08-24T11:54:05Z

lib/resources/pip.rb

      @package_name = package_name
+      @virtualenv_path = virtualenv_path


See above for my comment re: virtualenv_path. Instead, I think we should set an instance variable that contains the validated path to pip, and change the #pip_cmd method name to be clearer... maybe resolve_pip_path?

So, assuming you take my advice to change the argument name to pip_path above, it would look something like this:

@pip_cmd = resolve_pip_path(pip_path) return skip_resource "pip not found" if @pip_cmd.nil?

... and then resolve_pip_path would change a bit to support that. Does that make sense?

adamleff · 2017-08-24T11:57:35Z

lib/resources/pip.rb

@@ -57,7 +64,14 @@ def to_s
    def pip_cmd
      # Pip is not on the default path for Windows, therefore we do some logic
      # to find the binary on Windows
-      if inspec.os.windows?
+      if !@virtualenv_path.nil?


This is a real good first attempt, @tonybaloney! I think it's a better user experience to just assume the user is going to give us a path to pip directory rather than adding logic to test if the path is a dir and then assume it's a virtualenv.

How about logic like this:

- if user gave us a pip path, verify it exists, return nil if it doesn't, otherwise return what they gave us - if on windows, run the powershell to find the full pip path. Return nil if we can't find anything - Otherwise, assume it's just "pip", check to see if it exists with command('pip').exist?... return 'pip' if it does, return nil if it doesn't

Then, in initialize, we can call skip_resource if pip_cmd returns nil with helpful error messaging.

Signed-off-by: Anthony Shaw <[email protected]>

tonybaloney · 2017-08-27T22:07:19Z

@adamleff those changes have all been done. However the tests are failing, I think it's because it's checking for the existence of either these fake files and, the pip command which probably isn't on the Docker image. Or I may have made some other mistake

Signed-off-by: Adam Leff <[email protected]>

adamleff · 2017-08-29T03:28:58Z

@tonybaloney thanks for your patience! I just pushed up a commit to your branch to do a bit of refactoring in order to solve your unit test dilemma.

Here's what happened... the logic in resolve_pip_path was correct, but because you didn't mock out the internal commands InSpec would run to see if even the default pip command would exist in the PATH, the @pip_cmd variable had nil. This cause your pip show mocks to never run because instead of calling pip show django, InSpec was actually calling show django

The way I found this was to use the m gem/command to run a subset of the unit tests... the test output can be super noisy, so getting only the output from a test/file I care about is helpful. You can run these yourself like this:

bundle install
bundle exec m test/unit/resources/pip_test.rb

... this will run only the unit tests located in pip_test.rb - that's how I saw the "command not mocked" output that led me to that discovery.

Please take a look at the commit I sent up and let me know if you have any questions. Basically, I moved some of the default handling back up to initialize and moved any handling of an unfound command to only the skip_resource call. That way, the unit tests will still have a @pip_cmd of pip even if none is available.

If it looks good to you, and you're OK with it, I'll give this another review and we can work on getting this merged.

Thank you again for your help with this pull request!

Signed-off-by: Anthony Shaw <[email protected]>

tonybaloney · 2017-08-29T04:40:32Z

all makes sense. I'll do some more reading before the next change
all ok to merge 👍

adamleff

Super work, @tonybaloney! Thank you for your contribution, and congratulations on your first pull request to InSpec! Welcome to the community 🙂

Once another maintainer reviews and approves, we'll merge this in!

arlimus

Amazing work @tonybaloney thank you so much!! 😁

arlimus · 2017-08-30T20:05:02Z

And kudos @adamleff and @srenatus for all your help with the review!!

#2459) Signed-off-by: Markus Grobelin <[email protected]>

tonybaloney requested a review from a team as a code owner August 24, 2017 08:39

Update pip resource for #516 allow user to set path to pip executable

ab6f63d

Signed-off-by: Anthony Shaw <[email protected]>

srenatus reviewed Aug 24, 2017

View reviewed changes

support virtualenv path, pip file exec and better logic

126213a

Signed-off-by: Anthony Shaw <[email protected]>

adamleff changed the title ~~Update pip resource for #516 allow user to set path to pip executable~~ pip resource: support non-default pip locations, such as virtualenvs Aug 24, 2017

adamleff added the Type: Enhancement Improves an existing feature label Aug 24, 2017

adamleff suggested changes Aug 24, 2017

View reviewed changes

tonybaloney added 6 commits August 27, 2017 19:23

add tests for the change to the pip path and resource

2935ccb

Signed-off-by: Anthony Shaw <[email protected]>

tests are case sensitive, although command line is not

0e2f3bb

Signed-off-by: Anthony Shaw <[email protected]>

use a path verification method instead of a class method

7e20758

Signed-off-by: Anthony Shaw <[email protected]>

use guard clauses instead of conditionals

8aa75eb

Signed-off-by: Anthony Shaw <[email protected]>

change the control flow to return nil when commands are not available

1bc4342

Signed-off-by: Anthony Shaw <[email protected]>

fix the return values when custom pip path is not valid

100997a

Signed-off-by: Anthony Shaw <[email protected]>

Refactor pip path detection to fix unit tests

fff3c23

Signed-off-by: Adam Leff <[email protected]>

Merge branch 'master' into pip_516

c1a46a9

Signed-off-by: Anthony Shaw <[email protected]>

adamleff approved these changes Aug 29, 2017

View reviewed changes

adamleff requested a review from a team August 29, 2017 11:38

arlimus approved these changes Aug 30, 2017

View reviewed changes

arlimus merged commit d5f33f0 into inspec:master Aug 30, 2017

tonybaloney deleted the pip_516 branch September 5, 2017 23:51

mgrobelin mentioned this pull request Jan 15, 2018

pip resource docs: mention custom path to pip #2459

Merged

adamleff pushed a commit that referenced this pull request Jan 19, 2018

pip resource docs: mention custom path to pip as introduced by GH-2097 (

dee94b6

#2459) Signed-off-by: Markus Grobelin <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pip resource: support non-default pip locations, such as virtualenvs #2097

pip resource: support non-default pip locations, such as virtualenvs #2097

tonybaloney commented Aug 24, 2017 •

edited by adamleff

Loading

tonybaloney commented Aug 24, 2017

srenatus left a comment

srenatus Aug 24, 2017

tonybaloney Aug 24, 2017

srenatus Aug 24, 2017

tonybaloney Aug 24, 2017

srenatus Aug 24, 2017

tonybaloney commented Aug 24, 2017

srenatus commented Aug 24, 2017

adamleff left a comment

adamleff Aug 24, 2017

adamleff Aug 24, 2017

adamleff Aug 24, 2017

tonybaloney commented Aug 27, 2017

adamleff commented Aug 29, 2017

tonybaloney commented Aug 29, 2017

adamleff left a comment

arlimus left a comment

arlimus commented Aug 30, 2017

		@package_name = package_name
		@virtualenv_path = virtualenv_path

pip resource: support non-default pip locations, such as virtualenvs #2097

pip resource: support non-default pip locations, such as virtualenvs #2097

Conversation

tonybaloney commented Aug 24, 2017 • edited by adamleff Loading

tonybaloney commented Aug 24, 2017

srenatus left a 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

tonybaloney commented Aug 24, 2017

srenatus commented Aug 24, 2017

adamleff left a 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

tonybaloney commented Aug 27, 2017

adamleff commented Aug 29, 2017

tonybaloney commented Aug 29, 2017

adamleff left a comment

Choose a reason for hiding this comment

arlimus left a comment

Choose a reason for hiding this comment

arlimus commented Aug 30, 2017

tonybaloney commented Aug 24, 2017 •

edited by adamleff

Loading