Graph for services would not work #5569

Merged
merged 3 commits into from Jan 25, 2017

Projects

None yet

6 participants

@nerdalertdk
Contributor

I agree to the conditions of the Contributor Agreement contained in doc/General/Contributing.md.

Just found an bug, when adding an new service is sets the server_ds to {} See here, end of line L46
And here we check if service is equal to {} and not empty

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

  • [x ] Have you signed the Contributors agreement - please do NOT submit a pull request unless you have (signing the agreement in the same pull request is fine). Your commit message for signing the agreement must appear as per the docs.
  • [ x] Have you followed our code guidelines?
@nerdalertdk nerdalertdk Graph for services would not work
I agree to the conditions of the Contributor Agreement contained in doc/General/Contributing.md.

Just found an bug, when adding an new service is sets the server_ds to {} [See here, end of line L46](https://github.com/librenms/librenms/blob/d5296319fbadd41dc5a82cbe4bbc6e3fa82b1cb7/includes/services.inc.php#L46)

And here we check if service is equal to {} and not empty
aa0240d
@mention-bot

Thank you for submitting a PR @nerdalertdk! We have found the following @adaniels21487, @murrant and @infotek based on the history of these files to review this PR.

@murrant
Contributor
murrant commented Jan 24, 2017

Are you sure you don't need to check for both "" and "{}"?

@nerdalertdk
Contributor

Don't but all new services get the {} on insert

@laf
Member
laf commented Jan 24, 2017

I have legacy services where service_ds is blank so yes, a check for both should be included.

nerdalertdk added some commits Jan 25, 2017
@nerdalertdk nerdalertdk Update services.inc.php
check for {} and empty
aa5860f
@nerdalertdk nerdalertdk Update services.inc.php
FFS
f8f0bea
@scrutinizer-notifier

The inspection completed: No new issues

@nerdalertdk
Contributor

Changed it but blanks should be working in the current version as long as the nagios service is returning graph data

@laf laf merged commit e53503c into librenms:master Jan 25, 2017

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment