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

Allow administrators to edit devices sysName #8149

Merged
merged 4 commits into from Feb 7, 2018

Conversation

Projects
None yet
4 participants
@remyj38
Contributor

remyj38 commented Jan 25, 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


  • New feature to allow administrators to edit the device sysName from edit page.
  • Fixing indentation of html/pages/device/edit/device.inc.php

remyj38 added some commits Jan 25, 2018

Allow administrators to edit devices sysName
Signed-off-by: Rémy Jacquin <remy@remyj.fr>
Fix html/pages/device/edit/device.inc.php identation
Signed-off-by: Rémy Jacquin <remy@remyj.fr>
@laf

This comment has been minimized.

Member

laf commented Jan 25, 2018

This will just revert back to what's found in polling / discovery.

Why allow this to be changed out of interest?

@remyj38

This comment has been minimized.

Contributor

remyj38 commented Jan 26, 2018

In my usage of LibreNMS, I have a lot of hosts which don't have SNMP (so don't have sysName).
For this kind of hosts, it's easier to find it if a sysName is set.

@laf

This comment has been minimized.

Member

laf commented Jan 26, 2018

In my usage of LibreNMS, I have a lot of hosts which don't have SNMP (so don't have sysName).
For this kind of hosts, it's easier to find it if a sysName is set.

Ok but this change allows it for anyone. For snmp based devices the value will just be overwritten. You need to look at html/pages/device/edit/snmp.inc.php, that file allows certain input boxes to be shown when snmp is disabled only which is what you want.

You also shouldn't have the edit pencil button next to the input, just allow it be changed if it's non-snmp based.

Last thing, admin users should be allowed to change this like you have, but also normal users with permission to edit the device. You can use device_permitted() to determine this.

remyj38 added some commits Feb 6, 2018

Moved sysName editing to snmp tab
Signed-off-by: Rémy Jacquin <remy@remyj.fr>
@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Feb 6, 2018

The inspection completed: 724 Issues, 36 Patches

@remyj38

This comment has been minimized.

Contributor

remyj38 commented Feb 6, 2018

c4700d3 follows your recommendations

@laf

laf approved these changes Feb 6, 2018

This looks good to me now, thanks for contributing these changes.

@murrant

This comment has been minimized.

Member

murrant commented Feb 7, 2018

Wouldn't mind allowing this for all devices, to allow users to set custom display names on anything.

This is a good start, thanks.

@murrant murrant merged commit 19a17ed into librenms:master Feb 7, 2018

2 checks passed

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

@murrant murrant removed the Blocker 🚫 label Feb 7, 2018

inetAnt added a commit to criteo-forks/librenms that referenced this pull request Mar 19, 2018

Allow administrators to edit devices sysName (librenms#8149)
* Allow administrators to edit devices sysName
Signed-off-by: Rémy Jacquin <remy@remyj.fr>

* Fix html/pages/device/edit/device.inc.php identation
Signed-off-by: Rémy Jacquin <remy@remyj.fr>

* Revert "Allow administrators to edit devices sysName"

This reverts commit 11b127b.

* Moved sysName editing to snmp tab

Signed-off-by: Rémy Jacquin <remy@remyj.fr>
@lock

This comment has been minimized.

lock bot commented May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

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