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

add support for sagemcom #9835

Merged
merged 8 commits into from Mar 1, 2019

Conversation

Projects
None yet
3 participants
@vitalisator
Copy link
Contributor

commented Feb 16, 2019

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.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

@TheGreatDoc
Copy link
Contributor

left a comment

There are at least one MIB that is already in the mibs directory (IF-MIB).

Check them to dont have duplicate MIB files.

Show resolved Hide resolved LibreNMS/Snmptrap/Handlers/EquipStatusTrap.php Outdated
Show resolved Hide resolved LibreNMS/Snmptrap/Handlers/EquipStatusTrap.php Outdated
Show resolved Hide resolved LibreNMS/Snmptrap/Handlers/LogTrap.php Outdated
@TheGreatDoc

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2019

Thanks for your PR.

Awesome work.

vitalisator added some commits Feb 16, 2019

Show resolved Hide resolved html/images/os/sagemcom.svg Outdated
ucd-mib: false
arp-table: false
bgp-peers: false
vlans: false

This comment has been minimized.

Copy link
@murrant

murrant Feb 19, 2019

Member

Please don't disable all these modules like this. Some of these don't even exist others won't actually cause any additional load, others are disabled by default. You may add ones that have a tangible benefit.

$logAI = $trap->getOidData('LOG-MIB::logAI.'.$index);
$state = $trap->getOidData('LOG-MIB::logEquipStatusV2.'.$index);
if ($state == 'warning' || $state == 'major' || $state == '5' || $state == '3') {

This comment has been minimized.

Copy link
@murrant

murrant Feb 19, 2019

Member

You could actually do this with a single array and probably extract to a function

    $severity = $this->getSeverity($state);

    private function getSeverity($state)
    {
        $severity_map = [
            'warning' => 4,
            'major' => 4,
            '5' => 4,
            '3' => 4,
            'critical' => 5,
            ...
        ];
        return $severity_map[$state] ?? 0;
    }
} else {
$severity = 0;
}
log_event('SNMP Trap: Equipment Status ' . $state, $device->toArray(), 'state', $severity, $device->hostname);

This comment has been minimized.

Copy link
@murrant

murrant Feb 19, 2019

Member

The last parameter should not be hostname, just omit it. The device is already referenced.

@murrant

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

Looks pretty good overall! just a few small code style changes and the icon.

@vitalisator

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

please review/merge.

murrant added some commits Feb 22, 2019

@TheGreatDoc
Copy link
Contributor

left a comment

LGTM. What do you think @murrant ?

@vitalisator

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

@murrant , would be fine to go forward here :-)

@murrant murrant merged commit 49f0197 into librenms:master Mar 1, 2019

5 of 6 checks passed

codeclimate Code Climate encountered an error attempting to analyze this pull request.
Details
Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

funzoneq added a commit to funzoneq/librenms that referenced this pull request Apr 30, 2019

add support for sagemcom (librenms#9835)
* adds support for sagemcom

* requested changes

* delete unused variable

* logos

* modules fine tuning

* code style

* Update sagemcom.svg

* Update sagemcom.svg

@lock lock bot locked as resolved and limited conversation to collaborators Apr 30, 2019

@vitalisator vitalisator deleted the vitalisator:sagemcom branch May 12, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.