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

extra spec tests for redis_exporter #237

Merged
merged 5 commits into from
Jul 31, 2018

Conversation

blupman
Copy link

@blupman blupman commented Jul 17, 2018

Pull Request (PR) description

I have added a few tests for created resources in redis_exporter.pp

This Pull Request (PR) fixes the following issues

Copy link

@LongLiveCHIEF LongLiveCHIEF left a comment

Choose a reason for hiding this comment

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

I think this one will be good once the redundant spec is removed, and the archive spec is moved up and grouped with the installs binary spec.

it { is_expected.to contain_prometheus__daemon('prometheus_redis_exporter') }
it { is_expected.to contain_service('prometheus_redis_exporter') }
it { is_expected.to contain_user('redis-exporter') }
it { is_expected.to contain_file('/opt/redis_exporter-0.11.2.linux-amd64') }

Choose a reason for hiding this comment

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

This doesn't seem like a required resource.

Choose a reason for hiding this comment

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

This seems like it's covered by the spec on line 23 (the spec that tests that it installs the correct binary), and is not needed. The spec on line 23 is also more explicit, since it verifies that /opt/redis_exporter.... is a symlink to /usr/local/bin/redis_exporter binary install location.

You can remove this condition.

Copy link
Author

Choose a reason for hiding this comment

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

agreed, i have moved them up to 'install correct binary'

it { is_expected.to contain_service('prometheus_redis_exporter') }
it { is_expected.to contain_user('redis-exporter') }
it { is_expected.to contain_file('/opt/redis_exporter-0.11.2.linux-amd64') }
it { is_expected.to contain_archive('/tmp/redis_exporter-0.11.2.tar.gz') }

Choose a reason for hiding this comment

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

Same for this one, it seems like this doesn't fit the context of the spec you're adding tests for.

Choose a reason for hiding this comment

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

I think you should move this up with the installs correct binary spec, and it would be fine.

Copy link
Author

Choose a reason for hiding this comment

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

agreed, i have moved them up to 'install correct binary'

@bastelfreak
Copy link
Member

@blupman thanks for the PR! Can you please rebase against our latest master? That should fix the travis issues.

@bastelfreak
Copy link
Member

@LongLiveCHIEF can you please review this again and merge if you're happy with it?

@LongLiveCHIEF LongLiveCHIEF merged commit 834d6d5 into voxpupuli:master Jul 31, 2018
cegeka-jenkins pushed a commit to cegeka/puppet-prometheus that referenced this pull request Aug 28, 2019
Rovanion pushed a commit to Rovanion/puppet-prometheus that referenced this pull request May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants