Add support for NX-OS fan status #4824

Merged
merged 4 commits into from Oct 20, 2016

Projects

None yet

5 participants

@geordish
Contributor

Please note

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

@geordish geordish Add support for NX-OS fan status
f1560ae
@geordish geordish Fix bug scrutinizer found
006a68f
@geordish geordish Fix bug scrutinizer found
e19b907
+ * warning(4)
+ */
+
+ if (is_array($fan_trays)) {
@murrant
murrant Oct 18, 2016 edited Contributor

Do you need the is_array() here? foreach on an empty array does nothing.

@laf
laf Oct 18, 2016 Member

It's worth keeping if you move $entities = snmpwalk_cache_oid_num($device, $entity_oid, array()); below it so we don't run an unnecessary snmpwalk

@murrant
murrant Oct 19, 2016 Contributor

Agreed

@geordish
Contributor

I initially didn't have it. Scrutinizer complained about it, so I added it.

On 18 October 2016 at 22:00, Tony Murray notifications@github.com wrote:

@murrant commented on this pull request.

In includes/discovery/sensors/states/nxos.inc.php
#4824 (review)
:

+if ($device['os'] == 'nxos') {

  • $entity_oid = '1.3.6.1.2.1.47.1.1.1.1.7';
  • $entities = snmpwalk_cache_oid_num($device, $entity_oid, array());
  • $fan_tray_oid = '.1.3.6.1.4.1.9.9.117.1.4.1.1.1';
  • $fan_trays = snmpwalk_cache_oid_num($device, $fan_tray_oid, array());
  • /* CISCO-ENTITY-FRU-CONTROL-MIB cefcFanTrayOperStatus
  • \*  unknown(1),
    
  • \*  up(2),
    
  • \*  down(3),
    
  • \*  warning(4)
    
  • */
    
  • if (is_array($fan_trays)) {

Do you need the is_array() here, foreach on an empty array does nothing.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#4824 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABK4RVm1f-VRvd6h2PDWJI4cn6mlrRzjks5q1TNhgaJpZM4KZ00x
.

@laf
Member
laf commented Oct 18, 2016

Will test tomorrow.

@laf laf updated to move snmpwalk to inside the array check
683c648
@scrutinizer-notifier

The inspection completed: No new issues

@laf
Member
laf commented Oct 20, 2016

Tested fine but I've moved the additional snmp request so it wasn't run unnecessarily.

@laf laf merged commit 0774c52 into librenms:master Oct 20, 2016

2 checks passed

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