Skip to content
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

Updates to reflect FTL's native capability of reading the FTLCONF_ env vars #1460

Merged
merged 7 commits into from
Oct 29, 2023

Conversation

PromoFaux
Copy link
Member

What does this PR aim to accomplish?:

Now that pi-hole/FTL#1679 (review) has been merged, we no longer need to parse the environment variables in the startup script (with the exception of FTLCONF_webserver_api_password, which we will set to a random value if none is specified) and totally negates the need for the changes made in #1456 to add special cases for FTL config names with _ in them

Password scenarios:

Env var set (With or without the addition of FTLCONF_ENV_ONLY)

ph-development-v6  |   [i] Checking web password
ph-development-v6  |   [i] Assigning password defined by Environment Variable

No env var set and pwhash empty in config file

ph-development-v6  |   [i] Checking web password
ph-development-v6  |   [i] No password set in environment or config file, assigning random password: iK1me016
ph-development-v6  |   [✓] New password set

No env var set and pwhash has a value in config file

ph-development-v6  |   [i] Checking web password
ph-development-v6  |   [i] Password already set in config file

No env var set, but FTLCONF_ENV_ONLY set to true

ph-development-v6  |   [i] Checking web password
ph-development-v6  |   [i] No password supplied via FTLCONF_webserver_api_password, but FTLCONF_ENV_ONLY is set to true, using default (none)

We also check the value of FTLCONF_dns_upstreams for ; and replace them with , to ensure compatibility with FTL's environment variable parsing. A notice will be displayed to the user in this case.

ph-development-v6  |   [i] Replacing ';' with ',' in FTLCONF_dns_upstreams - please update the environment variable to use commas instead of semicolons

Lastly some formatting/old code removal has also been applied


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 and I have tested my changes.
  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)

  • I have read the above and my PR is ready for review. Check this box to confirm

@PromoFaux
Copy link
Member Author

Forgot the tests. PR now ready

Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L70 of the README: add port 443

README.md Outdated Show resolved Hide resolved
src/start.sh Outdated Show resolved Hide resolved
@PromoFaux
Copy link
Member Author

L70 of the README: add port 443

Reading over that section again, I might even just drop most of it.

README.md Outdated Show resolved Hide resolved
@yubiuser
Copy link
Member

It's not the arm64 tests that misreport, but amd64.

The reported issue is

chown: cannot access '/etc/pihole/logrotate': No such file or directory\r\nchmod: cannot access '/etc/pihole/logrotate': No such file or directory

This comes from

sh /opt/pihole/pihole-FTL-prestart.sh

Which triggers

https://github.com/pi-hole/pi-hole/blob/6cf39d9b924adf2c8bd49c7f4997b2ffff9e0a53/advanced/Templates/pihole-FTL-prestart.sh#L19-L21

Those lines were just added recently by pi-hole/pi-hole#5444.

So the test are failing rightfully, because on docker, we don't copy /etc/pihole/logrotate. You might want to touch these files on docker and add arm64 back ;-)


(No idea why adm64 passed, but they should not.

@yubiuser
Copy link
Member

My previous assumption was wrong, but lead me to find out what really went wrong. It's the check for a set password using FTL itself.

if [[ -n $(pihole-FTL --config webserver.api.pwhash) ]]; then

It only checks, if there is an response at all. This is the case if FTL returns the password, but also on errors. The underlying issue here is: the test pulled the wrong architecture, because TARGETPLATFORM in the Dockerfile was not set.

I forked the repo and fixed the tests:

yubiuser@7e8712d

https://github.com/yubiuser/docker-pi-hole/actions/runs/6597244291


I think you should re-add the tests, they helped to identify this bug.
Additionally, you could not only check for non-emptyness, but for a string starting with

nanopi@nanopi:~$ pihole-FTL --config webserver.api.pwhash
$BALLOON-SHA256....

@PromoFaux
Copy link
Member Author

Thanks for looking at that @yubiuser - reverted the commit removing the arm tests and added in the additional commit from your fork

PromoFaux and others added 3 commits October 22, 2023 17:42
- Update readme to take into accounts changes to FTLs environment variable handling
- shell/md linting, tidy away some code that is no longer needed

Signed-off-by: Adam Warner <[email protected]>
Co-authored-by: yubiuser <[email protected]>
Signed-off-by: Adam Warner <[email protected]>
README.md Outdated Show resolved Hide resolved
src/bash_functions.sh Outdated Show resolved Hide resolved
PromoFaux and others added 2 commits October 22, 2023 22:14
Signed-off-by: Adam Warner <[email protected]>
Co-authored-by: yubiuser <[email protected]>
Signed-off-by: Adam Warner <[email protected]>
@PromoFaux PromoFaux requested review from yubiuser and a team October 28, 2023 18:53
Copy link
Member

@DL6ER DL6ER left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work as expected

@PromoFaux PromoFaux merged commit 76e45a5 into development-v6 Oct 29, 2023
8 checks passed
@PromoFaux PromoFaux deleted the v6/FTLCONF_ branch October 29, 2023 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants