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

consul_exporter improvement for version 0.4.0 and above #264

Merged
merged 5 commits into from
Oct 14, 2018

Conversation

RogierO
Copy link

@RogierO RogierO commented Sep 28, 2018

Pull Request (PR) description

With the release of version 0.4.0 of the consul exporter, the single dashes ( - ) has been changed to double dashes ( -- ).
( [CHANGE] Adopt standard Prometheus flags library (mostly double dashes)) #66

For the manifest consul_exporter.pp the following improvements have been made:

  • A versioncmp has been added to the $consul_health_summary variable. If the version of consul exporter is lower than 0.4.0, it wil use the consul.health-summary flag with a single dash. For versions of 0.4.0 or higher it will use the double dash.
  • A versioncmp has been added to the $options variable, for the same reason as above.

This Pull Request (PR) fixes the following issues

n/a

@bastelfreak
Copy link
Member

Thanks for this @RogierO! Could you add a tiny acceptance test that installs and old version and afterwards a new one? You can find some examples here: https://github.com/voxpupuli/puppet-prometheus/tree/master/spec/acceptance

@bastelfreak bastelfreak added enhancement New feature or request needs-tests labels Sep 29, 2018
describe 'prometheus consul exporter version 0.3.0' do
it ' consul_exporter installs with version 0.3.0' do
pp = "class {'prometheus::consul_exporter': version => '0.3.0' }"
apply_manifest(pp, catch_failures: true)
Copy link
Member

Choose a reason for hiding this comment

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

can you here and in line 24 add apply_manifest(pp, catch_changes: true)? We always need to test for idempotency

@@ -1,4 +1,4 @@
require 'spec_helper_acceptance'
#require 'spec_helper_acceptance'
Copy link
Member

Choose a reason for hiding this comment

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

can you explain why you did this?

@bastelfreak bastelfreak added needs-work not ready to merge just yet tests-fail and removed needs-tests labels Oct 5, 2018
@RogierO
Copy link
Author

RogierO commented Oct 9, 2018

@bastelfreak I've added the spec test for the consul_exporter, including the adjustments you've told me in the comment above.

apply_manifest(pp, catch_changes: true)
end
describe process('consul_exporter') do
its(:args) { is_expected.to match %r{\ --consul.server} }
Copy link
Member

Choose a reason for hiding this comment

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

ah nice, I haven't seen this test in a while. Good job!

@bastelfreak bastelfreak removed needs-work not ready to merge just yet tests-fail labels Oct 14, 2018
@bastelfreak
Copy link
Member

Thanks!

@bastelfreak bastelfreak merged commit 529dc40 into voxpupuli:master Oct 14, 2018
cegeka-jenkins pushed a commit to cegeka/puppet-prometheus that referenced this pull request Aug 28, 2019
consul_exporter improvement for version 0.4.0 and above
Rovanion pushed a commit to Rovanion/puppet-prometheus that referenced this pull request May 5, 2021
consul_exporter improvement for version 0.4.0 and above
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants