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 SysName to Oxidized view #10012

Merged
merged 17 commits into from Apr 2, 2019

Conversation

Projects
None yet
4 participants
@smiles1969
Copy link
Contributor

commented Mar 22, 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 10012
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.

smiles1969 added some commits Mar 22, 2019

@CLAassistant

This comment has been minimized.

Copy link

commented Mar 22, 2019

CLA assistant check
All committers have signed the CLA.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2019

Hi @smiles1969
Thanx for your PR. This patch can make sense in the default situation, but not if you have "force_ip_to_sysname" enabled in the LibreNMS instance. In that case you end up with 2 times the sysname displayed, which does not add any usefully information :

Capture d’écran 2019-03-23 à 00 55 42

May be you could replace this column by the IP address in case the "force_ip_to_sysname" config option is set. Or make the column hidden by default, so it is only enabled when needed.

smiles1969 added some commits Mar 25, 2019

@smiles1969

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

I have added the config checks as suggested

smiles1969 added some commits Mar 25, 2019

@murrant

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

@smiles1969 sorry, I missed this the first time but I just noticed you used strict comparison. A loose comparison would be better as the value may be null. Or simply just !Config::get('force_ip_to_sysname')

smiles1969 added some commits Mar 25, 2019

@smiles1969

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

I've changed to !Config::get instead of == loose comparison and tested on my local repository.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

@smiles1969 Could you have a look at Travis tests ?

smiles1969 added some commits Mar 26, 2019

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

Looks good. @librenms/reviewers if you have a test setup with oxidized and not force_ip_to_sysname, do not hesitate to give feedback.

@murrant

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

@smiles1969 thanks for all the changes, but I actually realized something when looking at this today.
We should always send the column data and have it exist, just hide it when appropriate

echo '<th data-column-id="sysname" data-visible="' . (!Config::get('force_ip_to_sysname')  ? 'true' : 'false') . '">SysName</th>';
Replace ifcheck with ternary operator
Always send column id tag, but hide SysName conditionally
Show resolved Hide resolved html/includes/functions.inc.php Outdated
Show resolved Hide resolved html/pages/oxidized.inc.php Outdated

smiles1969 added some commits Apr 1, 2019

@murrant

murrant approved these changes Apr 2, 2019

@murrant

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Looks good :)

@murrant murrant merged commit c4fa327 into librenms:master Apr 2, 2019

2 checks passed

codeclimate All good!
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.