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 #901 by excluding the ReportRelative option #902

Closed
wants to merge 6 commits into from

Conversation

cyberkov
Copy link

Pull Request (PR) description

If the collectd version is 5.9.0, the ReportRelative option is not properly parsed. This unfortunately breaks the module on RedHat 8.1.

This Pull Request (PR) fixes the following issues

Fixes #901

@bastelfreak
Copy link
Member

Hi @cyberkov, thanks for the PR. I rebased your code and merged it at #907. Are you using the module on RHEL? Can you add Centos 8 / RHEL 8 to the metadata.json file?

@cyberkov
Copy link
Author

cyberkov commented Feb 8, 2020

Hi! Thanks a lot. Sure. I'll take care of it, probably on monday when I'm back in the office :-)

@cyberkov
Copy link
Author

Hello @bastelfreak!
I added RedHat 8 to the metadata but I wasn't sure whether it is necessary to add it to any spectests. The failing tests don't seem to be related to my change, but I am confident that the test in spec/classes/collectd_plugin_load_spec.rb will fail sooner or later.
Should I cover that case specifically or is this a thing that'll be covered some other time? :)

Thanks Hannes

@bastelfreak
Copy link
Member

The failing test was caused by a timeout on the docker daemon. I restarted the test. please have a look at spec/spec_helper_methods.rb. It contains two arrays with 7 as version number for CentOS, please update it as well:

diff --git a/spec/spec_helper_methods.rb b/spec/spec_helper_methods.rb
index ffbb578..0cdd12a 100644
--- a/spec/spec_helper_methods.rb
+++ b/spec/spec_helper_methods.rb
@@ -24,7 +24,7 @@ def all_supported_os_hash
       },
       {
         'operatingsystem' => 'CentOS',
-        'operatingsystemrelease' => ['7']
+        'operatingsystemrelease' => ['7', '8']
       },
       {
         'operatingsystem' => 'Ubuntu',
@@ -46,7 +46,7 @@ def baseline_os_hash
     supported_os: [
       {
         'operatingsystem' => 'CentOS',
-        'operatingsystemrelease' => ['7']
+        'operatingsystemrelease' => ['7', '8']
       }
     ]
   }

Also please feel free to enhance as many test files as you like :)

@bastelfreak bastelfreak added the needs-work not ready to merge just yet label Feb 11, 2020
@vox-pupuli-tasks
Copy link

Dear @cyberkov, 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
@vox-pupuli-tasks
Copy link

Dear @cyberkov, 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

@bastelfreak
Copy link
Member

ping @cyberkov :)

@cyberkov
Copy link
Author

ping @cyberkov :)

Hi!

Sorry I got distracted by recent events (I am in Austria) :)
I'll try to fix it as soon as I have enough time.

Cheers Hannes

@cyberkov cyberkov closed this Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs-work not ready to merge just yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error with Plugin "Load" with collectd-5.9 and RHEL-8
3 participants