{WIP} (FM-7071) Adding custom facts for devices #105
Conversation
tphoney
force-pushed
the
custom_facts
branch
4 times, most recently
from
f261712 to
ebf6b6d
Compare
|
Comments welcome, points of note.
|
DavidS
left a comment
DavidS
left a comment
There was a problem hiding this comment.
The base approach is sound. I've added a couple comments around tightening up the implementation.
Nothing in this approach seems to preclude implementing it in a non-Resource API device module, right? This could be called out for completeness.
I'd also love to see an idea how we can avoid pasting the facts code into each device to enable this. Otherwise we'll create a maintenance nightmare if we ever want to move that forward.
Finally, how does this avoid loading custom facts for devices that do not match the current device's type?
| ``` | ||
| and the fact lives here EG: | ||
| ``` | ||
| /opt/puppet/cache/devices/3750/facts.d/ |
There was a problem hiding this comment.
/opt/puppetlabs/puppet/cache/ is the agent's default cache dir.
There was a problem hiding this comment.
This also needs to be reevaluated under the recent conversations about recommended deployment modes for puppet device running.
|
|
||
| ``` | ||
| class Example_ResourceAPI_Fact | ||
| def self.add_fact(connection, facts) |
There was a problem hiding this comment.
since this is not only adding facts, should it be rather named update_facts?
what do you expect as value for the connection parameter? Depending on the answer the parameter should be called context (a Resource API context), device (an instance of the device's Puppet::Util::NetworkDevice::*::Device class), or require more explanation.
| ``` | ||
| def facts | ||
| facts = { 'foo' => 'bar' } | ||
| custom_facts = return_custom_facts |
There was a problem hiding this comment.
What does return_custom_facts return?
From the code below it seems to be a list of file names, then it should be called list_custom_fact_implementations.
| puts "Error loading/executing custom fact:#{custom_fact}" | ||
| Puppet.log_exception(detail) | ||
| end | ||
| end |
There was a problem hiding this comment.
the rescue needs to be outdented to line up with the matching begin
| stdout_str, status = Open3.capture2e( | ||
| "puppet device #{common_args} --deviceconfig #{device_conf.path} --pluginfactdest #{fact_dir} --apply 'if $facts[\"baz\"] != \"foo\" { fail(\"fact not found\") }'", | ||
| ) | ||
| expect(stdout_str).not_to match %r{Error:} |
There was a problem hiding this comment.
Please add a positive assertion, checking for a message iff the fact is there.
| This method gives access to the context/connection used by the device and the facts hash. | ||
|
|
||
| ``` | ||
| class Example_ResourceAPI_Fact |
There was a problem hiding this comment.
The example here is not inheriting from the CustomFact base class. I have no preference either way, but either the example should use the class, or the class should not be part of this PR.
| begin | ||
| load custom_fact | ||
| jim = Example_ResourceAPI_Fact | ||
| bla = jim.add_fact(nil, facts) |
There was a problem hiding this comment.
should this be self, instead of nil?
|
|
||
| #### Custom facts for 'puppet device' - custom fact author | ||
|
|
||
| Adding a custom fact only requires implemented the add_fact method, and putting it into the correct directory. |
There was a problem hiding this comment.
@tphoney It's not yet clear to me what the step-by-step workflow is for the custom fact creator.
Questions:
- The current PR for cisco_ios seems to load facts from Example_ResourceAPI_Fact. This is obviously just an example, but what will be the general way of adding custom facts to Cisco IOS devices? Do I need to implement a custom provider + Device subclass for that, even if I just want to add a custom fact to an already supported device like cisco_ios?
- Where and how do I deploy the custom fact class implementation?
- How will custom facts work for non-Resource-API-based devices with agentless module support?
3 participants
No description provided.