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

Feature: Plugins in the Port page #8665

Merged
merged 6 commits into from May 22, 2018

Conversation

Projects
None yet
5 participants
@PipoCanaja
Contributor

PipoCanaja commented May 6, 2018

Added new plugin menu_option in the "port" page, which contain hook calling public function port_container($device, $port) in plugins.
The display of this menu_option is conditionnal, and enabled with the following option in config.php:
$config['enable_ports_plugins'] = 1;

pluginmenuoptionwithtestpluginenabled

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

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented May 6, 2018

The inspection completed: 4 new issues, 2 updated code elements

@murrant

This comment has been minimized.

Member

murrant commented May 7, 2018

@PipoCanaja Thanks for your PR.
Can you please link to an implementation? We are unlikely to merge something with nothing that uses it.

Also, because LibreNMS is an open source project, we are open to merging things directly into LibreNMS instead of requiring additional set up.

@PipoCanaja

This comment has been minimized.

Contributor

PipoCanaja commented May 7, 2018

Hello,

Basically, I have a plugin that adds a few "SNMP Write" and "CLI Write" commandes to LibreNMS, which allow our NOC to shut/no shut ports, clear PortSecurity sticky Mac Addresses, View dot1X status, change Description and VLAN.
This plugin is for the moment absolutely not portable (checks a specific description syntax to find if the port should be available for NOC, uses Huawei OIDs for specific POE enable/disable actions etc etc ...).

As adding RW capability was clearly out of LibreNMS scope, I thought of this simple extension of the PluginStructure so everybody can actually extend LibreNMS in this (or other) directions, without breaking the AutoUpdate capability. More hooks would probably help people create plugins for more features that are not meant to be embedded into LibreNMS.

What kind of implementation (in the test Plugin) would you like to see ? For the moment, as shown in the capture, I did exactly the same as the Test Plugin already did with the "Device" page : putting a simple text so that any future user would see where it's code would access to the GUI.

Edit: A simple "Shut/NoShut" plugin using IF-MIB is provided in the next comment.

Bye

@PipoCanaja

This comment has been minimized.

Contributor

PipoCanaja commented May 7, 2018

Hello @murrant

You'll find a test plugin that allows to Shut/No shut ports using the IF:MIB.

https://github.com/PipoCanaja/InterfaceAdminPlugin

If you would like to include it directly into LibreNMS, I can add it to the current Pull Request.

PipoCanaja

@laf

This comment has been minimized.

Member

laf commented May 10, 2018

I really wanted to get rid of the plugin system as it wasn't actively used but a few people keep popping up who do.

If we were to merge this then I would say that it would be good if you could add the plugin to https://github.com/librenms-plugins However you should also just use the existing device snmp creds rather than having another $config option that people have to set.

You should also remove the $config check, if you enable the plugin you shouldn't need to also setup a new config option.

@PipoCanaja

This comment has been minimized.

Contributor

PipoCanaja commented May 11, 2018

Hello Iaf

I don't know about the others, but I think that the plugin system is interesting to cover things that cannot/should not be included into librenms for any reason. It also allow some personnalisation without breaking the auto-update mechanism ...

Concerning the plugin, I'll push it as asked. For the community, the point is that the current SNMP Community configured in LibreNMS is RO in my deployment. That's why I used a different one (in a quick and dirty way, I must say). The "nice" way would probably be to have a new property, per device, with RW community.
But for the moment, I'll change to use the community stored in LibreNMS.

For the $config check, I need to find a way to "ask" the plugins about which functions are provided, and if no plugin provide a "port_container" function, then I don't need to display the "plugin" menu_option at all. For the moment, the $config option does that manually.

I'll review the code following those recommandation and come back to you.
Bye
PipoCanaja

@PipoCanaja

This comment has been minimized.

Contributor

PipoCanaja commented May 11, 2018

Hi @laf ,

I added a method in Plugins.php to count the number of Plugins implementing a specific hook. This allow to display the "Plugins" menu_option automatically as soon as at least a plugin implements it.

I also corrected the Plugin itself to use LibreNMS community instead of a specific $config new param.

And the push to https://github.com/librenms-plugins is on it's way.

Bye

@PipoCanaja

This comment has been minimized.

Contributor

PipoCanaja commented May 11, 2018

Hi again,

Cannot make it creating a PR to https://github.com/librenms-plugins . In fact, I cannot even clone it. Do I miss something or the rights do not allow me to clone it ?

If you like, you can directly pick https://github.com/PipoCanaja/InterfaceAdminPlugin and add it to the https://github.com/librenms-plugins .

Thanx
PipoCanaja

PipoCanaja added some commits May 6, 2018

Added new plugin menu_option in the "port" page, which contain hook c…
…alling public function port_container($device, $port) in plugins
New method in Plugins.php to allow counting all plugins implementing …
…a specific hook. This allow conditionnal display of the plugin menu_option in the port view.
@laf

This comment has been minimized.

Member

laf commented May 13, 2018

If you like, you can directly pick PipoCanaja/InterfaceAdminPlugin and add it to the @librenms-plugins .

I've cloned it, if this goes ahead then we'll give you access to the repo in librenms-plugins.

@kkrumm1

This comment has been minimized.

Member

kkrumm1 commented May 13, 2018

Looks good to me

@PipoCanaja

This comment has been minimized.

Contributor

PipoCanaja commented May 17, 2018

Hello @laf

2 checks are still in "expected" status. Is it something normal, could I do something to correct ? I would be very interested to get the PR merged so that I can get it into the "prod" machine instead of the test one ;)

Thanx
PipoCanaja

@laf laf dismissed their stale review via 4e26213 May 19, 2018

@laf

This comment has been minimized.

Member

laf commented May 19, 2018

I've just removed the ?> from one of the files to kick the checks off again.

@laf laf added the Feature label May 19, 2018

@PipoCanaja

This comment has been minimized.

Contributor

PipoCanaja commented May 19, 2018

Let me know if you need more documentation. The existing device_overview_container was missing and I added the port_container as well.

@PipoCanaja

This comment has been minimized.

Contributor

PipoCanaja commented May 21, 2018

Hello @kkrumm1 @laf @murrant ,
I am wondering if this PR would have a chance to get merged this week. Sorry to push but I would love to clean up the prod librenms with this before being out for a couple of days of holidays.
Thanx and bye
PipoCanaja

@murrant

This comment has been minimized.

Member

murrant commented May 22, 2018

I'm kind of meh on the concept, but I'm not going to block it.

@laf

This comment has been minimized.

Member

laf commented May 22, 2018

I'm going to merge it in and see where this goes. I did want the plugin system dropped as it's not really used but if it is useful and can be expanded then I'm ok with running it for a bit longer before we decide.

@laf laf merged commit 5c83d43 into librenms:master May 22, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@PipoCanaja

This comment has been minimized.

Contributor

PipoCanaja commented May 22, 2018

Thanx !
Bye
PipoCanaja

@PipoCanaja PipoCanaja deleted the PipoCanaja:addPluginsHookInPortPage branch May 22, 2018

mattie47 added a commit to mattie47/librenms that referenced this pull request Jul 2, 2018

feature: Added plugin support in the Port page (librenms#8665)
* Added new plugin menu_option in the "port" page, which contain hook calling public function port_container($device, $port) in plugins

* Cleaning after pre-commit error

* New method in Plugins.php to allow counting all plugins implementing a specific hook. This allow conditionnal display of the plugin menu_option in the port view.

* Typo after rebase

* Update plugins.inc.php

* Updating the documentation with device_overview_container and port_container hooks.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 21, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.