wernerfred / check_synology

This plugin will check a lot of different values on your Synology DiskStation.
MIT License
17 stars 25 forks source link

Bring back option to configure target SNMP port #23

Closed amotl closed 5 months ago

amotl commented 2 years ago

Hi,

A custom port can be specified by using -p. The default value is 161.

I just discovered that the refactoring coming from #4/#19 dropped that feature to specify the target SNMP port number (#2), so we will have to bring it back when there is demand.

Also, after integrating #21, the Icinga configuration snippets will provide access to almost all parameters this program has to offer. However, the -p option is still missing there. Also depending on community demand, this can be added every time.

With kind regards, Andreas.

wernerfred commented 2 years ago

Good catch, I would like to have this functionality back in the script (still with default so that one does not have to use it). Adding it to the example files is a good idea, too 👍🏻

amotl commented 2 years ago

I will try to take care about it within one of the next iterations.

wernerfred commented 2 years ago

Current README also lists setting the port as an option. Should remove the part from the README in the meantime to not confuse new users?

wernerfred commented 2 years ago

As #31 now got merged i would like to publish a new release. But as the option to configure a target port is still a regression it would be a major version. So i am drafting a feature version bump as a pre release and release "normal" one as soon as this regression is solved.

EDIT: published pre-release v0.4.0-beta.1

amotl commented 2 years ago

Dear Frederic,

I will re-add this option in the course of the next iterations. That's why I wanted to add #39 ff. beforehand.

In the meanwhile, as I found out just yesterday, everyone can use a composite host syntax like appliance.example.net:1161, just adding the port number to the host name. Easy SNMP accepts that as well.

With kind regards, Andreas.

wernerfred commented 1 year ago

Braindump (for future me or others): The corresponding option for the easysnmp Session should be remote_port.

bigitag commented 1 year ago

I added the missing port definition to easysnmp.Session in #46