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.

Sign up for GitHub

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

Make port and host optional for mysql plugin #988

Merged
merged 1 commit into from
Sep 21, 2021
Merged

Make port and host optional for mysql plugin #988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions manifests/plugin/mysql/database.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
define collectd::plugin::mysql::database (
Enum['present', 'absent'] $ensure = 'present',
String $database = $name,
Stdlib::Host $host = 'localhost',
Copy link
Member

Choose a reason for hiding this comment

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

The change in the data type is OK though the default value should not change. This ensures it is backwards compatible while still allowing you to specify things differently for your use case.

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 don't think I understand; how can you allow undef and have a default value?

Copy link
Member

Choose a reason for hiding this comment

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

A value with a default cannot be set to undef.

The underlaying question is: If I do not set a port, and if I set port to 3306, is the result the same? For what I understand, the default Port 3306 is now removed from the configuration, but if no port is the same as port 3306, it is not a breaking change because the bahaviour is the same.

If the behavior change (e.g. metrics are not collected, the name of the metric change, etc), then this is backward incompatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the port, you are correct; we could add back the default value there. For the host, however, the behavior is different...

Stdlib::Port $port = 3306,
Optional[Stdlib::Host] $host = undef,
Optional[Stdlib::Port] $port = undef,
Boolean $masterstats = false,
Boolean $slavestats = false,
Optional[String[1]] $username = undef,
Expand Down
39 changes: 38 additions & 1 deletion spec/classes/collectd_plugin_mysql_database_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@
end
end

context 'default options' do
context 'specify host and port' do
let :title do
'dbname'
end
Expand All @@ -84,6 +84,13 @@
facts.merge(collectd_version: '5.6')
end

let :params do
jovandeginste marked this conversation as resolved.
Show resolved Hide resolved
{
jovandeginste marked this conversation as resolved.
Show resolved Hide resolved
'host' => 'localhost',
'port' => 3306,
}
end

it 'creates an mysql database' do
content_database_file = <<EOS
# Generated by Puppet
Expand All @@ -96,6 +103,36 @@
SlaveStats false
</Database>
</Plugin>
EOS
is_expected.to compile.with_all_deps
is_expected.to contain_class('collectd')
is_expected.to contain_class('collectd::plugin::mysql')
is_expected.to contain_file('dbname.conf').with(
content: content_database_file,
path: "#{options[:plugin_conf_dir]}/mysql-dbname.conf"
)
end
end

context 'default options' do
let :title do
'dbname'
end

let :facts do
facts.merge(collectd_version: '5.6')
end

it 'creates an mysql database' do
content_database_file = <<EOS
# Generated by Puppet

<Plugin mysql>
<Database "dbname">
MasterStats false
SlaveStats false
</Database>
</Plugin>
EOS
is_expected.to compile.with_all_deps
is_expected.to contain_class('collectd')
Expand Down
4 changes: 4 additions & 0 deletions templates/mysql-database.conf.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,21 @@

<Plugin mysql>
<Database "<%= @database %>">
<%- if @host -%>
Host "<%= @host %>"
<%- end -%>
<%- if @username -%>
User "<%= @username %>"
<%- end -%>
<%- if @password -%>
Password "<%= @password %>"
<%- end -%>
<%- if @port -%>
<% if scope.function_versioncmp([scope.lookupvar('collectd::collectd_version_real'), '5.0']) >= 0 -%>
Port "<%= @port %>"
<% else -%>
Port <%= @port %>
<%- end -%>
<%- end -%>
MasterStats <%= @masterstats %>
SlaveStats <%= @slavestats %>
Expand Down