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.

Sign up for GitHub

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

Jump to bottom

Checks should use libraries to minimize duplication of code #65

Open
majormoses opened this issue Jun 4, 2017 · 5 comments
Open

Checks should use libraries to minimize duplication of code #65

majormoses opened this issue Jun 4, 2017 · 5 comments
Labels
Help Wanted

Comments

@majormoses
Copy link
Member

Currently there is a lot of copy/paste code between checks (and metrics) let's move as much as we can into library modules. The most obvious example is connection logic.
@majormoses majormoses mentioned this issue Jun 4, 2017
Closed
@rthouvenin
Copy link
Contributor

I have started to write tests here: https://github.com/rthouvenin/sensu-plugins-rabbitmq/tree/refactor

rthouvenin added a commit to rthouvenin/sensu-plugins-rabbitmq that referenced this issue Jul 24, 2017
@rthouvenin
Add base class for metrics (sensu-plugins#65)
9917867
@rthouvenin
Copy link
Contributor

And in the same branch I've started the refactor: master...rthouvenin:refactor

Now before I continue, I'd be keen on having feedback on whether this is in the right direction. I am not a ruby programmer and this is fairly abstract code, so I may not know the idioms for that kind of tasks.

Also, I doubt I will have the time to refactor+test all other checks, and I believe we should refactor only what's tested. Moreover I don't use the other checks so someone who actually uses them might be able to better spot missed use cases.
Once it is ready for it, should we merge this and test+refactor checks as they are updated, or should we involve more people so that we refactor everything?

@majormoses
Copy link
Member Author

@rthouvenin took a quick look and it so far looks fairly reasonable, the one bit of feedback is to split out the different classes and modules to separate files to make it easier to navigate. Regarding the testing and looping in others we should have someone else also review but I am of the stance that if you got something good going it's best to get that incremental change rather than wait for someone to donate some massive amount of effort. Makes sense we do run an external rabbitmq cluster as our sensu transport so we can probably help with some testing even if we are not currently using all the checks.

rthouvenin added a commit to rthouvenin/sensu-plugins-rabbitmq that referenced this issue Aug 10, 2017
@rthouvenin
Add base class for metrics (sensu-plugins#65)
d97dc11
@rthouvenin
Copy link
Contributor

Alright thanks. I have separated the module and classes in separate files, and I am starting a PR that can be review / tested in more details.

@rthouvenin rthouvenin mentioned this issue Aug 10, 2017
Merged
5 tasks
@majormoses
Copy link
Member Author

perfect I will try to take a look tonight.

majormoses pushed a commit that referenced this issue Sep 6, 2017
@rthouvenin @majormoses
Move duplicated code to libraries (#71)
2c5f865 
* Add plugin stub for tests

* Add tests for queue metrics

* Add tests for exchange metrics

* Add tests for queue check

* Add base class for metrics (#65)

* Add common check class

* Move common options into a module

* Common rabbitmq_info getter

* Fix Rubocop offenses

* Split module and classes in separate files

* Update Changelog

* Remove require_relative

* Use ruby_dig for older rubies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Help Wanted
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mbbroberg @majormoses @rthouvenin