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

Do not create FTL.port file anymore #1445

Merged
merged 2 commits into from
Sep 21, 2022
Merged

Do not create FTL.port file anymore #1445

merged 2 commits into from
Sep 21, 2022

Conversation

yubiuser
Copy link
Member

@yubiuser yubiuser commented Sep 21, 2022

By submitting this pull request, I confirm the following (please check boxes, eg [X]) Failure to fill the template will close your PR:

Please submit all pull requests against the development branch. Failure to do so will delay or deny your request

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?: 5


FTL used to write its API port to /run/FTL.port (or whatever was configured in pihole-FTL.conf as path of PORTFILE). This was meant as a way to allow core and web to get the current/configured port. However, this is a unnecessary step, because FTL will use the default port 4711 unless FTLPORT is set in pihole-FTL.conf. If this is set, web and core can simply take the value from this config file instead of doing a detour to FTL.port.

Corresponding PRs: pi-hole/web#2371 and pi-hole/pi-hole#4945

Documentation at: pi-hole/docs#778


While removing the code for the port file, I noticed we have an old FTLtest.sh which was not used within FTL but as a guidance for users how to interact with the API. I don't think it's much of a use today.

This template was created based on the work of udemy-dl.

Signed-off-by: Christian König <ckoenig@posteo.de>
Signed-off-by: Christian König <ckoenig@posteo.de>
@yubiuser yubiuser mentioned this pull request Sep 21, 2022
1 task
@DL6ER DL6ER merged commit 5748da8 into development Sep 21, 2022
@DL6ER DL6ER deleted the no_port branch September 21, 2022 19:01
@PromoFaux
Copy link
Member

and YYY

@yubiuser did you already do the core changes but forget to PR? I also need to do update docker stuff, too.

PromoFaux added a commit to pi-hole/docker-pi-hole that referenced this pull request Sep 24, 2022
Signed-off-by: Adam Warner <me@adamwarner.co.uk>
@yubiuser
Copy link
Member Author

I have the code and the branch, but did not open a PR yet because I have black installed locally and it "formatted" the edited test*.py file, making the review tougher than nececcary with a few hundred changed lines. Wanted to get the blackPR in first

@MichaIng
Copy link
Contributor

Tiny regression: When the port is changed in the config file, web UI and others will get a wrong port until FTL is restarted. ss would be another option to read out the actually used port, but requires some awk/sed action.

@rdwebdesign
Copy link
Member

rdwebdesign commented Sep 30, 2022

I notice the opposite:
Using the file as Port number source was causing a problem for the web interface.
When web interface tries to read the file after a restart, the file is available only a few milliseconds after the restart (FTL doesn't write the file right away).
The result is: web interface never finds the port immediately after a restart.

Edit:
Ohhhh... I saw what you mean.
If a user changes this manually and never restarts FTL, the value won't match with the current port.
This should never happen (users should restart pi-hole if they change the config), but I understand your concern.

@yubiuser
Copy link
Member Author

Tiny regression: When the port is changed in the config file, web UI and others will get a wrong port until FTL is restartet.

Good catch. However, I think we can tolerate this. I've never seen anyone changing the port in the wild.

@pralor-bot
Copy link

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

https://discourse.pi-hole.net/t/padd-broken-after-latest-updates/58461/2

@pralor-bot
Copy link

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

https://discourse.pi-hole.net/t/padd-broken-after-latest-updates/58461/5

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.

6 participants