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

WIP: add configurable detach delay to logs #5307

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

laurazard
Copy link
Member

@laurazard laurazard commented Jul 30, 2024

- What I did

Hacky PR for demonstration/discussion purposes only.

Addresses: #5305

- How I did it

- How to verify it

  • docker run -d --name bork alpine sh -c "while true; do echo hi; sleep 1; done"
  • docker logs -f --delay 3 bork
  • (in another terminal) docker restart bork

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2024

Codecov Report

Attention: Patch coverage is 29.62963% with 19 lines in your changes missing coverage. Please review.

Project coverage is 61.41%. Comparing base (826fc32) to head (f632123).
Report is 10 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5307      +/-   ##
==========================================
- Coverage   61.45%   61.41%   -0.05%     
==========================================
  Files         299      299              
  Lines       20856    20882      +26     
==========================================
+ Hits        12818    12824       +6     
- Misses       7122     7142      +20     
  Partials      916      916              

@laurazard laurazard force-pushed the add-detach-delay-logs branch 3 times, most recently from 96b7b32 to a8844f8 Compare July 30, 2024 12:52
@@ -49,6 +54,7 @@ func NewLogsCommand(dockerCli command.Cli) *cobra.Command {
flags.BoolVarP(&opts.timestamps, "timestamps", "t", false, "Show timestamps")
flags.BoolVar(&opts.details, "details", false, "Show extra details provided to logs")
flags.StringVarP(&opts.tail, "tail", "n", "all", "Number of lines to show from the end of the logs")
flags.IntVarP(&opts.detachDelay, "delay", "d", 0, "Number of seconds to wait for container restart before exiting")
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could do what tail does and put this behind a separate -F option:

     -f      The -f option causes tail to not stop when end of file is reached, but rather to wait for additional data to be ap‐
             pended to the input.  The -f option is ignored if the standard input is a pipe, but not if it is a FIFO.

     -F      The -F option implies the -f option, but tail will also check to see if the file being followed has been renamed or
             rotated.  The file is closed and reopened when tail detects that the filename being read from has a new inode number.

             If the file being followed does not (yet) exist or if it is removed, tail will keep looking and will display the file
             from the beginning if and when it is created.

             The -F option is the same as the -f option if reading from standard input rather than a file.

So basically:

  • -f would only work on existing containers and exit when the container ends
  • -F would never exit, it would always just wait for the container, even if it doesn't exist at the time

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooh, I like that UX.

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.

3 participants