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

Created a page to show all known VMs #8640

Merged
merged 12 commits into from May 21, 2018

Conversation

Projects
None yet
5 participants
@aldemira
Contributor

aldemira commented Apr 30, 2018

I've created single page which shows all VMs known to LibreNMS. Also I thought not everyone would like to enable this option so it requires $config['show_allvm_list'] = true; in config.php to be enabled as a menu item (under devices)

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

aldemira added some commits Apr 30, 2018

@jozefrebjak

Nice feature. I like it and I will be using it.

@murrant

This comment has been minimized.

Member

murrant commented May 5, 2018

I'd suggest counting the entries in the vminfo table instead of adding a config option.

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented May 7, 2018

The inspection completed: No new issues

@laf

Overall looks good although you will need to also have the check that the user accessing that page has access to the device the VM is related to.

Any reason why you are using sub queries? We try and just do JOINs normally and it looks like it would work.

Could you post a screenshot including the search element?

@aldemira

This comment has been minimized.

Contributor

aldemira commented May 10, 2018

@laf TBH I copied the SQL query from device dependencies ajax call. I'm going to fix that in a separate PR as well.
screen shot 2018-05-10 at 13 03 29
screen shot 2018-05-10 at 13 28 24

@laf

LGTM - Tested using a fake entry into the vminfo table which is all I could do.

@laf

This comment has been minimized.

Member

laf commented May 10, 2018

@aldemira Actually would be good if you could add a license to the new file + update the one you did add using this template; https://docs.librenms.org/#Developing/Licensing/

aldemira and others added some commits May 11, 2018

@laf

This comment has been minimized.

Member

laf commented May 19, 2018

@aldemira I've refactored the code a little bit. Can you test this patch and push it to this PR please:

https://p.libren.ms/view/3a29c5c3

@laf laf dismissed their stale review via f377302 May 21, 2018

@laf

laf approved these changes May 21, 2018

LGTM. Thanks for adding this @aldemira :)

@laf laf merged commit f389500 into librenms:master May 21, 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

@laf laf added Feature WebUI labels May 30, 2018

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

feature: Added VM overview page in devices menu (librenms#8640)
I've created single page which shows all VMs known to LibreNMS. Also I thought not everyone would like to enable this option so it requires $config['show_allvm_list'] = true; in config.php to be enabled as a menu item (under devices)

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.

- [x] Have you followed our [code guidelines?](http://docs.librenms.org/Developing/Code-Guidelines/)

#### Testers

If you would like to test this pull request then please run: `./scripts/github-apply <pr_id>`, i.e `./scripts/github-apply 5926`

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

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