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

feature: Web validation #7474

Merged
merged 15 commits into from Oct 26, 2017

Conversation

Projects
None yet
3 participants
@murrant
Member

murrant commented Oct 11, 2017

Allow validations to be run and viewed from the web page.
Pre-check cannot be run on the web, because we won't get a web page if those don't work.
Make validations modular and hopefully easier to manage/write.

Complete, looking for testers to verify.

Note, must be updated for db schema changes because it alters the format of the schema file slightly.

TODO:

  • fix certain validations that fail when run under the webui
  • clickable links
  • Review new console output
  • go through and pull out or create fixes text
  • clean up code and standardize names

Probably future:
make fixes actionable (like a fix button)
make async instead of running on page load

image

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

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 15, 2017

Member

Great idea. Let us know when it's ready for proper testing / code review.

Member

laf commented Oct 15, 2017

Great idea. Let us know when it's ready for proper testing / code review.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 17, 2017

Member

PHP Fatal error: Cannot use LibreNMS\Config as Config because the name is already in use in /opt/librenms/LibreNMS/Validations/Database.php on line 28 - From ./validate.php

Member

laf commented Oct 17, 2017

PHP Fatal error: Cannot use LibreNMS\Config as Config because the name is already in use in /opt/librenms/LibreNMS/Validations/Database.php on line 28 - From ./validate.php

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 17, 2017

Member

image

Member

laf commented Oct 17, 2017

image

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 17, 2017

Member

image

-bash-4.2$ ll .pki/
total 4
drwxrw---- 2 librenms librenms 4096 Aug 22  2016 nssdb
Member

laf commented Oct 17, 2017

image

-bash-4.2$ ll .pki/
total 4
drwxrw---- 2 librenms librenms 4096 Aug 22  2016 nssdb

@laf laf added this to the 1.33 milestone Oct 17, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 17, 2017

Member

Seems like a few bugs for me. I run mysql strict mode on as an fyi.

Member

laf commented Oct 17, 2017

Seems like a few bugs for me. I run mysql strict mode on as an fyi.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 17, 2017

Member

I can't reproduce this:

PHP Fatal error: Cannot use LibreNMS\Config as Config because the name is already in use in /opt/librenms/LibreNMS/Validations/Database.php on line 28 - From ./validate.php

or the permission one, what are the permissions for the parent directory?

Fixed the output error, made some changes to cli output without checking web after.

Member

murrant commented Oct 17, 2017

I can't reproduce this:

PHP Fatal error: Cannot use LibreNMS\Config as Config because the name is already in use in /opt/librenms/LibreNMS/Validations/Database.php on line 28 - From ./validate.php

or the permission one, what are the permissions for the parent directory?

Fixed the output error, made some changes to cli output without checking web after.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 17, 2017

Member

I can't reproduce this:

PHP Fatal error: Cannot use LibreNMS\Config as Config because the name is already in use in /opt/librenms/LibreNMS/Validations/Database.php on line 28 - From ./validate.php

Still happens even pulling your latest changes in.

or the permission one, what are the permissions for the parent directory?

drwxrw---- 3 librenms librenms 4096 Aug 22 2016 .pki

Fixed the output error, made some changes to cli output without checking web after.

Better now.

The UI looks cluttered, more an observation at the moment as I don't have a suggestion to improve.

Member

laf commented Oct 17, 2017

I can't reproduce this:

PHP Fatal error: Cannot use LibreNMS\Config as Config because the name is already in use in /opt/librenms/LibreNMS/Validations/Database.php on line 28 - From ./validate.php

Still happens even pulling your latest changes in.

or the permission one, what are the permissions for the parent directory?

drwxrw---- 3 librenms librenms 4096 Aug 22 2016 .pki

Fixed the output error, made some changes to cli output without checking web after.

Better now.

The UI looks cluttered, more an observation at the moment as I don't have a suggestion to improve.

murrant added some commits Oct 17, 2017

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 18, 2017

Member

Yeah, just working on presenting the data right now. We could collapse all the groups initially, but we would need to add some extra visual cues that they can be expanded.

Member

murrant commented Oct 18, 2017

Yeah, just working on presenting the data right now. We could collapse all the groups initially, but we would need to add some extra visual cues that they can be expanded.

murrant added some commits Oct 18, 2017

fix some scrutinizer issues
remove as many local functions from validator.php as possible
move extensions from pre-check
remove duplicate timezone check
looks like there is some db schema differences between mariadb 10.1 and 10.2, investigating
Check schema version first for database.
Remove stop to go back to command line for install docs.
Add helpful link when there is no devices added to /addhost
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 21, 2017

Member

Not a fault of this PR but having just removed and re-installed php I got:

Array ( [0] => 1 [1] => Uncaught exception 'Exception' with message 'DateTime::__construct(): It is not safe to rely on the system's timezone settings. You are *required* to use the date.timezone setting or the date_default_timezone_set() function. In case you used any of those methods and you are still getting this warning, you most likely misspelled the timezone identifier. We selected the timezone 'UTC' for now, but please set date.timezone to select your timezone.' in /opt/librenms/LibreNMS/Validations/Updates.php:51 Stack trace: #0 /opt/librenms/LibreNMS/Validations/Updates.php(51): DateTime->__construct() #1 /opt/librenms/LibreNMS/Validator.php(69): LibreNMS\Validations\Updates->validate(Object(LibreNMS\Validator)) #2 /opt/librenms/html/pages/validate.inc.php(47): LibreNMS\Validator->validate() #3 /opt/librenms/html/index.php(215): require('/opt/librenms/h...') #4 {main} thrown [2] => /opt/librenms/LibreNMS/Validations/Updates.php [3] => 51 )

As I hadn't set the timezone. this leads to a totally blank page.

Member

laf commented Oct 21, 2017

Not a fault of this PR but having just removed and re-installed php I got:

Array ( [0] => 1 [1] => Uncaught exception 'Exception' with message 'DateTime::__construct(): It is not safe to rely on the system's timezone settings. You are *required* to use the date.timezone setting or the date_default_timezone_set() function. In case you used any of those methods and you are still getting this warning, you most likely misspelled the timezone identifier. We selected the timezone 'UTC' for now, but please set date.timezone to select your timezone.' in /opt/librenms/LibreNMS/Validations/Updates.php:51 Stack trace: #0 /opt/librenms/LibreNMS/Validations/Updates.php(51): DateTime->__construct() #1 /opt/librenms/LibreNMS/Validator.php(69): LibreNMS\Validations\Updates->validate(Object(LibreNMS\Validator)) #2 /opt/librenms/html/pages/validate.inc.php(47): LibreNMS\Validator->validate() #3 /opt/librenms/html/index.php(215): require('/opt/librenms/h...') #4 {main} thrown [2] => /opt/librenms/LibreNMS/Validations/Updates.php [3] => 51 )

As I hadn't set the timezone. this leads to a totally blank page.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 21, 2017

Member

Possible suggestion for the display:

https://p.libren.ms/view/raw/c523c7e8

image

Member

laf commented Oct 21, 2017

Possible suggestion for the display:

https://p.libren.ms/view/raw/c523c7e8

image

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 22, 2017

Member

I made that change plus added a text label. Honestly, I'm not that focused on the display, because I feel like it will be wrong and we'll need to tweak it anyway.

Member

murrant commented Oct 22, 2017

I made that change plus added a text label. Honestly, I'm not that focused on the display, because I feel like it will be wrong and we'll need to tweak it anyway.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Oct 22, 2017

Member

The webui change I proposed frames each error a bit better so you can distinguish between where one finishes and the other starts. Would you mind if I / you changed it to use it?

Member

laf commented Oct 22, 2017

The webui change I proposed frames each error a bit better so you can distinguish between where one finishes and the other starts. Would you mind if I / you changed it to use it?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 23, 2017

Member

@laf go ahead and push whatever you think to my branch.

Member

murrant commented Oct 23, 2017

@laf go ahead and push whatever you think to my branch.

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Oct 25, 2017

The inspection completed: 95 updated code elements

scrutinizer-notifier commented Oct 25, 2017

The inspection completed: 95 updated code elements

@@ -0,0 +1 @@
DELETE FROM `pollers` WHERE `poller_name` LIKE '%\n';

This comment has been minimized.

@laf

laf Oct 25, 2017

Member

I was just about to merge this until I saw this.

I'm not sure of the implications with this so I'm going to take a look at my work install tomorrow.

@laf

laf Oct 25, 2017

Member

I was just about to merge this until I saw this.

I'm not sure of the implications with this so I'm going to take a look at my work install tomorrow.

This comment has been minimized.

@murrant

murrant Oct 25, 2017

Member

This PR makes a change to pollers where poller_name is just the hostname. The old would have a line return at the end of the hostname. This now doesn't. It results in multiple entries for the same poller in the pollers table. Which isn't really used anywhere except validation and the poller display.

I wasn't sure how to clean those up, so I just did this since they will be repopulated next poller run.

@murrant

murrant Oct 25, 2017

Member

This PR makes a change to pollers where poller_name is just the hostname. The old would have a line return at the end of the hostname. This now doesn't. It results in multiple entries for the same poller in the pollers table. Which isn't really used anywhere except validation and the poller display.

I wasn't sure how to clean those up, so I just did this since they will be repopulated next poller run.

This comment has been minimized.

@murrant

murrant Oct 25, 2017

Member

perhaps I could edit poller wrapper to ignore the line return when checking if it exists and update that record with the new name.

@murrant

murrant Oct 25, 2017

Member

perhaps I could edit poller wrapper to ignore the line return when checking if it exists and update that record with the new name.

This comment has been minimized.

@murrant

murrant Oct 26, 2017

Member

@laf I reviewed options and came back to this.

  1. Update poller-wrapper.py and librenms-service.py to update the a name if not needed, but that adds unnecessary code in both spots.
  2. Do an update to trim the poller_name field. This could result in duplicate entries in the pollers table.
  3. Delete all entries that poler_name ends in a line return, they will be re-added next poller run with the correct poller_name.

3 seems the best to me.

@murrant

murrant Oct 26, 2017

Member

@laf I reviewed options and came back to this.

  1. Update poller-wrapper.py and librenms-service.py to update the a name if not needed, but that adds unnecessary code in both spots.
  2. Do an update to trim the poller_name field. This could result in duplicate entries in the pollers table.
  3. Delete all entries that poler_name ends in a line return, they will be re-added next poller run with the correct poller_name.

3 seems the best to me.

This comment has been minimized.

@laf

laf Oct 26, 2017

Member

Looking at the where it's used, 3 does seem fine

@laf

laf Oct 26, 2017

Member

Looking at the where it's used, 3 does seem fine

@murrant murrant removed the Needs Testing label Oct 26, 2017

@laf laf merged commit 51ba934 into librenms:master Oct 26, 2017

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 murrant:web-validations branch Oct 26, 2017

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 17, 2018

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

lock bot commented May 17, 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 17, 2018

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