correct filters for MacPorts package provider #2721
Conversation
macports needs to provide macports_package to `darwin`, not `mac_os_x`. `mac_os_x` is the platform, `darwin` is the OS.
|
@patcox you need to sort out your failed tests. |
| @@ -3,7 +3,7 @@ class Provider | |||
| class Package | |||
| class Macports < Chef::Provider::Package | |||
|
|
|||
| provides :macports_package, os: "mac_os_x" | |||
| provides :macports_package, os: "darwin" | |||
There was a problem hiding this comment.
this should probably have no filters... if someone actually specified macports_package, then they are overriding the normal logic. That seems to be how the other OS-specific providers with their own resource work... (like homebrew_package)
There was a problem hiding this comment.
Yeah, but does it make any sense on linux/solaris/freebsd/etc? Ultimately, it'd be nice to unload these classes that didn't apply to the o/s to reduce memory pressure.
There was a problem hiding this comment.
I could see arguments either way.
For consistency's sake maybe it would be best to remove the filters. (see homebrew @ https://github.com/opscode/chef/blob/master/lib/chef/provider/package/homebrew.rb#L29)
And then if we want to unload the classes later we could do that in a separate PR.
There was a problem hiding this comment.
I sort of feel like we should be more narrow and then expand later if there's a reason, rather than try to do it the other direction since its avoids a possible major revision blocker to the later change.
There was a problem hiding this comment.
My thought is more like this... I can make apt work on Redhat, and I can make yum work on Debian, etc. if someone goes out of their way to specifically say "I want to use macports_package", we shouldn't stand in their way. We should just never use macports for standard package resource on a host we don't expect it to work on.
There was a problem hiding this comment.
Hmmmm.... okay, I seem to have not restricted yum_package, apt_package, etc, so I guess I agreed with you back when I merged in the provider resolver stuff (although I coulda sworn I decided to do the other thing).
|
Added a new commit removing the filter altogether, as discussed in code review. |
|
Well, the test that's failing is "1) Chef::ProviderResolver resolving static providers on Ubuntu 14.04 when the resource is a macports_package should fall back into the old provider mapper code and hooks", except that test is now invalid... it's unclear what @lamont-granquist would like ot happen to this test. |
|
It should get removed from that one because it not longer falls through to the old provider mapper code, and now it uses the provider resolver correctly, so i think there's another list it gets added to. |
| :execute, :file, :gem_package, :git, :homebrew_package, :http_request, :link, | ||
| :log, :macports_package, :pacman_package, :paludis_package, :perl, :python, | ||
| :remote_directory, :route, :rpm_package, :ruby, :ruby_block, :script, :subversion, | ||
| :template, :timestamped_deploy, :whyrun_safe_ruby_block, :yum_package, |
There was a problem hiding this comment.
I moved :homebrew_package and :macports_package to their correct alphabetical order and made the lines roughly the same length.
|
Tests are green now. :) |
|
👍 |
|
👍 Sweet. Lets get a 3rd +1 and I'll merge. |
|
@opscode/client-core @opscode/client-engineers paging code review |
|
👍 🍰 |
|
WFM. |
correct filters for MacPorts package provider
|
Hi. I am an automated pull request bot named Curry. There are commits in this pull request whose authors are not yet authorized to contribute to Chef Software, Inc. projects or are using a non-GitHub verified email address. To become authorized to contribute, you will need to sign the Contributor License Agreement (CLA) as an individual or on behalf of your company. You can read more on Chef's blog. GitHub Users Who Are Not Authorized To ContributeThe following GitHub users do not appear to have signed a CLA: |
macports needs to provide macports_package to
darwin, notmac_os_x.mac_os_xis the platform,darwinis the OS.This resolves #2691.
@jaymzh will do the merge so it's under his CCLA.