Only require vmstat on `#run` - Githubissues

sensu-plugins / sensu-plugins-memory-checks

This plugin provides native memory instrumentation for monitoring and metrics collection, including: memory usage via `free` and `vmstat`, including metrics. Note that this plugin may have cross-platform issues.

http://sensu-plugins.io

MIT License

15 stars 50 forks source link

Only require vmstat on `#run` #48

Closed PChambino closed 7 years ago

PChambino commented 7 years ago

Pull Request Checklist

General

[x] Update Changelog following the conventions laid out on Keep A Changelog
[ ] Update README with any necessary configuration snippets
[ ] Binstubs are created if needed
[ ] RuboCop passes
[ ] Existing tests pass

Purpose

check-ram.rb: Only require vmstat on #run

vmstat is being required twice: at the top of the file, and then inside the method #run. In order for check-ram.rb not to fail, vmstat should only be required inside the method #run.

Known Compatablity Issues

sstarcher commented 7 years ago

The plugin requires vmstat and all of our other plugins specify dependencies at the top. I would rather remove the block inside of the run method.

PChambino commented 7 years ago

Do we care about the error message? Or is a simple require enough?

sstarcher commented 7 years ago

I have not seen that pattern prior in any of the other sensu plugins. vmstat is listed as a dependency in the gem file. I would guess that block is an artifact when sensu-plugins were all in a single repo.

PChambino commented 7 years ago

It was actually added early this year 👉 7bd9fed33 Although, I also don't see this pattern in any of the plugins that I use, so I am ok with a simple require. I will update the PR.

sstarcher commented 7 years ago

@PChambino good catch.

@sensu-plugin @portertech @cwjohnston opinions from others?

PChambino commented 7 years ago

Opened an alternative PR instead of updating this one. #49

sstarcher commented 7 years ago

@PChambino sounds good we will merge one of these 2, but I want to get input from others before making the change.

majormoses commented 7 years ago

@sstarcher since its not in the gemspec intentionally because vmstat requires a c compiler and to prevent this requirement on users not using that check. I think we may still want to keep this check and messaging as that is more obvious than looking at the comment header. We can move it out of run and move to the top.

sstarcher commented 7 years ago

Lets go with this solution for now as it's an outlier.