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

Add pkgin package provider #199

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
2 participants
@mudge
Copy link
Contributor

mudge commented Nov 8, 2011

pkgin is a binary package manager for pkgsrc as used by Joyent on their SmartMachines. This is a package provider for pkgin with the standard installable and uninstallable features.

My Puppet Labs Redmine account can be found at http://projects.puppetlabs.com/users/3814

Add pkgin package provider
pkgin is a binary package manager for pkgsrc as used by Joyent on
their SmartMachines. This is a package provider for pkgin with the
standard installable and uninstallable features.

has_feature :installable, :uninstallable

def self.parse(package, force_status=nil)

This comment has been minimized.

Copy link
@jhelwig

jhelwig Dec 12, 2011

Contributor

Not a major thing, but unless self.parse needs to exist for the type/provider system here, it might be better to have a method name with a bit more semantic meaning. Something along the lines of parse_pkgin_line?

matching_package = properties if properties && resource[:name] == properties[:name]
end

matching_package

This comment has been minimized.

Copy link
@jhelwig

jhelwig Dec 13, 2011

Contributor

matching_package isn't really needed here. #detect will return the first element where the block returns true, so this could be written as:

packages.detect do |package|
  properties = self.class.parse(package)
  properties && resource[:name] == properties[:name]
end

Where this is the last thing in def query.

This comment has been minimized.

Copy link
@mudge

mudge Dec 14, 2011

Author Contributor

The problem here is that what needs to be returned is properties, not the original package so detect alone won't be enough. If we separate out the detection from the parsing, it'll mean parsing twice instead of once, e.g.

matching_package = packages.detect do |package|
  properties = self.class.parse(package)
  properties && resource[:name] == properties[:name]
end

self.class.parse(matching_package) if matching_package
describe "Puppet provider interface" do
it { respond_to(:install) }
it { respond_to(:uninstall) }
it { respond_to(:query) }

This comment has been minimized.

Copy link
@jhelwig

jhelwig Dec 13, 2011

Contributor

These three are really unnecessary given that the later tests will fail if any of these methods are missing. We're usually removing tests like this as we encounter them since they don't really provide any extra coverage.

@jhelwig

This comment has been minimized.

Copy link
Contributor

jhelwig commented Dec 13, 2011

Paul,

Thanks for working on this provider. Sorry for taking so long to get around to reviewing it. I had a few comments, but nothing major. Let me know if you won't have time to address them. It's all stuff that can easily be addressed when merging in the pull request, if you're busy.

@mudge

This comment has been minimized.

Copy link
Contributor Author

mudge commented Dec 14, 2011

Hey Jacob,

Thanks for the review; I've renamed parse to parse_pkgin_line and removed the redundant tests as per your remarks. Re the detect, please see my in-line comments but perhaps there is a better way to tackle this.

@jhelwig

This comment has been minimized.

Copy link
Contributor

jhelwig commented Dec 14, 2011

I completely glossed over the return of properties, instead of package. Your comment makes complete sense, and I don't see an obviously better way to do it, off hand.

I've squashed the two commits down into one, and merged it into the master branch in 9b7c73a.

@jhelwig jhelwig closed this Dec 14, 2011

hlindberg pushed a commit to hlindberg/puppet that referenced this pull request Oct 16, 2014

melissa pushed a commit to melissa/puppet that referenced this pull request Mar 30, 2018

Merge pull request puppetlabs#199 from james-stocks/PCP-65
(maint) Remove unused env variables from Rakefile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.