pi-hole / FTL

The Pi-hole FTL engine
https://pi-hole.net
Other
1.34k stars 187 forks source link

Add read-only config option #1988

Closed DL6ER closed 6 days ago

DL6ER commented 4 weeks ago

What does this implement/fix?

Add new misc.readOnly config option to force the configuration to be read-only. It can only be modified through the config file but neither the API nor the CLI as long as read-only mode is enabled.

Related issue or feature (if applicable): https://discourse.pi-hole.net/t/support-writing-to-pihole-toml-directly/70522/

Pull request in docs with documentation (if applicable): N/A


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

Checklist:

github-actions[bot] commented 3 weeks ago

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions[bot] commented 3 weeks ago

Conflicts have been resolved.

DL6ER commented 3 weeks ago

Has been confirmed working by original requestor on Discourse

DL6ER commented 3 weeks ago

Thank you, this was some very good review hint! The issue was that we skipped recomputing the checksum in read-only mode and, hence, FTL did not realize that the file actually changed once you went back from false -> true. However, this bug was only triggered if you have changed other settings before this, effectively hiding it from my tests where I have set misc.readOnly = true as the first thing during testing.

pralor-bot commented 6 days ago

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/support-writing-to-pihole-toml-directly/70522/11