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

Don't enable packages plugin by default #717

Merged
merged 2 commits into from
Feb 2, 2016

Conversation

jaym
Copy link
Contributor

@jaym jaym commented Feb 2, 2016

This is a hack. We can make this into a proper
DSL thing where specifying something like default_enabled false.

For now, this could be enabled through

ohai[:plugins][:packages][:enabled] = true

I want to make sure we don't back ourselves into a situation where we want/need to make a breaking change. This is a possible alternative to completely reverting the change, though I feel it may conflict with the idea of disabled_plugins.

@lamont-granquist
Copy link
Contributor

i think disabled_plugins is also a hack, this actually looks a bit better to me...

@lamont-granquist
Copy link
Contributor

@danielsdeleo
Copy link
Contributor

The consensus we came to for disabling the pnp plugin by default was to remove it from the tree and put it in a cookbook. We should follow the same procedure everywhere, though personally I'm not invested in which one we choose.

@mwrock
Copy link
Member

mwrock commented Feb 2, 2016

👍

@jaym
Copy link
Contributor Author

jaym commented Feb 2, 2016

we should definitely follow the same pattern for everything we do. I think having an ohai extra's cookbook has its benefits. Shipping it external to Chef probably means we can iterate faster. It has the downside that it may be a little harder to use.

perhaps we should revert for now, and RFC to figure out if people have a strong preference.

@lamont-granquist
Copy link
Contributor

I actually like this approach. I'd prefer it to look like this, tho:

Ohai.plugin(:Packages) do
  enabled false  # defaults to true
  collect_data(:linux) do  # automagically checks configuration(enabled)
  end
end

@jaym
Copy link
Contributor Author

jaym commented Feb 2, 2016

Right, and it fits nicely with parts of chef-boneyard/chef-rfc#183. Though this is an RFC so it could possibly be a terrible idea.

@lamont-granquist
Copy link
Contributor

Yeah i think we also discussed deprecating disabled_plugins along with the RFC for ohai configuration. That could be tied nicely into this.

@danielsdeleo
Copy link
Contributor

For the long term, I guess we could have "optional_attributes" be a thing and you would request them via attribute paths, e.g., optional_attributes "packages", "windows/pnp". It'd be a breaking change to mark a bunch of existing things as optional, so we could make a breaking config language change at the same time. If everyone is cool with this as a first effort, then I'm 👍 But we need to figure out what to do about *nix for this change as the travis tests are pointing out. Is it just windows package listing that's new, or is the whole plugin new?

@lamont-granquist
Copy link
Contributor

whole plugin is new

@danielsdeleo
Copy link
Contributor

Should we make it "optional" for everyone then?

@jaym
Copy link
Contributor Author

jaym commented Feb 2, 2016

@danielsdeleo i'm not sure that we need optional_attributes. https://github.com/chef/chef-rfc/blob/master/rfc053-ohai-config.md covers the configuration i think

@jaym
Copy link
Contributor Author

jaym commented Feb 2, 2016

Oh, I see, that's for like chef configuration?

@danielsdeleo
Copy link
Contributor

@jaym yeah, the idea is that some plugins would be marked "optional" and would be opt-in, then you would enable them via configuration somewhere that says "I expect ohai to collect these attributes when it runs"

@lamont-granquist
Copy link
Contributor

i would like to get to requested attributes driving enabled plugins. for now i think just having switches at the plugin level and exposed in the configuration settings is good enough to move the ball forwards.

@btm
Copy link
Contributor

btm commented Feb 2, 2016

I'd prefer that we disable the "bad" plugins but leave them in ohai, and use a configuration value (rfc035 compliant) in the chef configuration (client.rb) to re-enable them if the user chooses.

the only downside is it is a little painful to get those configuration values into your client.rb right now but chef-boneyard/chef-rfc#183 should solve that and was the next step on the roadmap I laid out in the rfc035 discussion that diverted into an ohai hints / UX discussion.

mostly because this will be a completely reasonable pattern, and as we get closer to being able to easily configure ohai via Chef, its going to be very valuable and Right[tm] for all choices.

@lamont-granquist
Copy link
Contributor

Also at some point in the future we can disable all the plugins by default, and then the requested_attributes feature can selectively enable and run the plugins as necessary... I think this is a step in that direction.

@jaym jaym force-pushed the jdm/disable-packages-by-default branch from ed53e2a to 0e58e8d Compare February 2, 2016 20:04
@btm
Copy link
Contributor

btm commented Feb 2, 2016

@lamont-granquist exactly. by 2025 we will have completed the entire roadmap we originally had for ohai 7.

@jaym
Copy link
Contributor Author

jaym commented Feb 2, 2016

The plan is to merge this. We want to take Ohai in this direction, and hence this concept can be generalized into all of Ohai without making a breaking change in the future.

jaym added 2 commits February 2, 2016 13:39
This is a hack. We can make this into a proper
DSL thing where specifying something like `default_enabled false`.

For now, this could be enabled through

```ruby
ohai[:plugins][:packages][:enabled] = true
```
@jaym jaym force-pushed the jdm/disable-packages-by-default branch from 0e58e8d to 97ac251 Compare February 2, 2016 21:39
@tyler-ball
Copy link
Contributor

👍

1 similar comment
@lamont-granquist
Copy link
Contributor

👍

jaym added a commit that referenced this pull request Feb 2, 2016
Don't enable packages plugin by default
@jaym jaym merged commit c151909 into master Feb 2, 2016
@jaym jaym deleted the jdm/disable-packages-by-default branch February 2, 2016 23:03
@thommay thommay added Type: Enhancement Adds new functionality. and removed Enhancement labels Jan 24, 2017
@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
Type: Enhancement Adds new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants