swap: add ReportIO option #814
Conversation
Add the new ReportIO option of collectd 5.8.
| @@ -6,13 +6,14 @@ | |||
| Boolean $reportbytes = true, | |||
| Boolean $valuesabsolute = true, | |||
| Boolean $valuespercentage = false, | |||
| Boolean $reportio = true, | |||
There was a problem hiding this comment.
For new options, I personally would prefer that they be Optional[Boolean] so that backward compatibility is preserved and we can eventually drop all of the if collected::version conditional logic. This does mean that you need to use something like == undef or defined() in templates though.
There was a problem hiding this comment.
Sorry I'm lost, I have done that in the past and I have been asked to do the reverse #800 (comment)
Can you see with @bastelfreak what you really want before I go further.
There was a problem hiding this comment.
From our review guide:
Are there new params with datatype Boolean? The default value is a tricky decision which needs careful reviewing. Sometimes a True/False is the better approach, sometimes undef
From an interface perspective it isn't a breaking change because the new parameter has a default value. Also it isn't written directly to the config file, only if collectd has the correct version, so the behavior isn't a breaking change either.
Instead of the collectd version check we could ensure it isn't undef. I'm not sure if we gain much when the parameter is undef. I'm a bit unsure here and would like to get more feedback from others.
There was a problem hiding this comment.
@sileht There's a few inconsistencies in this module and I'm refactoring large parts of it at the moment. Another thing that comes to mind is application defaults versus the defaults in this module. For instance, if the application default behaviour if the value is not in the configuration file is true, should this module explicitly put in the value in there? If so, what should happen in this module when a newer version of collectd does a breaking change switching the default behaviour to true/false?
Anyway, these consistency changes can be done as a separate Pull Request, so I am merging this for now.
swap: add ReportIO option
Add the new ReportIO option of collectd 5.8.