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

fix: rrdtool web settings #6698

Merged
merged 4 commits into from May 30, 2017

Conversation

Projects
None yet
7 participants
@crcro
Contributor

crcro commented May 21, 2017

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

to fully implement web settings (or db config) the db configs should overwrite the global ones (from files).

discovered this issue when tried to change the rrdtool bin from web but it was still using the defaults.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot May 21, 2017

Thank you for submitting a PR @crcro! We have found the following @laf, @murrant and @f0o based on the history of these files to review this PR.

Thank you for submitting a PR @crcro! We have found the following @laf, @murrant and @f0o based on the history of these files to review this PR.

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant May 21, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented May 21, 2017

CLA assistant check
All committers have signed the CLA.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@murrant

@CrCo, this would be a change in behavior and require notification.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 21, 2017

Member

I'm against this 100%. Breaking anything is best avoided but when this can break rrdtool then that's a pretty bad thing to do.

v2 is aiming to prioritise db config over config.php but I don't believe we should change v1, it's been that way since db config was added and I don't know anyone else yet caught out by this.

The best option would be to just make an ajax call for the config options and test if they exist in config.php

Member

laf commented May 21, 2017

I'm against this 100%. Breaking anything is best avoided but when this can break rrdtool then that's a pretty bad thing to do.

v2 is aiming to prioritise db config over config.php but I don't believe we should change v1, it's been that way since db config was added and I don't know anyone else yet caught out by this.

The best option would be to just make an ajax call for the config options and test if they exist in config.php

@crcro

This comment has been minimized.

Show comment
Hide comment
@crcro

crcro May 21, 2017

Contributor

@laf ... i run this and doesn't break anything. without it settings the rrdtool path in web is useless, being overwriting always by default.inc.php or config.php (and then the docs should be changed as they say the preferred way to set rrdtool path is via web not via config.php).

Contributor

crcro commented May 21, 2017

@laf ... i run this and doesn't break anything. without it settings the rrdtool path in web is useless, being overwriting always by default.inc.php or config.php (and then the docs should be changed as they say the preferred way to set rrdtool path is via web not via config.php).

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 21, 2017

Member

That's different, the fix for rrdtool being in includes/defaults.inc.php is to remove it.

Changing the preference of db config over config.php is not something that should happen imho.

Member

laf commented May 21, 2017

That's different, the fix for rrdtool being in includes/defaults.inc.php is to remove it.

Changing the preference of db config over config.php is not something that should happen imho.

@crcro

This comment has been minimized.

Show comment
Hide comment
@crcro

crcro May 21, 2017

Contributor

@laf ... ok, reverted the config.php over db, and removed the rrdtool from defaults.inc.php

Contributor

crcro commented May 21, 2017

@laf ... ok, reverted the config.php over db, and removed the rrdtool from defaults.inc.php

@crcro crcro changed the title from fix: web settings to fix: rrdtool web settings May 21, 2017

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
Show outdated Hide outdated includes/mergecnf.inc.php
@@ -27,7 +27,7 @@
/**
* merge the database config with the global config
* Global config overrides db
* DB Config overwrites the files (to fully implement web settings)

This comment has been minimized.

@laf

laf May 21, 2017

Member

This needs reverting as the original text still remains true.

@laf

laf May 21, 2017

Member

This needs reverting as the original text still remains true.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant May 22, 2017

Member

@crcro if you want to set a setting in the webui, either don't set it in config.php or use unset() to unset the default.

Member

murrant commented May 22, 2017

@crcro if you want to set a setting in the webui, either don't set it in config.php or use unset() to unset the default.

@crcro

This comment has been minimized.

Show comment
Hide comment
@crcro

crcro May 24, 2017

Contributor

@murrant, @laf how would be the best approach to make the webui rrrdtool path setting functional?

Contributor

crcro commented May 24, 2017

@murrant, @laf how would be the best approach to make the webui rrrdtool path setting functional?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf May 24, 2017

Member

I've pushed an update to add a safeguard. @murrant I'm fine with this now, how about you?

Member

laf commented May 24, 2017

I've pushed an update to add a safeguard. @murrant I'm fine with this now, how about you?

@laf

laf approved these changes May 24, 2017

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier May 24, 2017

The inspection completed: No new issues

The inspection completed: No new issues

@laf laf merged commit 5af3048 into librenms:master May 30, 2017

3 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@crcro crcro deleted the crcro:fix-rrdtool-webui branch Jun 3, 2017

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 18, 2018

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

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

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