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

Fix Docker log tailing #1066

Merged
merged 7 commits into from
Jul 19, 2024
Merged

Fix Docker log tailing #1066

merged 7 commits into from
Jul 19, 2024

Conversation

gronka
Copy link
Contributor

@gronka gronka commented Jul 11, 2024

Change the long-lived process from supervisord to tail, and prepend tail lines with the name of the file.

Description

Fixes this issue:
#1048

Changes Made to Code:

  • run.sh

Related

Additional Notes

  • At startup, there will be a warning printed that some log files do not exist. They can be ignored; this seemed preferable to sending tail errors to /dev/null

Quality Checks

  • [basic testing] New code is 100% tested
  • [*] Code has been formated
  • [N/A] Code has been linted
  • [N/A] Docstrings for new methods have been added

Change the long-lived process from supervisord to tail, and prepend tail lines
with the name of the file.
@coveralls
Copy link

coveralls commented Jul 11, 2024

Coverage Status

coverage: 100.0%. remained the same
when pulling 18719c2 on gronka:fix-tail
into 71ea0c8 on tethysplatform:main.

@gronka
Copy link
Contributor Author

gronka commented Jul 18, 2024

I made some 2 changes recommended by @araglu. I think the variable name PREFIX is a much better name, and using sed with --unbuffered makes new lines output immediately, which is a significantly better experience when working locallly with docker-compose.

@swainn swainn requested review from swainn and sdc50 July 18, 2024 22:46
@swainn swainn added the docker label Jul 18, 2024
Copy link
Member

@swainn swainn left a comment

Choose a reason for hiding this comment

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

Looks good to me. I just updated with some changes from main that should allow the docker build to go through and the docker startup test.

@swainn
Copy link
Member

swainn commented Jul 18, 2024

Hey @gronka I just tested this locally and I'm seeing a lot of these errors and no helpful logging:

run.sh: line 2: $'\r': command not found
run.sh: line 3: syntax error near unexpected token `$'{\r''
run.sh: line 3: `tail_file() {

Looks like some carriage return characters are causing issues maybe?

@swainn
Copy link
Member

swainn commented Jul 18, 2024

Looks like some carriage return characters are causing issues maybe?

That was the problem I think. I opened the file in VSCode and it was show CRLF line endings. I changed it to LF, and it replaced all the line ending and is working now.

image

@swainn swainn self-requested a review July 18, 2024 23:58
Copy link
Member

@swainn swainn left a comment

Choose a reason for hiding this comment

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

Please change from CRLF to LF line endings.

@gronka
Copy link
Contributor Author

gronka commented Jul 19, 2024

@swainn Thanks for figuring it out. I program in linux, so I don't know where an \r could possibly come from. Maybe it had to do with rebasing after you updated my PR to main, and then I tried to update my own branch to main - you can see I ended up duplicating commit messages. I'm a bit lost on the process here, sorry.

I don't have CRLF in my files, so I'm not sure how to proceed. I just deleted my repo and cloned it, and I still don't see any CLRF.

Is this PR now a hidden branch in the tethysplatform repository?

@gronka
Copy link
Contributor Author

gronka commented Jul 19, 2024

I usually add * text=auto to .gitattributes, which might be nice here (probably not with * in this repo).

I think I was able to check out this PR branch with git fetch origin pull/1066/head:fix-tail. Is that correct? I still don't see any CRLF. dos2unix doesn't seem to change the file

@swainn
Copy link
Member

swainn commented Jul 19, 2024

I don't have CRLF in my files, so I'm not sure how to proceed. I just deleted my repo and cloned it, and I still don't see any CLRF.

Hey I think I figured out what was going on. I built the Docker image for testing using Docker Desktop in Windows, so I bet the line endings go changed when I checked it out in windows. I just tested as-is in linux and it worked great. Sorry for the confusion.

@swainn swainn merged commit fad47d9 into tethysplatform:main Jul 19, 2024
42 checks passed
@swainn swainn linked an issue Jul 19, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG - NGINX and Tethys Logs not output to container logs like we expect
3 participants