Skip to content

remove inherit from exporters to not trigger a install of prometheus daemon#187

Closed
blupman wants to merge 4 commits intovoxpupuli:masterfrom
TMGMedia:master
Closed

remove inherit from exporters to not trigger a install of prometheus daemon#187
blupman wants to merge 4 commits intovoxpupuli:masterfrom
TMGMedia:master

Conversation

@blupman
Copy link
Copy Markdown

@blupman blupman commented Apr 26, 2018

I have run into issue #184 and have removed the inherits from a few exporters to test our theory in no longer automaticaly 'inheriting' the prometheus daemon, upon installing a exporter.

If this is a acceptable sollution, i can expand the merge request to change all the other exporters in the same way.

Thanks,

Niels Jansen

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

@bastelfreak
Copy link
Copy Markdown
Member

HI @blupman, thanks for the PR. I really like the adjustments to the hiera data and the updated datatypes. However, I'm not sure if (lookup) is the right approach. Shouldn't the main class simply have a parameter like $manage_service. You set it to false and the server won't be installed? Getting rid of inheritance is fine, but the main class should always be in the catalog.

@bastelfreak bastelfreak added enhancement New feature or request needs-work not ready to merge just yet needs-feedback Further information is requested labels May 8, 2018
@blupman
Copy link
Copy Markdown
Author

blupman commented May 8, 2018

Hi @bastelfreak, If the main class is required to be loaded, should we add a
parameter to not manage prometheus. Should it be called: install_prometheus?
This would require the user to set it to false for all machines where only the
some exporter needs to be installed.

This would make a breaking change for the current puppet forge module users, if
the want to prevent haveing the prometheus server installed on all machines
where currently only exporteres are installed.

One other sollution would be to have a sepporate prometheus 'server' sub class
which actualy installs the prometheus server. this would also be quite a
breaking change in the way we need to configure the prometheus service.

I'm not sure of a sollution, to make the prometheus puppet module behave the
same as in the current puppet forge module.

So thats actualy why i'm not sure on how to proceed.

@bastelfreak
Copy link
Copy Markdown
Member

I need to do some local testing, but I think it was possible in the past to install an exporter without the prometheus server. I've 4 boxes here that only have the node_exporter.

@bastelfreak
Copy link
Copy Markdown
Member

I debugged this. Turns out I broke it during a refactoring. Example: https://github.com/voxpupuli/puppet-prometheus/pull/178/files#diff-589781ae49e81337f6a53b370d04c73dR164 inherits prometheus::params was changed to inherits prometheus.

@blupman
Copy link
Copy Markdown
Author

blupman commented May 10, 2018

Thanks for your work! Shall i change my pull request to sollution you approve? (and if yes, how shall i proceed?)

@bastelfreak
Copy link
Copy Markdown
Member

@blupman I think the best idea is to introduce a Boolean $manage_prometheus_server that defaults to false. It should wrap all the install logic in https://github.com/voxpupuli/puppet-prometheus/blob/master/manifests/init.pp. In addition some spec tests would be good that check that the actual server isn't installed when we only provision an exporter.

@bastelfreak
Copy link
Copy Markdown
Member

I'm going to close this is favour of #194

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

Labels

enhancement New feature or request needs-feedback Further information is requested needs-work not ready to merge just yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants