Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CHEF-5037] default to IPS packages on Solaris 5.11+ #1268

Merged
merged 2 commits into from
Mar 19, 2014

Conversation

ccope
Copy link
Contributor

@ccope ccope commented Feb 14, 2014

This behavior was correct until 7fa1ea5 flipped the logic.

This behavior was correct until 7fa1ea5 flipped the logic.
@elijah
Copy link

elijah commented Feb 14, 2014

Does this work on Solaris 11+ without trashing the behavior on e.g. SmartOS, where pkgin is the method rather than IPS? I'm not familiar with where the conditionals for that are - I usually use a pkgin cookbook myself, but others may not know to do so, or understand the differences...

@lamont-granquist
Copy link
Contributor

SmartOS is a different block that uses this class:

Chef::Provider::Package::SmartOS

The "Solaris" and "IPS" package providers are both wrong for smartos, this doesn't affect them, just fixes solaris.

@lamont-granquist
Copy link
Contributor

@ccope any chance for some specs to test this in spec/platform_spec.rb so that it doesn't break again?

it looks like the existing unit tests are kind of bad for this and you'd need to probably wrap the existing tests with a "context 'while testing with fake data'" or something and then have a "context 'while testing the configured platform data'" and then make sure that Chef::Platform.find("Solaris", "5.9") gets the right package provider. we don't do anything like this right now. let me know if you need help.

@ccope
Copy link
Contributor Author

ccope commented Feb 18, 2014

@lamont-granquist Sure. I haven't done much rspec before, but both of these seem to work. Is there a preferred style?

it "should use the solaris package provider on Solaris <11" do
  Chef::Platform.find("Solaris2", "5.9")[:package].should eql(Chef::Provider::Package::Solaris)
end

it "should use the IPS package provider on Solaris 11" do
  pmap = Chef::Platform.find("Solaris2", "5.11")
  pmap[:package].should eql(Chef::Provider::Package::Ips)
end

@lamont-granquist
Copy link
Contributor

Style should mostly just match the style of the spec file that you're modifying. The use of let variables over instance variables is highly encouraged, but not an issue in this case. Those look fine.

I prefer the new expect().to syntax now, but like consistency throughout the spec file better, and you shouldn't need to update the whole thing for this patch.

@lamont-granquist
Copy link
Contributor

LGTM

sersut pushed a commit that referenced this pull request Mar 19, 2014
[CHEF-5037] default to IPS packages on Solaris 5.11+
@sersut sersut merged commit 0ca7110 into chef:master Mar 19, 2014
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants