Add dpdk_telemetry plugin #913
Conversation
manifests/plugin/dpdk_telemetry.pp
Outdated
| # | ||
| class collectd::plugin::dpdk_telemetry ( | ||
| Enum['present', 'absent'] $ensure = 'present', | ||
| Optional[String] $client_socket_path = '/var/run/.client', |
There was a problem hiding this comment.
Use Optional[Stdlib::Absolutepath] instead as this value is either undef or a path to a file.
There was a problem hiding this comment.
if it's an Optional[] datatype, the default value has to be undef. If you want to pass a default path, like /var/run/.client, it doesn't need to be Optional[] because you cannot pass undef to it. Same applies for $dpdk_socket_path
There was a problem hiding this comment.
The default value could be, but does not have to be undef. Since the value is either undef or a path to a file, then Stdlib::Absolutepath makes more sense than String.
There was a problem hiding this comment.
I changed String to Stdlib:Absolutepath and kept "Optional[...]" Is that all right?
There was a problem hiding this comment.
@ghoneycutt if you pass undef to a variable that has a default value, undef won't overwrite it. In my opinion the Optional[] needs to be removed.
There was a problem hiding this comment.
I removed Optional[] since, as @bastelfreak mentioned, we can't change it back to undef if we already have defined default value. Without Optional[] there is now no need for *_socket_path => undef test case.
There was a problem hiding this comment.
@ghoneycutt are you satisfied with applied modifications?
| end | ||
| end | ||
|
|
||
| context ':ensure => present and :dpdk_socket_path => /test/path/telemetry' do |
There was a problem hiding this comment.
add a context for ensure => absent
There was a problem hiding this comment.
Test case with "ensure => absent" added.
| { dpdk_socket_path: '/test/path/telemetry' } | ||
| end | ||
|
|
||
| it "Will create #{options[:plugin_conf_dir]}/10-dpdk_telemetry.conf" do |
There was a problem hiding this comment.
Since client_socket_path can be undef, there should be a test that shows the content when the param is undef. Same for dpdk_socket_path.
There was a problem hiding this comment.
I was looking into creating test case that you described but I stumbled upon obstacle. I wrote following test:
context ':ensure => present, :client_socket_path => undef and :dpdk_socket_path => undef' do
let :params do
{ client_socket_path: :undef,
dpdk_socket_path: :undef }
end
sth = <<EOS
# Generated by Puppet
<LoadPlugin dpdk_telemetry>
Globals false
</LoadPlugin>
<Plugin dpdk_telemetry>
</Plugin>
EOS
it "Will create #{options[:plugin_conf_dir]}/10-dpdk_telemetry.conf" do
is_expected.to compile.with_all_deps
is_expected.to contain_file('dpdk_telemetry.load').with(
ensure: 'present',
path: "#{options[:plugin_conf_dir]}/10-dpdk_telemetry.conf",
content: sth
)
end
end
Configuration is generated using specified default values rather than assuming that no value was provided.
I found interesting message by jcbollinger here, suggesting that assigning undef doesn't actually assign undef, only implies that no specified value is being assigned, and therefore default should be used.
I'm relatively new to Puppet, RSpec and Ruby so I can't be sure, however my observations suggests that it in fact might work that way. Additionally, I couldn't find this kind of test case in this repository. Are you sure that test case you described is possible to write? If so, could you give any suggestion how to proceed?
|
Thanks for the great contribution! |
bastelfreak
added
enhancement
|
Dear @prabiegx, thanks for the PR! This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
|
Dear @prabiegx, thanks for the PR! This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
1 similar comment
|
Dear @prabiegx, thanks for the PR! This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
prabiegx
force-pushed
the
collectd_dpdk_telemetry
branch
from
0ce966d
to
51ea4a1
Compare
|
Changes squashed and rebased. |
|
Is this ready for merge or needs further changes? |
|
Thanks for the PR @prabiegx ! |
The dpdk_telemetry plugin collects DPDK ethernet device metrics via dpdk_telemetry library.