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

Fix formatting in python plugin template #282

Merged
merged 2 commits into from
Jul 5, 2015

Conversation

ttarczynski
Copy link
Contributor

Fix for: #281

@ttarczynski
Copy link
Contributor Author

The failed tests show that the python module should be used with the double quotes included in parameters values:

  context 'spam module' do
    let(:title) { 'spam' }
    let :params do
      { 
        :config     => { 'spam' => '"wonderful" "lovely"' },
        :modulepath => '/var/lib/collectd/python',
      }
    end
      should contain_concat__fragment('collectd_plugin_python_conf_spam').with({
        :content => /spam "wonderful" "lovely"/,
      })

I think this behavior has changed since version 3.3.0 and it would help if we have a note in the documentation.
Should I revert the double quotes change? Then I would only add new lines after each param and a short note in docs about the quotes.

@blkperl
Copy link
Member

blkperl commented Jun 29, 2015

@deric or @arioch Did your recent PR break this?

So is this a difference between versions of collectd then?

@ttarczynski
Copy link
Contributor Author

I think collectd configuration syntax hasn't changed.
I've meant a change in puppet-module-collectd between version 3.3.0 and 3.4.0

The rspec code (collectd_plugin_python_module_spec.rb) shows configuration similar to the man page example: collectd-python. But there is the missing newlines issue (#281)

@deric
Copy link
Contributor

deric commented Jun 30, 2015

Nope, that wasn't me. The line ending was removed in #267.

@arioch
Copy link
Contributor

arioch commented Jun 30, 2015

#267 fixed not getting any python based data. It's still happily running in production over here.

<% end %>
<%- @config.sort.each do |key,value| -%>
<%- if value.is_a?(String) -%>
<%= key %> "<%= value %>"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be careful, this doesn't check if string is already quoted. Have a look at the specs.

<%- else -%>
<%= key %> <%= value %>
<%- end -%>
<%- end -%>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be replaced by <% end %> if you don't wanna re-introduce #267.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think #267 was caused by a missing newline after </Module>

@deric
Copy link
Contributor

deric commented Jun 30, 2015

@arioch sure, but removing line endings after key-value pair (<%= key -%> <%= value -%>) wasn't necessary.

@ttarczynski
Copy link
Contributor Author

I'm not sure if I'm using the pull request correctly. I've just added another commit, to change only newlines (and not quotes).

@arioch @deric I think the code on master works ok for a single parameter in Module block, but it doesn't for multiple params, e.g. #281

<% end %>
<%- @config.sort.each do |key,value| -%>
<%= key %> <%= value %>
<%- end -%>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replace this by <% end %>, otherwise there won't be new line between multiple modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested with multiple modules.
It results with config file:

# Generated by Puppet
<Plugin "python">
  ModulePath "/usr/lib64/collectd"
  LogTraces false
  Interactive false

  Import "elasticsearch"

  <Module "elasticsearch">
    Cluster test
    Verbose false
    Version 1.0
  </Module>

  Import "rabbitmq"

  <Module "rabbitmq">
    Host "localhost"
    Password "ppp"
    Port "15672"
    PrefixVhost true
    Realm "RabbitMQ Management"
    Username "uuu"
  </Module>

</Plugin>

I think newlines between modules looks ok here.
It's done with ensure_newline used in manifests/plugin/python.pp
and #267 fixed missing newline at the end of file: pull/267/files

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deric when I replace <%- end -%> by <% end %> I get extra (not needed) new line before </Module> and wrong indentation in the <Module> block:

Notice: /Stage[main]/Collectd::Plugin::Python/Concat[/etc/collectd.d/python-config.conf]/File[/etc/collectd.d/python-config.conf]/content: 
--- /etc/collectd.d/python-config.conf  2015-06-30 09:45:40.752012515 +0200
+++ /tmp/puppet-file20150703-10516-16ym6qz-0    2015-07-03 08:50:02.977012923 +0200
@@ -8,19 +8,21 @@

   <Module "elasticsearch">
     Cluster opslogstash
-    Verbose false
-    Version 1.0
+      Verbose false
+      Version 1.0
+  
   </Module>

   Import "rabbitmq"

   <Module "rabbitmq">
     Host "localhost"
-    Password "ppp"
-    Port "15672"
-    PrefixVhost true
-    Realm "RabbitMQ Management"
-    Username "uuu"
+      Password "ppp"
+      Port "15672"
+      PrefixVhost true
+      Realm "RabbitMQ Management"
+      Username "uuu"
+  
   </Module>

 </Plugin>

@deric
Copy link
Contributor

deric commented Jul 3, 2015

Ok, cool. Thanks for debugging this.

@blkperl
Copy link
Member

blkperl commented Jul 3, 2015

So is this ready to be merged then?

@deric
Copy link
Contributor

deric commented Jul 4, 2015

Yeah, good to merge. I've tested this on a configuration with multiple python modules. Indentation and new lines works fine. Tested on puppet 3.8.1

blkperl added a commit that referenced this pull request Jul 5, 2015
Fix formatting in python plugin template
@blkperl blkperl merged commit 85e230c into voxpupuli:master Jul 5, 2015
@ttarczynski ttarczynski deleted the python-formatting branch July 6, 2015 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants