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

refactor: Use snmp.version config option to allow users to set versions available #8512

Merged
merged 4 commits into from Apr 12, 2018

Conversation

Projects
None yet
4 participants
@laf
Member

laf commented Apr 5, 2018

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

$config['snmp']['version'] was unused before. This allows people to disable snmp versions they don't need.

@laf laf added the Refactor label Apr 5, 2018

@murrant

@laf Instead of setting in defaults, use the default option of Condig::get(). Put v2c first by default, as we try them in order, and trying other versions slows it down a lot. Plus we don't want to add devices by v1 if we can help it.

@murrant

This comment has been minimized.

Member

murrant commented Apr 6, 2018

Leave it in defaults, just commented.

@trcyberoptic

This comment has been minimized.

trcyberoptic commented Apr 6, 2018

initial refector works for me, thanks!

@laf

This comment has been minimized.

Member

laf commented Apr 6, 2018

@laf Instead of setting in defaults, use the default option of Condig::get(). Put v2c first by default, as we try them in order, and trying other versions slows it down a lot. Plus we don't want to add devices by v1 if we can help it.

Maybe I should just drop the webui support in this PR then I can default this back to just v2c but it at least allows people to customise it.

Why set the defaults in Config::get() as you'd have to do it each time it's called (which would only be once now but probably not in the future?)

@laf

This comment has been minimized.

Member

laf commented Apr 10, 2018

I've removed the addhost.php changes so this now maintains backwards compatibility.

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Apr 10, 2018

The inspection completed: No new issues

@murrant murrant merged commit 9b1a3fb into librenms:master Apr 12, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@laf laf deleted the laf:refactor/community-3700 branch Apr 12, 2018

TheMysteriousX added a commit to TheMysteriousX/librenms that referenced this pull request May 20, 2018

refactor: Use snmp.version config option to allow users to set versio…
…ns available (librenms#8512)

* refactor: Use snmp.version config option to allow users to set versions available

* Updated to just use snmp default version for auto discovery

* remove additional spaces

* remove use Config

@lock lock bot locked as resolved and limited conversation to collaborators Jun 11, 2018

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