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

memory check is done in % instead of KB #12

Merged
merged 4 commits into from Mar 22, 2016

Conversation

thapakazi
Copy link
Contributor

I have some humble changes here, to address my own issue

@analytically
Copy link

This breaks backwards compatibility. Would you be able to add two new options that make the check use % instead instead of replacing the kB ones. Thanks!

@thapakazi
Copy link
Contributor Author

@analytically sure !!
or for ease, we have have seperate check file like: check-redis-memory-percent.rb so the
What do you say ??

...my applogies didn't think of compatibility. 😲

@analytically
Copy link

No, best to keep it in the same file, but using options.

@analytically
Copy link

Actually, yes, put it in a separate check file.

@thapakazi
Copy link
Contributor Author

There is a small issue to adress regarding %, I will try push the changes asap.

P.S: @analytically how do you guys test the plugins, I am doing it with manual process with some hit-and-trial method. Any words of wisdom you want to share ?

@eheydrick
Copy link
Contributor

It would be great if you could add some specs. Beyond that, do a smoke test against a real instance.

@eheydrick
Copy link
Contributor

Any updates on this?

@thapakazi
Copy link
Contributor Author

@eheydrick kinda occupied, I will try my best push changes this weekend

Old file is reverted back.
@thapakazi
Copy link
Contributor Author

@eheydrick /cc @analytically
Pushed the changes on sperate file as check-redis-memory-percentage.rb and I have reverted the old file.

Here is snapshot results in my notification channel. that check threshold 2% is too clumsy... but i am getting lazy these days..
sensu_check_memory_result_

@envintus
Copy link
Contributor

@eheydrick, @zerOnepal Is this pull request still under consideration to be merged?

@thapakazi
Copy link
Contributor Author

@envintus i have pushed the changes..

head-mouse
wonder what's taking too long for it to merge...

#
# Released under the same terms as Sensu (the MIT license); see LICENSE
# for details.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure... my bad

@eheydrick
Copy link
Contributor

@zerOnepal can you add the new plugin to the CHANGELOG and README and we'll get this merged.

changes:
  1. added standard header
  2. edit: CHANGELOG and README.md
@thapakazi
Copy link
Contributor Author

@eheydrick done
happy_keed_playing_with_water

eheydrick added a commit that referenced this pull request Mar 22, 2016
memory check is done in % instead of KB
@eheydrick eheydrick merged commit 6178a15 into sensu-plugins:master Mar 22, 2016
@eheydrick
Copy link
Contributor

Thanks @zerOnepal!

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

Successfully merging this pull request may close these issues.

None yet

5 participants