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

haproxy: default to version 0.9.0 + use long option #179

Closed
wants to merge 1 commit into from

Conversation

mika
Copy link

@mika mika commented Mar 21, 2018

Starting with the following commit in https://github.com/prometheus/haproxy_exporter:

| commit cb544ab842c398242fc5e22053da49dda3058aa0
| Author: Calle Pettersson [email protected]
| Date: Sat Aug 12 13:17:26 2017 +0100
|
| Switch to kingpin

it's no longer "-haproxy.scrape-uri=" but
"--haproxy.scrape-uri=" instead (and the
short option isn't supported anymore, failing
with "error: unknown short flag '-a', try --help").

The relevant change is included in haproxy_exporter releases
starting with >=v0.8.0, so while at it default to the latest
stable version 0.9.0.

@bastelfreak
Copy link
Member

Hi @mika. thanks for the PR. Can you please rebase and add a test for this?

@mika
Copy link
Author

mika commented Apr 5, 2018

What kind of test are you exactly expecting?

@bastelfreak
Copy link
Member

I thought a bit about this. Users won't have the option to install older haproxy_exporter versions because $option is an internal variable, so they can't switch from --haproxy.scrape-uri= back to -haproxy.scrape-uri=. Is that something that should be implemented?

@mika
Copy link
Author

mika commented May 8, 2018

Using a separate option instead of handling this via (overloading) $options sounds like a good idea to me.

@bastelfreak
Copy link
Member

@mika in the meantime we agreed on adding support for new exporters, but staying at the current version of them. This allows us to do more frequent releases without major changes. One example for a test is present at: https://github.com/voxpupuli/puppet-prometheus/blob/master/spec/acceptance/prometheus_server_spec.rb

It installs prometheus several times, with different versions.

Let us know if you need any help with beaker or the acceptance tests. We're almost 24/7 available in the #voxpupuli IRC channel on freenode and on https://puppetcommunity.slack.com

Starting with the following commit in https://github.com/prometheus/haproxy_exporter:

| commit cb544ab842c398242fc5e22053da49dda3058aa0
| Author: Calle Pettersson <[email protected]>
| Date:   Sat Aug 12 13:17:26 2017 +0100
|
|     Switch to kingpin

it's no longer "-haproxy.scrape-uri=" but
"--haproxy.scrape-uri=" instead (and the
short option isn't supported anymore, failing
with "error: unknown short flag '-a', try --help").

The relevant change is included in haproxy_exporter releases
starting with >=v0.8.0, so while at it default to the latest
stable version 0.9.0.
@bastelfreak
Copy link
Member

Fixes #261

@joaquin386
Copy link

if( versioncmp($version, '0.8.0') < 0 ){
$options = "--haproxy.scrape-uri="${cnf_scrape_uri}" ${extra_options}"
} else {
$options = "-haproxy.scrape-uri="${cnf_scrape_uri}" ${extra_options}"
}

Or something like that

@dhoppe
Copy link
Member

dhoppe commented Dec 3, 2018

@bastelfreak I do not think we need an additional test, because this should be already handled by:
https://github.com/voxpupuli/puppet-prometheus/blob/master/spec/classes/haproxy_exporter_spec.rb#L27

I agree with @joaquin386 that an if/else should do the trick and make sure that this module is backwards compatible.

@bastelfreak
Copy link
Member

Hi. I'm going to close this PR. The current version of the haproxy_exporter that we install is 0.9.0 and we already use the new options. Please reopen and rebase the PR if you're still interested in getting this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants