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

Make the FTL log tail optional. Pass in TAIL_FTL_LOG: 0 to silence it #1385

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

PromoFaux
Copy link
Member

What does this PR aim to accomplish?:

See title. By default the image will tail the FTL log. Users can disable this if it is too noisy for their preference


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

Copy link
Contributor

@edgd1er edgd1er left a comment

Choose a reason for hiding this comment

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

If no logs are required may be pihole-FTL could be started et kept in foreground ('-f)

@PromoFaux
Copy link
Member Author

If we did that, then the container would stop if ever FTL was restarted/stopped.

For now at least I'd like to keep the command as-is

@sonarcloud
Copy link

sonarcloud bot commented Jul 23, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@edgd1er

This comment was marked as outdated.

@PromoFaux

This comment was marked as outdated.

@Gontier-Julien

This comment was marked as outdated.

@PromoFaux

This comment was marked as outdated.

@Gontier-Julien

This comment was marked as outdated.

@PromoFaux PromoFaux marked this pull request as draft July 25, 2023 16:37
@PromoFaux

This comment was marked as outdated.

@PromoFaux
Copy link
Member Author

If no logs are required may be pihole-FTL could be started et kept in foreground ('-f)

Something else I realised in doing some testing this evening. I am not sure what it is, but I remember it happening on v5 container too, if we start pihole-FTL in the foreground, it stops outputting to the docker log. Note there are more lines in the actual file than there are on the bottom half of the screen:

image

@DL6ER
Copy link
Member

DL6ER commented Jul 27, 2023

What is actually puzzling here is that it stops right in the middle of reporting the database statistics. They are simply a collection of printf statements, there is nothing in the code that could actually make it stop after the first line:

https://github.com/pi-hole/FTL/blob/5753b50df40defa15d16f6989e12a9ecf3439be9/src/log.c#L502-L512

@github-actions
Copy link

github-actions bot commented Sep 3, 2023

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

…environment variable TAIL_FTL_LOG with a value of 0. Defaults to enabled (1)

Signed-off-by: Adam Warner <[email protected]>
@github-actions
Copy link

github-actions bot commented Sep 3, 2023

Conflicts have been resolved.

@PromoFaux PromoFaux marked this pull request as ready for review September 3, 2023 12:04
@PromoFaux
Copy link
Member Author

Marked conversation about init systems as outdated due to #1406 being merged.

Starting FTL in the foreground leads to weirdness with the output where it just cuts out - see previous two comments. This works well enough. Either we tail the log or we don't. Simples.

@PromoFaux PromoFaux requested a review from a team September 3, 2023 12:07
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.

Needs documentation of the new env setting.

@PromoFaux
Copy link
Member Author

@yubiuser #1429

src/start.sh Outdated Show resolved Hide resolved
Co-authored-by: yubiuser <[email protected]>
Signed-off-by: Adam Warner <[email protected]>
@PromoFaux PromoFaux merged commit 93462c0 into development-v6 Sep 20, 2023
8 checks passed
@PromoFaux PromoFaux deleted the v6/quiet-output branch September 20, 2023 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants