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

enable override of \$config[] values set in includes/definitions.inc.php #5096

Merged
merged 2 commits into from Dec 9, 2016

Conversation

Projects
None yet
5 participants
@barryodonovan
Contributor

barryodonovan commented Nov 30, 2016

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

@barryodonovan

This comment has been minimized.

Contributor

barryodonovan commented Nov 30, 2016

I'd also like to edit the docs to reflect this but am uncertain where it might fit...

@Gorian

This comment has been minimized.

Contributor

Gorian commented Nov 30, 2016

does overriding them in config.php not work? That seems like a bug if config.php variables don't take priority...

@laf

This comment has been minimized.

Member

laf commented Nov 30, 2016

My personal opinion is no to this.

This could increase support burden as users can now edit two different config files. It also means that everything in definitions is now changeable so when someone is talking about (as an example) the small overview graphs not working, we expect to see the ones in definitions but now the user could have changed them all to three other graphs. So overall this means we can no longer trust what we expect to see.

The reason this was needed was to change the device type of an OS. This is possible (per device) in the WebUI. Whilst not the perfect solution for this, it is a solution and one that doesn't come at a cost to the potential burden this could bring. We will never be everything to everyone and this strikes me as one of those areas where a specific need shouldn't be reason to merge something in.

@barryodonovan

This comment has been minimized.

Contributor

barryodonovan commented Dec 2, 2016

since the refactor to use includes/init.php, there is no longer the same error with calling includes/definitions.inc.php before config.php
I have edited my patch to do this instead and removed the local_definitions file

@barryodonovan barryodonovan changed the title from add local-definitions file to override \$config[] values to enable override of \$config[] values set in includes/definitions.inc.php Dec 2, 2016

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Dec 2, 2016

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

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Dec 2, 2016

The inspection completed: No new issues

@laf

This comment has been minimized.

Member

laf commented Dec 4, 2016

Running on my home install for a bit to test.

@laf

laf approved these changes Dec 7, 2016

@laf

laf approved these changes Dec 7, 2016

@laf

This comment has been minimized.

Member

laf commented Dec 7, 2016

All good with me.

@laf

This comment has been minimized.

Member

laf commented Dec 7, 2016

@librenms/reviewers

Merging 24 hours.

@laf laf merged commit fc941bc into librenms:master Dec 9, 2016

2 checks passed

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

VimCommando added a commit to VimCommando/librenms that referenced this pull request Jan 4, 2017

@barryodonovan barryodonovan deleted the barryodonovan:issue_5065 branch Feb 24, 2017

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