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: Move OS definitions into yaml files #5189

Merged
merged 16 commits into from Dec 23, 2016

Conversation

Projects
None yet
5 participants
@laf
Member

laf commented Dec 17, 2016

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.

Refactored OS definitions into yaml files so we aren't loading everything in $config.

Some things for now:

  1. Polling and discovery should function as normal but over time we should move some of the grunt of these into functions which use data from the yaml files overall making it easier to add support for new OS.

  2. We have to load all yaml files for the webui at present as we access various $config['os'][$os] values all over the show.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Dec 17, 2016

Auto-Deploy finished, Test PR at http://5189.ci.librenms.org or https://5189.ci.librenms.org

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Dec 18, 2016

Auto-Deploy finished, Test PR at http://5189.ci.librenms.org or https://5189.ci.librenms.org

@Gorian

Gorian approved these changes Dec 19, 2016

I like this idea, looks good!

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Dec 19, 2016

Auto-Deploy finished, Test PR at http://5189.ci.librenms.org or https://5189.ci.librenms.org

@murrant

This comment has been minimized.

Member

murrant commented Dec 20, 2016

What is the reason you don't remove everything from definitions.inc.php?
Also, why the change to .gitignore?

@laf

This comment has been minimized.

Member

laf commented Dec 20, 2016

I'll remove everything from definitions when it's ready for merging, I'm rebasing when we add support and I do it with a quick script to pull out what's in definitions.

.gitignore is because I use composer install for yaml repo which then installs dependencies into lib/yaml/vendor

@murrant

This comment has been minimized.

Member

murrant commented Dec 20, 2016

oh yeah, I changed the vendor in my .gitignore to /vendor for my snmp-object branch.

$data = Yaml::parse(file_get_contents($file));
} catch (ParseException $e) {
//$this->assertNotEmpty($data, "$file Could not be parsed");
$this->assertEmpty($e->getMessage(), "$file Could not be parsed");

This comment has been minimized.

@murrant

murrant Dec 22, 2016

Member

Perhaps fail another way here.
throw new PHPUnitException("$file Could not be parsed");
maybe...

@murrant

This comment has been minimized.

Member

murrant commented Dec 22, 2016

Can either merge this before or after #5216
If we merge it after, we won't have to move the library files around. (and you can test the new dependency docs) :)

Also, this should probably be a feature not "refactor" 😄

@laf

This comment has been minimized.

Member

laf commented Dec 22, 2016

I'm happy either way but this will need another rebase anyway after the next mib update.

Your PR is quite big though so checking might take a while :)

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Dec 23, 2016

Auto-Deploy finished, Test PR at http://5189.ci.librenms.org or https://5189.ci.librenms.org

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Dec 23, 2016

Auto-Deploy finished, Test PR at http://5189.ci.librenms.org or https://5189.ci.librenms.org

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Dec 23, 2016

The inspection completed: 631 Issues, 18 Patches

@laf

This comment has been minimized.

Member

laf commented Dec 23, 2016

Changed the web ui to no longer load all yaml files and only those the user has in the devices table. I.e if all devices are ios then only that file is loaded.

@laf laf merged commit f5a16be into librenms:master Dec 23, 2016

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@laf laf deleted the laf:yaml branch Dec 23, 2016

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