refactor: small fixes for cisco-voice code #4719

Merged
merged 4 commits into from Oct 13, 2016

Projects

None yet

4 participants

@laf
Member
laf commented Oct 5, 2016

Please note

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

Just some tidying up. Maybe @adaniels21487 could test / peer review?

@laf laf refactor: small fixes for cisco-voice code
8aa02e1
@laf laf fixing some small issues
ba72567
@adaniels21487

Hi @laf
Just a couple of little things.

- $active += $value[''];
+ $output = snmpwalk_cache_oid_num($device, "1.3.6.1.4.1.9.9.86.1.2.1.1.6", null);
+ if (is_array($output)) {
+ foreach ($option as $key => $value) {
@adaniels21487
adaniels21487 Oct 5, 2016 edited Contributor

This should be:
foreach ($output as $key => $value) {

@@ -16,7 +16,7 @@
$total = snmpwalk_cache_oid_num($device, "1.3.6.1.4.1.9.9.86.1.7.1.0", null);
$total = $total['1.3.6.1.4.1.9.9.86.1.7.1.0'][''];
- if (isset($total) && ($total != "") && ($total != 0)) {
+ if (isset($total) && $total > 0) {
// Available
$available = snmpwalk_cache_oid_num($device, "1.3.6.1.4.1.9.9.86.1.7.2.0", null);
$available = $available['1.3.6.1.4.1.9.9.86.1.7.2.0'][''];
@adaniels21487
adaniels21487 Oct 5, 2016 edited Contributor

You didnt change it, but down below I spot a bug:

dat_update($device, 'cisco-iosxcode', $tags, $fields);
should be:
data_update($device, 'cisco-iosxcode', $tags, $fields);

No one will be hitting this bug as on the calling code, the include is wrong:
includes/polling/cisco-voice.inc.php

include "cisco-voice/cisco-xcode.inc.php";
should be:
include "cisco-voice/cisco-iosxcode.inc.php";

@laf
Member
laf commented Oct 5, 2016

Thanks for the review @adaniels21487. Fixed those items now.

laf added some commits Oct 6, 2016
@laf laf rebased
1b19a4a
@laf laf rebased
0131b96
@laf
Member
laf commented Oct 7, 2016

rebased

@laf
Member
laf commented Oct 7, 2016

Rebased

@scrutinizer-notifier

The inspection completed: 3 new issues

@laf
Member
laf commented Oct 11, 2016

bump

@adaniels21487
Contributor

Hi @laf
I approved this a few days ago, does this still need something from me?

Thanks,
Aaron

@laf
Member
laf commented Oct 11, 2016

Nope, just needs someone to merge it :)

@laf laf merged commit 1868945 into librenms:master Oct 13, 2016

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laf laf deleted the laf:cisco-voice-changes branch Oct 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment