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

Fixes #8347 - facts feature should not be enabled by default #237

Closed
wants to merge 1 commit into from

Conversation

orrabin
Copy link
Member

@orrabin orrabin commented Nov 16, 2014

No description provided.

@lzap
Copy link
Member

lzap commented Nov 18, 2014

To give you some background - facts_api is only used on the discovery image, this should been turned off by default. Anyway, the plugin is not loaded when facter gem is missing.

LGTM except the bundler.d change, we don't need this for discovery I think. But if @domcleal don't mind adding this new group let's have it there (we will likely need a packaging change tho).

@dmitri-d
Copy link
Member

The idea is to use the facter gem that is available on the system, not to install ours. Could you remove bundler.d/facts.rb? Looks good otherwise.

@domcleal
Copy link
Contributor

That's what bundler_ext is for, and this is the same pattern as in core. It looks correct to me.

@orrabin
Copy link
Member Author

orrabin commented Nov 19, 2014

@domcleal I removed bundler.d/facts.rb before seeing your comment.
@witlessbird what if facter gem isn't available on the system? shouldn't we install?

@dmitri-d
Copy link
Member

If facter isn't available we don't load the plugin: https://github.com/theforeman/smart-proxy/blob/develop/modules/facts/facts_plugin.rb#L8

@lzap
Copy link
Member

lzap commented Nov 20, 2014

Yes @witlessbird is right, I forgot. Looks sane, thank you. Merging as fc67f91.

@lzap lzap closed this Nov 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants