COOK-3084 - wrong python on RHEL5


#1

Bringing this to the mailing list as requested…

The comments in the ticket seem to suggest folks think I’m trying to use
the interpreter attribute, I’m not…

If you specify a straight virtualenv:

python_virtualenv “mine” do
end

on RHEL5 then the defaults in resources/virtualenv.rb:

attribute :interpreter, :default => ‘python’

mean that you end up pointing at /usr/bin/python (which is 2.4), not
/usr/bin/python26. This was a regression (for RHEL5 at least) introduced
by COOK-1415.

By contrast the code in recipes/package.rb:

if platform_family?(‘rhel’) && major_version < 6
include_recipe 'yum::epel’
python_pkgs = [“python26”, “python26-devel”]
node.set[‘python’][‘binary’] = “/usr/bin/python26”

Explicitly chooses /usr/bin/python26 on RHEL5.

All I’m trying to do with the patch on COOK-3084 is ensure that the value
of python passed to the virtualenv is consistent.

There is a minor change in behaviour on other platforms in that previously
the virtualenv provider would pass --python=python whereas now it will pass
–python=/usr/bin/python.

In many ways, not passing --python in at all would seem more correct, but
that’s not how the provider’s ever worked - something like (untested):

diff --git a/providers/virtualenv.rb b/providers/virtualenv.rb
index dfd9f4e…addb724 100644
— a/providers/virtualenv.rb
+++ b/providers/virtualenv.rb
@@ -29,7 +29,8 @@ end
action :create do
unless exists?
Chef::Log.info(“Creating virtualenv #{new_resource} at
#{new_resource.path}”)

  • execute “#{virtualenv_cmd} --python=#{new_resource.interpreter}
    #{new_resource.options} #{new_resource.path}” do
  • interpreter = new_resource.interpreter ? “
    –python=#{new_resource.interpreter}” : “”
  • execute “#{virtualenv_cmd}#{interpreter} #{new_resource.options}
    #{new_resource.path}” do
    user new_resource.owner if new_resource.owner
    group new_resource.group if new_resource.group
    end
    diff --git a/resources/virtualenv.rb b/resources/virtualenv.rb
    index dcb282c…e9f7327 100644
    — a/resources/virtualenv.rb
    +++ b/resources/virtualenv.rb
    @@ -28,7 +28,7 @@ def initialize(*args)
    end

attribute :path, :kind_of => String, :name_attribute => true
-attribute :interpreter, :default => ‘python’
+attribute :interpreter, :kind_of => String
attribute :owner, :regex => Chef::Config[:user_valid_regex]
attribute :group, :regex => Chef::Config[:group_valid_regex]
attribute :options, :kind_of => String


Alex Kiernan


#2

So I was the one that objected. Basically you should almost never use that argument to venv. If you want virtualenv to use python26 by default, then you need to install it with python26. In general it is fragile at best to try and use virtualenv to operate on a different version than it was installed with. We use node[‘python’][‘binary’] to control how pip gets installed, and then pip is what installs venv, so at heart that is the thing you need to change. The correct fix in this case is to entirely remove the use of --python from the virtualenv code, and instead 1) allow the user to control which virtualenv binary is used and 2) add an opaque #options or #extra_arguments attribute to allow giving arbitrary overrides.

–Noah

On Jun 5, 2013, at 1:50 PM, Alex Kiernan wrote:

Bringing this to the mailing list as requested…

The comments in the ticket seem to suggest folks think I’m trying to use the interpreter attribute, I’m not…

If you specify a straight virtualenv:

python_virtualenv “mine” do
end

on RHEL5 then the defaults in resources/virtualenv.rb:

attribute :interpreter, :default => ‘python’

mean that you end up pointing at /usr/bin/python (which is 2.4), not /usr/bin/python26. This was a regression (for RHEL5 at least) introduced by COOK-1415.

By contrast the code in recipes/package.rb:

if platform_family?(‘rhel’) && major_version < 6
include_recipe 'yum::epel’
python_pkgs = [“python26”, “python26-devel”]
node.set[‘python’][‘binary’] = “/usr/bin/python26”

Explicitly chooses /usr/bin/python26 on RHEL5.

All I’m trying to do with the patch on COOK-3084 is ensure that the value of python passed to the virtualenv is consistent.

There is a minor change in behaviour on other platforms in that previously the virtualenv provider would pass --python=python whereas now it will pass --python=/usr/bin/python.

In many ways, not passing --python in at all would seem more correct, but that’s not how the provider’s ever worked - something like (untested):

diff --git a/providers/virtualenv.rb b/providers/virtualenv.rb
index dfd9f4e…addb724 100644
— a/providers/virtualenv.rb
+++ b/providers/virtualenv.rb
@@ -29,7 +29,8 @@ end
action :create do
unless exists?
Chef::Log.info(“Creating virtualenv #{new_resource} at #{new_resource.path}”)

  • execute “#{virtualenv_cmd} --python=#{new_resource.interpreter} #{new_resource.options} #{new_resource.path}” do
  • interpreter = new_resource.interpreter ? " --python=#{new_resource.interpreter}" : “”
  • execute “#{virtualenv_cmd}#{interpreter} #{new_resource.options} #{new_resource.path}” do
    user new_resource.owner if new_resource.owner
    group new_resource.group if new_resource.group
    end
    diff --git a/resources/virtualenv.rb b/resources/virtualenv.rb
    index dcb282c…e9f7327 100644
    — a/resources/virtualenv.rb
    +++ b/resources/virtualenv.rb
    @@ -28,7 +28,7 @@ def initialize(*args)
    end

attribute :path, :kind_of => String, :name_attribute => true
-attribute :interpreter, :default => ‘python’
+attribute :interpreter, :kind_of => String
attribute :owner, :regex => Chef::Config[:user_valid_regex]
attribute :group, :regex => Chef::Config[:group_valid_regex]
attribute :options, :kind_of => String


Alex Kiernan


#3

I’ll check the untested patch I posted… pretty sure it’ll make an out of
the box RHEL5 box do the right thing. Passing in --python by default seems
wrong to me. Pushing it in via options if that’s what you need seems
reasonable.

Are we saying we want to remove the interpreter attribute entirely and bump
the version to 2.0.0?

On Wed, Jun 5, 2013 at 10:05 PM, Noah Kantrowitz noah@coderanger.netwrote:

So I was the one that objected. Basically you should almost never use that
argument to venv. If you want virtualenv to use python26 by default, then
you need to install it with python26. In general it is fragile at best to
try and use virtualenv to operate on a different version than it was
installed with. We use node[‘python’][‘binary’] to control how pip gets
installed, and then pip is what installs venv, so at heart that is the
thing you need to change. The correct fix in this case is to entirely
remove the use of --python from the virtualenv code, and instead 1) allow
the user to control which virtualenv binary is used and 2) add an opaque
#options or #extra_arguments attribute to allow giving arbitrary overrides.

–Noah

On Jun 5, 2013, at 1:50 PM, Alex Kiernan wrote:

Bringing this to the mailing list as requested…

The comments in the ticket seem to suggest folks think I’m trying to use
the interpreter attribute, I’m not…

If you specify a straight virtualenv:

python_virtualenv “mine” do
end

on RHEL5 then the defaults in resources/virtualenv.rb:

attribute :interpreter, :default => ‘python’

mean that you end up pointing at /usr/bin/python (which is 2.4), not
/usr/bin/python26. This was a regression (for RHEL5 at least) introduced by
COOK-1415.

By contrast the code in recipes/package.rb:

if platform_family?(‘rhel’) && major_version < 6
include_recipe 'yum::epel’
python_pkgs = [“python26”, “python26-devel”]
node.set[‘python’][‘binary’] = “/usr/bin/python26”

Explicitly chooses /usr/bin/python26 on RHEL5.

All I’m trying to do with the patch on COOK-3084 is ensure that the
value of python passed to the virtualenv is consistent.

There is a minor change in behaviour on other platforms in that
previously the virtualenv provider would pass --python=python whereas now
it will pass --python=/usr/bin/python.

In many ways, not passing --python in at all would seem more correct,
but that’s not how the provider’s ever worked - something like (untested):

diff --git a/providers/virtualenv.rb b/providers/virtualenv.rb
index dfd9f4e…addb724 100644
— a/providers/virtualenv.rb
+++ b/providers/virtualenv.rb
@@ -29,7 +29,8 @@ end
action :create do
unless exists?
Chef::Log.info(“Creating virtualenv #{new_resource} at
#{new_resource.path}”)

  • execute “#{virtualenv_cmd} --python=#{new_resource.interpreter}
    #{new_resource.options} #{new_resource.path}” do
  • interpreter = new_resource.interpreter ? “
    –python=#{new_resource.interpreter}” : “”
  • execute “#{virtualenv_cmd}#{interpreter} #{new_resource.options}
    #{new_resource.path}” do
    user new_resource.owner if new_resource.owner
    group new_resource.group if new_resource.group
    end
    diff --git a/resources/virtualenv.rb b/resources/virtualenv.rb
    index dcb282c…e9f7327 100644
    — a/resources/virtualenv.rb
    +++ b/resources/virtualenv.rb
    @@ -28,7 +28,7 @@ def initialize(*args)
    end

attribute :path, :kind_of => String, :name_attribute => true
-attribute :interpreter, :default => ‘python’
+attribute :interpreter, :kind_of => String
attribute :owner, :regex => Chef::Config[:user_valid_regex]
attribute :group, :regex => Chef::Config[:group_valid_regex]
attribute :options, :kind_of => String


Alex Kiernan


Alex Kiernan


#4

On 6/5/13 1:50 PM, Alex Kiernan wrote:

diff --git a/providers/virtualenv.rb b/providers/virtualenv.rb
index dfd9f4e…addb724 100644
— a/providers/virtualenv.rb
+++ b/providers/virtualenv.rb
@@ -29,7 +29,8 @@ end
action :create do
unless exists?
Chef::Log.info(“Creating virtualenv #{new_resource} at
#{new_resource.path}”)

  • execute “#{virtualenv_cmd} --python=#{new_resource.interpreter}
    #{new_resource.options} #{new_resource.path}” do
  • interpreter = new_resource.interpreter ? “
    –python=#{new_resource.interpreter}” : “”
  • execute “#{virtualenv_cmd}#{interpreter} #{new_resource.options}
    #{new_resource.path}” do
    user new_resource.owner if new_resource.owner
    group new_resource.group if new_resource.group
    end

As I mentioned in the code review, I would be +1 on this as an
intermediary step before the full fix.


Steven Danna
Systems Engineer, Opscode, Inc
GPG Key: http://stevendanna.github.com/downloads/code/public.key


#5

From virtualenv help:

-p PYTHON_EXE, --python=PYTHON_EXE
The Python interpreter to use, e.g.,
–python=python2.5 will use the python2.5 interpreter
to create the new environment. The default is the
interpreter that virtualenv was installed with

-1 for removal of the ‘–python’ option and major version bump right
now. I’d add that as a branch slated for version 2.0.0 to be fixed
sometime in the future, when there are other big changes.

To Noah’s point of installing/invoking virtualenv with the desired
binary, it seems that in order to make that work, this 1 method
would have to be updated to use the correct one, since currently it
will look for whichever it first finds in $PATH.
Not opposing the approach, but it’s probably a larger scope than this
particular PR.

I think that having the python cookbook be opinionated about which
binary to use on RHEL5 in one instance, and not conforming to that is
probably a logical bug, and should be addressed to be in line with the
decision to use the resource’s argument or the node attribute.

Q: Can you use node attributes in providers? When do they resolve, and
if a subsequent attribute is set later in the run, how does that
affect this?

If the answer is “it’s all good and cool - setting
node[‘python’][‘binary’] in a role/env/override attribute later works
as expected”, then I think I’m on board with the change.

-Mike

On Thu, Jun 6, 2013 at 9:26 AM, Steven Danna steve@opscode.com wrote:

On 6/5/13 1:50 PM, Alex Kiernan wrote:

diff --git a/providers/virtualenv.rb b/providers/virtualenv.rb
index dfd9f4e…addb724 100644
— a/providers/virtualenv.rb
+++ b/providers/virtualenv.rb
@@ -29,7 +29,8 @@ end
action :create do
unless exists?
Chef::Log.info(“Creating virtualenv #{new_resource} at
#{new_resource.path}”)

  • execute “#{virtualenv_cmd} --python=#{new_resource.interpreter}
    #{new_resource.options} #{new_resource.path}” do
  • interpreter = new_resource.interpreter ? “
    –python=#{new_resource.interpreter}” : “”
  • execute “#{virtualenv_cmd}#{interpreter} #{new_resource.options}
    #{new_resource.path}” do
    user new_resource.owner if new_resource.owner
    group new_resource.group if new_resource.group
    end

As I mentioned in the code review, I would be +1 on this as an
intermediary step before the full fix.


Steven Danna
Systems Engineer, Opscode, Inc
GPG Key: http://stevendanna.github.com/downloads/code/public.key


#6

On 6/6/13 6:49 AM, Mike wrote:

Q: Can you use node attributes in providers? When do they resolve, and
if a subsequent attribute is set later in the run, how does that
affect this?

If the answer is “it’s all good and cool - setting
node[‘python’][‘binary’] in a role/env/override attribute later works
as expected”, then I think I’m on board with the change.

For most cases it will work as expected. I believe that certain methods
of updating attributes inside a recipe could cause problems, but I would
have have to do some experimentation to be sure.

Overall, however, I don’t like using node attributes to modify resource
behavior behind the scenes like this as it puts the data which modifies
the behavior of a resource pretty far from the resource itself.

Cheers,

Steven


Steven Danna
Systems Engineer, Opscode, Inc
GPG Key: http://stevendanna.github.com/downloads/code/public.key


#7

A big +1 from me for the -1 on the ticket… you’ve made me go and get
test-kitchen working :slight_smile:

I’ve added a test-kitchen test cookbook to show the problem and I’m just
testing out the three branches which do what I think are the options:

  • patch level, fix up what’s passed to --python (my original patch)
  • minor level, don’t pass in --python unless interpreter is explicitly
    specified
  • major level, drop interpreter altogether

May or may not get to it this evening, depends how soon my wife makes it
home (I gather the traffic’s terrible, so I may get it done :))

On Thu, Jun 6, 2013 at 2:59 PM, Steven Danna steve@opscode.com wrote:

On 6/6/13 6:49 AM, Mike wrote:

Q: Can you use node attributes in providers? When do they resolve, and
if a subsequent attribute is set later in the run, how does that
affect this?

If the answer is “it’s all good and cool - setting
node[‘python’][‘binary’] in a role/env/override attribute later works
as expected”, then I think I’m on board with the change.

For most cases it will work as expected. I believe that certain methods
of updating attributes inside a recipe could cause problems, but I would
have have to do some experimentation to be sure.

Overall, however, I don’t like using node attributes to modify resource
behavior behind the scenes like this as it puts the data which modifies
the behavior of a resource pretty far from the resource itself.

Cheers,

Steven


Steven Danna
Systems Engineer, Opscode, Inc
GPG Key: http://stevendanna.github.com/downloads/code/public.key


Alex Kiernan


#8

Four branches pushed:

All three changes pass for CentOS 5.9/6.4 package installs; I’m leaving the
minor level change (the proposed patch earlier in this thread) running the
full suite and will update the ticket assuming it passes.

On Thu, Jun 6, 2013 at 5:42 PM, Alex Kiernan alex.kiernan@gmail.com wrote:

A big +1 from me for the -1 on the ticket… you’ve made me go and get
test-kitchen working :slight_smile:

I’ve added a test-kitchen test cookbook to show the problem and I’m just
testing out the three branches which do what I think are the options:

  • patch level, fix up what’s passed to --python (my original patch)
  • minor level, don’t pass in --python unless interpreter is explicitly
    specified
  • major level, drop interpreter altogether

May or may not get to it this evening, depends how soon my wife makes it
home (I gather the traffic’s terrible, so I may get it done :))

On Thu, Jun 6, 2013 at 2:59 PM, Steven Danna steve@opscode.com wrote:

On 6/6/13 6:49 AM, Mike wrote:

Q: Can you use node attributes in providers? When do they resolve, and
if a subsequent attribute is set later in the run, how does that
affect this?

If the answer is “it’s all good and cool - setting
node[‘python’][‘binary’] in a role/env/override attribute later works
as expected”, then I think I’m on board with the change.

For most cases it will work as expected. I believe that certain methods
of updating attributes inside a recipe could cause problems, but I would
have have to do some experimentation to be sure.

Overall, however, I don’t like using node attributes to modify resource
behavior behind the scenes like this as it puts the data which modifies
the behavior of a resource pretty far from the resource itself.

Cheers,

Steven


Steven Danna
Systems Engineer, Opscode, Inc
GPG Key: http://stevendanna.github.com/downloads/code/public.key


Alex Kiernan


Alex Kiernan


#9

Tests completed across all platforms on the avoid-venv-python branch
successfully. Should I update the ticket back to fix provided and point at
this branch?
On 6 Jun 2013 19:20, “Alex Kiernan” alex.kiernan@gmail.com wrote:

Four branches pushed:

All three changes pass for CentOS 5.9/6.4 package installs; I’m leaving
the minor level change (the proposed patch earlier in this thread) running
the full suite and will update the ticket assuming it passes.

On Thu, Jun 6, 2013 at 5:42 PM, Alex Kiernan alex.kiernan@gmail.comwrote:

A big +1 from me for the -1 on the ticket… you’ve made me go and get
test-kitchen working :slight_smile:

I’ve added a test-kitchen test cookbook to show the problem and I’m just
testing out the three branches which do what I think are the options:

  • patch level, fix up what’s passed to --python (my original patch)
  • minor level, don’t pass in --python unless interpreter is explicitly
    specified
  • major level, drop interpreter altogether

May or may not get to it this evening, depends how soon my wife makes it
home (I gather the traffic’s terrible, so I may get it done :))

On Thu, Jun 6, 2013 at 2:59 PM, Steven Danna steve@opscode.com wrote:

On 6/6/13 6:49 AM, Mike wrote:

Q: Can you use node attributes in providers? When do they resolve, and
if a subsequent attribute is set later in the run, how does that
affect this?

If the answer is “it’s all good and cool - setting
node[‘python’][‘binary’] in a role/env/override attribute later works
as expected”, then I think I’m on board with the change.

For most cases it will work as expected. I believe that certain methods
of updating attributes inside a recipe could cause problems, but I would
have have to do some experimentation to be sure.

Overall, however, I don’t like using node attributes to modify resource
behavior behind the scenes like this as it puts the data which modifies
the behavior of a resource pretty far from the resource itself.

Cheers,

Steven


Steven Danna
Systems Engineer, Opscode, Inc
GPG Key: http://stevendanna.github.com/downloads/code/public.key


Alex Kiernan


Alex Kiernan


#10

Alex,

I looked through the code, looks cool that you’ve got TK running.
Beyond having test cookbooks that exercise the LWRP, I didn’t see
actual tests that assert a given state.
Were they missing?

Steven - I somewhat disagree about the “distance” of an attribute
percolating into a provider - I think that if the cookbook provides
sane defaults for attributes used, and an operator chooses to override
that particular attribute, then it is somewhat incumbent upon the
operator to read the docs and understand the impact of the override.

In this particular case, we would “protect them from themselves” -
since we are already declaring a preferred interpreter binary for the
target platform, remaining consistent with the declaration makes sense
to me.

So my vote would be for the Patch version, since the current behavior is broken.

-M

On Thu, Jun 6, 2013 at 4:11 PM, Alex Kiernan alex.kiernan@gmail.com wrote:

Tests completed across all platforms on the avoid-venv-python branch
successfully. Should I update the ticket back to fix provided and point at
this branch?

On 6 Jun 2013 19:20, “Alex Kiernan” alex.kiernan@gmail.com wrote:

Four branches pushed:

All three changes pass for CentOS 5.9/6.4 package installs; I’m leaving
the minor level change (the proposed patch earlier in this thread) running
the full suite and will update the ticket assuming it passes.

On Thu, Jun 6, 2013 at 5:42 PM, Alex Kiernan alex.kiernan@gmail.com
wrote:

A big +1 from me for the -1 on the ticket… you’ve made me go and get
test-kitchen working :slight_smile:

I’ve added a test-kitchen test cookbook to show the problem and I’m just
testing out the three branches which do what I think are the options:

  • patch level, fix up what’s passed to --python (my original patch)
  • minor level, don’t pass in --python unless interpreter is explicitly
    specified
  • major level, drop interpreter altogether

May or may not get to it this evening, depends how soon my wife makes it
home (I gather the traffic’s terrible, so I may get it done :))

On Thu, Jun 6, 2013 at 2:59 PM, Steven Danna steve@opscode.com wrote:

On 6/6/13 6:49 AM, Mike wrote:

Q: Can you use node attributes in providers? When do they resolve, and
if a subsequent attribute is set later in the run, how does that
affect this?

If the answer is “it’s all good and cool - setting
node[‘python’][‘binary’] in a role/env/override attribute later works
as expected”, then I think I’m on board with the change.

For most cases it will work as expected. I believe that certain methods
of updating attributes inside a recipe could cause problems, but I would
have have to do some experimentation to be sure.

Overall, however, I don’t like using node attributes to modify resource
behavior behind the scenes like this as it puts the data which modifies
the behavior of a resource pretty far from the resource itself.

Cheers,

Steven


Steven Danna
Systems Engineer, Opscode, Inc
GPG Key: http://stevendanna.github.com/downloads/code/public.key


Alex Kiernan


Alex Kiernan


#11

At the time I was focussed on getting test kitchen working and just
exercising the LWRP was sufficient to show the problem. Having gone
back for another look at chef-client it’s pretty clear how to add
those assertions and would improve the tests, I’ll go have another
look.

For me, the premise should be about giving some cross platform
consistency; the levers are there if you need to go tweak, but you
should only need to pull them if you’ve a genuine need to diverge from
the defaults. Consuming the python recipe on EL5 using all the
defaults, then needing to know that you have to pull one of those
levers in order to use the virtualenv LWRP seems wrong to me.

On Sat, Jun 8, 2013 at 9:44 PM, Mike miketheman@gmail.com wrote:

Alex,

I looked through the code, looks cool that you’ve got TK running.
Beyond having test cookbooks that exercise the LWRP, I didn’t see
actual tests that assert a given state.
Were they missing?

Steven - I somewhat disagree about the “distance” of an attribute
percolating into a provider - I think that if the cookbook provides
sane defaults for attributes used, and an operator chooses to override
that particular attribute, then it is somewhat incumbent upon the
operator to read the docs and understand the impact of the override.

In this particular case, we would “protect them from themselves” -
since we are already declaring a preferred interpreter binary for the
target platform, remaining consistent with the declaration makes sense
to me.

So my vote would be for the Patch version, since the current behavior is broken.

-M

On Thu, Jun 6, 2013 at 4:11 PM, Alex Kiernan alex.kiernan@gmail.com wrote:

Tests completed across all platforms on the avoid-venv-python branch
successfully. Should I update the ticket back to fix provided and point at
this branch?

On 6 Jun 2013 19:20, “Alex Kiernan” alex.kiernan@gmail.com wrote:

Four branches pushed:

All three changes pass for CentOS 5.9/6.4 package installs; I’m leaving
the minor level change (the proposed patch earlier in this thread) running
the full suite and will update the ticket assuming it passes.

On Thu, Jun 6, 2013 at 5:42 PM, Alex Kiernan alex.kiernan@gmail.com
wrote:

A big +1 from me for the -1 on the ticket… you’ve made me go and get
test-kitchen working :slight_smile:

I’ve added a test-kitchen test cookbook to show the problem and I’m just
testing out the three branches which do what I think are the options:

  • patch level, fix up what’s passed to --python (my original patch)
  • minor level, don’t pass in --python unless interpreter is explicitly
    specified
  • major level, drop interpreter altogether

May or may not get to it this evening, depends how soon my wife makes it
home (I gather the traffic’s terrible, so I may get it done :))

On Thu, Jun 6, 2013 at 2:59 PM, Steven Danna steve@opscode.com wrote:

On 6/6/13 6:49 AM, Mike wrote:

Q: Can you use node attributes in providers? When do they resolve, and
if a subsequent attribute is set later in the run, how does that
affect this?

If the answer is “it’s all good and cool - setting
node[‘python’][‘binary’] in a role/env/override attribute later works
as expected”, then I think I’m on board with the change.

For most cases it will work as expected. I believe that certain methods
of updating attributes inside a recipe could cause problems, but I would
have have to do some experimentation to be sure.

Overall, however, I don’t like using node attributes to modify resource
behavior behind the scenes like this as it puts the data which modifies
the behavior of a resource pretty far from the resource itself.

Cheers,

Steven


Steven Danna
Systems Engineer, Opscode, Inc
GPG Key: http://stevendanna.github.com/downloads/code/public.key


Alex Kiernan


Alex Kiernan


Alex Kiernan


#12

the defaults. Consuming the python recipe on EL5 using all the
defaults, then needing to know that you have to pull one of those
levers in order to use the virtualenv LWRP seems wrong to me.

I don’t think I understand which proposed solution this is referring to.
-M

On Sun, Jun 9, 2013 at 1:47 AM, Alex Kiernan alex.kiernan@gmail.com wrote:

At the time I was focussed on getting test kitchen working and just
exercising the LWRP was sufficient to show the problem. Having gone
back for another look at chef-client it’s pretty clear how to add
those assertions and would improve the tests, I’ll go have another
look.

For me, the premise should be about giving some cross platform
consistency; the levers are there if you need to go tweak, but you
should only need to pull them if you’ve a genuine need to diverge from
the defaults. Consuming the python recipe on EL5 using all the
defaults, then needing to know that you have to pull one of those
levers in order to use the virtualenv LWRP seems wrong to me.

On Sat, Jun 8, 2013 at 9:44 PM, Mike miketheman@gmail.com wrote:

Alex,

I looked through the code, looks cool that you’ve got TK running.
Beyond having test cookbooks that exercise the LWRP, I didn’t see
actual tests that assert a given state.
Were they missing?

Steven - I somewhat disagree about the “distance” of an attribute
percolating into a provider - I think that if the cookbook provides
sane defaults for attributes used, and an operator chooses to override
that particular attribute, then it is somewhat incumbent upon the
operator to read the docs and understand the impact of the override.

In this particular case, we would “protect them from themselves” -
since we are already declaring a preferred interpreter binary for the
target platform, remaining consistent with the declaration makes sense
to me.

So my vote would be for the Patch version, since the current behavior is broken.

-M

On Thu, Jun 6, 2013 at 4:11 PM, Alex Kiernan alex.kiernan@gmail.com wrote:

Tests completed across all platforms on the avoid-venv-python branch
successfully. Should I update the ticket back to fix provided and point at
this branch?

On 6 Jun 2013 19:20, “Alex Kiernan” alex.kiernan@gmail.com wrote:

Four branches pushed:

All three changes pass for CentOS 5.9/6.4 package installs; I’m leaving
the minor level change (the proposed patch earlier in this thread) running
the full suite and will update the ticket assuming it passes.

On Thu, Jun 6, 2013 at 5:42 PM, Alex Kiernan alex.kiernan@gmail.com
wrote:

A big +1 from me for the -1 on the ticket… you’ve made me go and get
test-kitchen working :slight_smile:

I’ve added a test-kitchen test cookbook to show the problem and I’m just
testing out the three branches which do what I think are the options:

  • patch level, fix up what’s passed to --python (my original patch)
  • minor level, don’t pass in --python unless interpreter is explicitly
    specified
  • major level, drop interpreter altogether

May or may not get to it this evening, depends how soon my wife makes it
home (I gather the traffic’s terrible, so I may get it done :))

On Thu, Jun 6, 2013 at 2:59 PM, Steven Danna steve@opscode.com wrote:

On 6/6/13 6:49 AM, Mike wrote:

Q: Can you use node attributes in providers? When do they resolve, and
if a subsequent attribute is set later in the run, how does that
affect this?

If the answer is “it’s all good and cool - setting
node[‘python’][‘binary’] in a role/env/override attribute later works
as expected”, then I think I’m on board with the change.

For most cases it will work as expected. I believe that certain methods
of updating attributes inside a recipe could cause problems, but I would
have have to do some experimentation to be sure.

Overall, however, I don’t like using node attributes to modify resource
behavior behind the scenes like this as it puts the data which modifies
the behavior of a resource pretty far from the resource itself.

Cheers,

Steven


Steven Danna
Systems Engineer, Opscode, Inc
GPG Key: http://stevendanna.github.com/downloads/code/public.key


Alex Kiernan


Alex Kiernan


Alex Kiernan


#13

On Sun, Jun 9, 2013 at 5:51 PM, Mike miketheman@gmail.com wrote:

the defaults. Consuming the python recipe on EL5 using all the
defaults, then needing to know that you have to pull one of those
levers in order to use the virtualenv LWRP seems wrong to me.

I don’t think I understand which proposed solution this is referring to.

How it is today - all of the proposed solutions remove the need for
special cased interpreter use on EL5.

Basically, I was agreeing with you :slight_smile:

Just waiting on the test-suite run to complete before pushing new
branches which include minitest assertions.

-M

On Sun, Jun 9, 2013 at 1:47 AM, Alex Kiernan alex.kiernan@gmail.com wrote:

At the time I was focussed on getting test kitchen working and just
exercising the LWRP was sufficient to show the problem. Having gone
back for another look at chef-client it’s pretty clear how to add
those assertions and would improve the tests, I’ll go have another
look.

For me, the premise should be about giving some cross platform
consistency; the levers are there if you need to go tweak, but you
should only need to pull them if you’ve a genuine need to diverge from
the defaults. Consuming the python recipe on EL5 using all the
defaults, then needing to know that you have to pull one of those
levers in order to use the virtualenv LWRP seems wrong to me.

On Sat, Jun 8, 2013 at 9:44 PM, Mike miketheman@gmail.com wrote:

Alex,

I looked through the code, looks cool that you’ve got TK running.
Beyond having test cookbooks that exercise the LWRP, I didn’t see
actual tests that assert a given state.
Were they missing?

Steven - I somewhat disagree about the “distance” of an attribute
percolating into a provider - I think that if the cookbook provides
sane defaults for attributes used, and an operator chooses to override
that particular attribute, then it is somewhat incumbent upon the
operator to read the docs and understand the impact of the override.

In this particular case, we would “protect them from themselves” -
since we are already declaring a preferred interpreter binary for the
target platform, remaining consistent with the declaration makes sense
to me.

So my vote would be for the Patch version, since the current behavior is broken.

-M

On Thu, Jun 6, 2013 at 4:11 PM, Alex Kiernan alex.kiernan@gmail.com wrote:

Tests completed across all platforms on the avoid-venv-python branch
successfully. Should I update the ticket back to fix provided and point at
this branch?

On 6 Jun 2013 19:20, “Alex Kiernan” alex.kiernan@gmail.com wrote:

Four branches pushed:

All three changes pass for CentOS 5.9/6.4 package installs; I’m leaving
the minor level change (the proposed patch earlier in this thread) running
the full suite and will update the ticket assuming it passes.

On Thu, Jun 6, 2013 at 5:42 PM, Alex Kiernan alex.kiernan@gmail.com
wrote:

A big +1 from me for the -1 on the ticket… you’ve made me go and get
test-kitchen working :slight_smile:

I’ve added a test-kitchen test cookbook to show the problem and I’m just
testing out the three branches which do what I think are the options:

  • patch level, fix up what’s passed to --python (my original patch)
  • minor level, don’t pass in --python unless interpreter is explicitly
    specified
  • major level, drop interpreter altogether

May or may not get to it this evening, depends how soon my wife makes it
home (I gather the traffic’s terrible, so I may get it done :))

On Thu, Jun 6, 2013 at 2:59 PM, Steven Danna steve@opscode.com wrote:

On 6/6/13 6:49 AM, Mike wrote:

Q: Can you use node attributes in providers? When do they resolve, and
if a subsequent attribute is set later in the run, how does that
affect this?

If the answer is “it’s all good and cool - setting
node[‘python’][‘binary’] in a role/env/override attribute later works
as expected”, then I think I’m on board with the change.

For most cases it will work as expected. I believe that certain methods
of updating attributes inside a recipe could cause problems, but I would
have have to do some experimentation to be sure.

Overall, however, I don’t like using node attributes to modify resource
behavior behind the scenes like this as it puts the data which modifies
the behavior of a resource pretty far from the resource itself.

Cheers,

Steven


Steven Danna
Systems Engineer, Opscode, Inc
GPG Key: http://stevendanna.github.com/downloads/code/public.key


Alex Kiernan


Alex Kiernan


Alex Kiernan


Alex Kiernan


#14

On Sun, Jun 9, 2013 at 1:31 PM, Alex Kiernan alex.kiernan@gmail.com wrote:

Just waiting on the test-suite run to complete before pushing new
branches which include minitest assertions.

Is this pushed? Are the branches all set?

Do we need to vote or did logic win out here somewhere? Who still
needs some convincing?

Bryan


#15

On 10 Jun 2013 21:54, “Bryan McLellan” btm@loftninjas.org wrote:

On Sun, Jun 9, 2013 at 1:31 PM, Alex Kiernan alex.kiernan@gmail.com
wrote:

Just waiting on the test-suite run to complete before pushing new
branches which include minitest assertions.

Is this pushed?

It is.

Are the branches all set?

Should be…

Do we need to vote or did logic win out here somewhere? Who still
needs some convincing?

If we can get consensus on which approach I’ll feel the test run across
that branch as it’s roughly 90 minutes per test run per branch.


#16

My vote is the patch-level change on this branch:
https://github.com/akiernan/python/tree/handle-rhel5.
Compare here: https://github.com/akiernan/python/compare/opscode-cookbooks:master...handle-rhel5

My question would be regarding the test here:
https://github.com/akiernan/python/compare/opscode-cookbooks:master...handle-rhel5#L7R29

Should/can this default to a node attribute, so if the attribute was
modified to “/usr/bin/python27” it could pass as well? Or do we
refrain from adding attributes to tests?

Anyways, this is awesome work Alex - very thorough!
-M

On Mon, Jun 10, 2013 at 5:15 PM, Alex Kiernan alex.kiernan@gmail.com wrote:

On 10 Jun 2013 21:54, “Bryan McLellan” btm@loftninjas.org wrote:

On Sun, Jun 9, 2013 at 1:31 PM, Alex Kiernan alex.kiernan@gmail.com
wrote:

Just waiting on the test-suite run to complete before pushing new
branches which include minitest assertions.

Is this pushed?

It is.

Are the branches all set?

Should be…

Do we need to vote or did logic win out here somewhere? Who still
needs some convincing?

If we can get consensus on which approach I’ll feel the test run across that
branch as it’s roughly 90 minutes per test run per branch.


#17

If I had a free hand I’d pick this one
https://github.com/akiernan/python/tree/remove-interpreter as I feel that
making interpreter a first class attribute sends the wrong message, but a
major version bump for a single change seems excessive.

Given I don’t have a free hand, my preferred option would be
https://github.com/akiernan/python/tree/avoid-venv-python since it doesn’t
gratuitously pass in --python and feels like it has the best chance of
doing what the virtualenv authors intended.

I actually like https://github.com/akiernan/python/tree/handle-rhel5 the
least, but I initially did it like that in an effort to find the smallest
possible change which would resolve the problem (and most likely to get
accepted… oh how wrong I was :))

My question would be regarding the test here:

https://github.com/akiernan/python/compare/opscode-cookbooks:master...handle-rhel5#L7R29

Should/can this default to a node attribute, so if the attribute was
modified to “/usr/bin/python27” it could pass as well? Or do we
refrain from adding attributes to tests?

I’m always wary of tests that do that kind of thing, it feels like it’s too
easy to get into a situation where you’re really doing `assert x == x’. But
I guess these are integration tests and we’re taking a trip through the
runtime so that shouldn’t apply; equally I really can’t see anyone being
crazy enough to try and get EL5 up to Python 2.7.

In all honesty I think the tests I’ve added actually want refactoring into
a proper virtualenv test suite, but that doesn’t really sit well with a
red-green defect fixing style; I’d rather get this change in then think
about the refactor.

On Mon, Jun 10, 2013 at 10:31 PM, Mike miketheman@gmail.com wrote:

My vote is the patch-level change on this branch:
https://github.com/akiernan/python/tree/handle-rhel5.
Compare here:
https://github.com/akiernan/python/compare/opscode-cookbooks:master...handle-rhel5

My question would be regarding the test here:

https://github.com/akiernan/python/compare/opscode-cookbooks:master...handle-rhel5#L7R29

Should/can this default to a node attribute, so if the attribute was
modified to “/usr/bin/python27” it could pass as well? Or do we
refrain from adding attributes to tests?

Anyways, this is awesome work Alex - very thorough!
-M

On Mon, Jun 10, 2013 at 5:15 PM, Alex Kiernan alex.kiernan@gmail.com
wrote:

On 10 Jun 2013 21:54, “Bryan McLellan” btm@loftninjas.org wrote:

On Sun, Jun 9, 2013 at 1:31 PM, Alex Kiernan alex.kiernan@gmail.com
wrote:

Just waiting on the test-suite run to complete before pushing new
branches which include minitest assertions.

Is this pushed?

It is.

Are the branches all set?

Should be…

Do we need to vote or did logic win out here somewhere? Who still
needs some convincing?

If we can get consensus on which approach I’ll feel the test run across
that
branch as it’s roughly 90 minutes per test run per branch.


Alex Kiernan