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

(FACT-1111) Bring back `facter -p` #1033

Merged
merged 2 commits into from Jul 10, 2015

Conversation

@branan
Copy link
Member

branan commented Jul 10, 2015

The beast rises from beyond the grave to once again terrorize the
local villagers.

This brings back our circular dependency on Puppet. When the
--puppet or -p arguments are passed to Facter, it will attempt to
load Puppet's ruby code in order to find additional facts that may
have been pluginsunc to the agent.

@branan

This comment has been minimized.

Copy link
Member Author

branan commented Jul 10, 2015

[branan@branan-thinkpad build]$ bin/facter myfact

[branan@branan-thinkpad build]$ bin/facter -p myfact
2015-07-10 10:47:32.987521 WARN  puppetlabs.facter - Could not load puppet; some facts may be unavailable: cannot load such file -- puppet

[branan@branan-thinkpad build]$ RUBYLIB=/home/branan/proj/pl/puppet/lib bin/facter -p myfact
foobar
@branan

This comment has been minimized.

Copy link
Member Author

branan commented Jul 10, 2015

This needs a test at the acceptance layer still. Working on that now.

#include <internal/ruby/api.hpp>
#include <internal/ruby/module.hpp>

using namespace std;
using namespace facter::facts;

static const string load_puppet = "\

This comment has been minimized.

Copy link
@branan

branan Jul 10, 2015

Author Member

I could try to map this to the appropriate rb API constructs, but that is super gross and probably more fragile than just using rb_eval_string here

This comment has been minimized.

Copy link
@peterhuene

peterhuene Jul 10, 2015

Contributor

This could just be a const char* since we're only passing it directly to Ruby anyway. Although doesn't hurt to be a string, one less global constructor/destructor wouldn't be a bad thing :)

@whopper

This comment has been minimized.

Copy link
Contributor

whopper commented Jul 10, 2015

@branan I think facter -p will conflict with --no-custom-facts and --no-ruby - should we add a message to https://github.com/branan/facter/blob/bug/stable/fact-1111-facter-p/exe/facter.cc#L160 ?

@MikaelSmith

This comment has been minimized.

Copy link
Member

MikaelSmith commented Jul 10, 2015

Oh yeah, using those together should definitely error.

@branan

This comment has been minimized.

Copy link
Member Author

branan commented Jul 10, 2015

@whopper @MikaelSmith oh, yup. On it.

{
module mod(facts, paths);
if (initialize_puppet) {
try {
eval(load_puppet);

This comment has been minimized.

Copy link
@MikaelSmith

MikaelSmith Jul 10, 2015

Member

Have you checked that this works? You're setting $LOAD_PATH after initialize_search_paths is called from the module constructor. It may make sense to pass {} to the module constructor and call initialize_search_paths(paths) after loading puppet.

This comment has been minimized.

Copy link
@peterhuene

peterhuene Jul 10, 2015

Contributor

Might as well just keep it a Facter.reset followed by a Facter.search_external([Puppet[:pluginfactsdest]]) if ... in the ruby code snippet. That's assuming pluginfactsdest is a single directory; search_external expects an array.

This comment has been minimized.

Copy link
@MikaelSmith

MikaelSmith Jul 10, 2015

Member

That also makes sense.

Puppet.initialize_settings\n\
unless $LOAD_PATH.include?(Puppet[:libdir])\n\
$LOAD_PATH << Puppet[:libdir]\n\
end";

This comment has been minimized.

Copy link
@peterhuene

peterhuene Jul 10, 2015

Contributor

We still need to fix up pluginfactsdest, no?

This comment has been minimized.

Copy link
@joshcooper

joshcooper Jul 10, 2015

Member

That is both amazing and terrifying at the same time!

@peterhuene

This comment has been minimized.

Copy link
Contributor

peterhuene commented Jul 10, 2015

Other than noted above, 👍

@branan branan force-pushed the branan:bug/stable/fact-1111-facter-p branch from c54bd5a to d71efd8 Jul 10, 2015
@daenney

This comment has been minimized.

Copy link
Member

daenney commented Jul 10, 2015

So a feature is being introduced which is immediately marked as deprecated? It can't be that hard to teach people to use puppet facts instead...

@peterhuene

This comment has been minimized.

Copy link
Contributor

peterhuene commented Jul 10, 2015

@daenney It's technically not a new feature, just a reinstatement of a previously deprecated feature :) The help message matches what we have in Facter 2.x. We're fully restoring the previous functionality with a "TODO: completely rethink this in the future". Unfortunately puppet facts needs some overhaul before we can properly point to it as a viable replacement of facter -p.

@daenney

This comment has been minimized.

Copy link
Member

daenney commented Jul 10, 2015

@peterhuene Ah, I see. Sadness though.

@kylog

This comment has been minimized.

Copy link
Contributor

kylog commented Jul 10, 2015

@daenney many tears have been shed on this one.

It's hard to strike the balance between moving briskly into the future and easing transitions from the past. Not just here but in many places.

Sigh.

@branan branan force-pushed the branan:bug/stable/fact-1111-facter-p branch from d71efd8 to 06778c0 Jul 10, 2015
@puppetcla

This comment has been minimized.

Copy link

puppetcla commented Jul 10, 2015

CLA signed by all contributors.

throw po::error("no-ruby and custom-dir options conflict: please specify only one.");
}
if (vm.count("puppet") && vm.count("no-custom-facts")) {
throw po::error("puppet and no-custom-facts conflict: please specify only one.");

This comment has been minimized.

Copy link
@whopper

whopper Jul 10, 2015

Contributor

Might want to make this puppet and no-custom-facts options conflict...

This comment has been minimized.

Copy link
@branan

branan Jul 10, 2015

Author Member

Word. easy enough.

throw po::error("puppet and no-custom-facts conflict: please specify only one.");
}
if (vm.count("puppet") && vm.count("no-ruby")) {
throw po::error("puppet and no-ruby conflict: please specify only one.");

This comment has been minimized.

Copy link
@whopper

whopper Jul 10, 2015

Contributor

Same as above

@branan branan force-pushed the branan:bug/stable/fact-1111-facter-p branch from 06778c0 to 93034cb Jul 10, 2015
The beast rises from beyond the grave to once again terrorize the
local villagers.

This brings back our circular dependency on Puppet. When the
`--puppet` or `-p` arguments are passed to Facter, it will attempt to
load Puppet's ruby code in order to find additional facts that may
have been pluginsunc to the agent.
@branan branan force-pushed the branan:bug/stable/fact-1111-facter-p branch from 93034cb to 62bb8f8 Jul 10, 2015
@branan

This comment has been minimized.

Copy link
Member Author

branan commented Jul 10, 2015

@peterhuene addressed your issues.

@branan

This comment has been minimized.

Copy link
Member Author

branan commented Jul 10, 2015

  • I've confirmed I can load facts from plugindest and pluginfactdest when using -p but not without.
  • I've confirmed that adding -p does not break loading of external facts from ~/.facter/facts.d
  • I've confirmed that adding -p does not break loading of custom facts from the path provided by --custom-dir

There are probably a few more combinations of arguments I could teset, but I'm pretty confident this is behaving correctly

@MikaelSmith

This comment has been minimized.

Copy link
Member

MikaelSmith commented Jul 10, 2015

Acceptance tests coming in a separate commit?

@branan

This comment has been minimized.

Copy link
Member Author

branan commented Jul 10, 2015

@MikaelSmith same PR, new commit. Yup

@peterhuene

This comment has been minimized.

Copy link
Contributor

peterhuene commented Jul 10, 2015

👍

1 similar comment
@MikaelSmith

This comment has been minimized.

Copy link
Member

MikaelSmith commented Jul 10, 2015

👍

@branan

This comment has been minimized.

Copy link
Member Author

branan commented Jul 10, 2015

Test is pusehd up

@@ -0,0 +1,30 @@
test_name "facter -p loads facts from puppet"

This comment has been minimized.

Copy link
@MikaelSmith

MikaelSmith Jul 10, 2015

Member

Is there a good way to guard on finding puppet? I can envision test environments where we only build Facter to manually run acceptance.

This comment has been minimized.

Copy link
@whopper

whopper Jul 10, 2015

Contributor

skip_test "Could not find Puppet" if on(agent, "which puppet") == ""?

This comment has been minimized.

Copy link
@MikaelSmith

MikaelSmith Jul 10, 2015

Member

I'm ok merging this and leaving that for later, when we actually have a workflow setup.


step "Agent #{agent}: create custom fact"
agent.mkdir_p(custom_dir)
create_remote_file(agent, custom_file, "Facter.add(:custom) do setcode do 'custom' end end")

This comment has been minimized.

Copy link
@MikaelSmith

MikaelSmith Jul 10, 2015

Member

Single lines are nicer with {}.

@branan branan force-pushed the branan:bug/stable/fact-1111-facter-p branch from c6dc93a to 0e1975a Jul 10, 2015
@MikaelSmith

This comment has been minimized.

Copy link
Member

MikaelSmith commented Jul 10, 2015

👍

MikaelSmith added a commit that referenced this pull request Jul 10, 2015
@MikaelSmith MikaelSmith merged commit ccd26a6 into puppetlabs:master Jul 10, 2015
3 checks passed
3 checks passed
continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.17%) to 85.36%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.