-
-
Notifications
You must be signed in to change notification settings - Fork 242
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
Add support for mysqld_exporter version 0.11.0 #247
Conversation
manifests/mysqld_exporter.pp
Outdated
@@ -137,7 +137,7 @@ | |||
notify => $notify_service, | |||
} | |||
|
|||
$options = "-config.my-cnf=${cnf_config_path} ${extra_options}" | |||
$options = "--config.my-cnf==${cnf_config_path} ${extra_options}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change would make it backwards incompatible. Can you implement a selector statement that uses the old or the new syntax, depending on the version?
Hi @TheMeier, thanks for the PR. Can you please take a look at the inline comment I made? Also please rebase and take a look at the used email address. It isn't associated with your github account. |
@bastelfreak i am not so sure this can be done in a non backward-breaking way. The point is users could be using |
Also rebased.. |
ping |
Whatever we do here, we should probably also do on #179 |
data/defaults.yaml
Outdated
@@ -168,7 +168,7 @@ prometheus::mysqld_exporter::group: 'mysqld-exporter' | |||
prometheus::mysqld_exporter::package_ensure: 'latest' | |||
prometheus::mysqld_exporter::package_name: 'mysqld_exporter' | |||
prometheus::mysqld_exporter::user: 'mysqld-exporter' | |||
prometheus::mysqld_exporter::version: '0.9.0' | |||
prometheus::mysqld_exporter::version: '0.11.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should support the newer version, but leave the default alone? We could later update several exporter default versions in a single breaking change in a different PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like a good compromise to me. It users to upgrade withought changing any of the current behaviour.
Flags now use the Kingpin library, and require double-dashes. prometheus/mysqld_exporter#222 Breaking change: if you do not set a version for mysqld_exporter *and* set `extra_options` make sure you update `extra_options` to be compatible with version 0.11.0 and greater.
@TheMeier thanks for the updates. Can you please add an acceptance test that installs version |
@bastelfreak done |
@TheMeier Thanks! |
Flags now use the Kingpin library, and require double-dashes. prometheus/mysqld_exporter#222
Flags now use the Kingpin library, and require double-dashes. prometheus/mysqld_exporter#222
Pull Request (PR) description
Update mysqld_exporter version to 0.11.0
mysqld_exporter flags now use the Kingpin library, and require double-dashes. prometheus/mysqld_exporter#222
Breaking change: if you do not set a version for mysqld_exporter and set
extra_options
make sureyou update
extra_options
to be compatible with version 0.11.0 and greater.