-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
podman-remote logs: --timestamps is a NOP #15797
Comments
Since you already have a test in your PR feels free to apply this fix so I don't have to create another PR for one line: diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go
index 324802a1f..572807ad6 100644
--- a/pkg/domain/infra/tunnel/containers.go
+++ b/pkg/domain/infra/tunnel/containers.go
@@ -517,7 +517,7 @@ func (ic *ContainerEngine) ContainerLogs(_ context.Context, nameOrIDs []string,
stdout := opts.StdoutWriter != nil
stderr := opts.StderrWriter != nil
options := new(containers.LogOptions).WithFollow(opts.Follow).WithSince(since).WithUntil(until).WithStderr(stderr)
- options.WithStdout(stdout).WithTail(tail)
+ options.WithStdout(stdout).WithTail(tail).WithTimestamps(opts.Timestamps)
var err error
stdoutCh := make(chan string) |
Oh, nice, thank you! I will do so, but will wait until #15796 merges. |
@Luap99 the timestamps I get now are 1-second resolution. With plain podman, I get sub-second:
Is this intentional? Is it fixable? |
I have no idea. The timestamp is set on the server so it is part of the API, changing this format could be considered breaking?
vs Line 205 in 83c148c
Line 22 in 83c148c
|
Looks like docker is using the local podman format: I guess we could change the remote format to the local to better match docker? |
Fixed in #15794 but the mergebot didn't seem to catch my edit. |
Looks like nobody has actually ever tested
podman logs --timestamps
.Expected:
The text was updated successfully, but these errors were encountered: