feature: add logs cli details opt support and add more cli tests #2261
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2261 +/- ##
==========================================
+ Coverage 67.25% 67.26% +0.01%
==========================================
Files 213 213
Lines 17482 17511 +29
==========================================
+ Hits 11757 11779 +22
- Misses 4326 4329 +3
- Partials 1399 1403 +4
|
ZYecho
force-pushed
the
add-logs-detail
branch
from
14c7568 to
0f580bf
Compare
apis/server/container_bridge.go
Outdated
| return err | ||
| } | ||
|
|
||
| if c.HostConfig.LogConfig.LogOpts != nil { |
There was a problem hiding this comment.
I think the detail should be done in daemon/logger/logmessage.go, because the we don't want to split the logic into api bridge and daemon.
There was a problem hiding this comment.
OK I try to change it
ZYecho
force-pushed
the
add-logs-detail
branch
from
0f580bf to
95fac0a
Compare
|
@fuweid Any echo about the update? |
daemon/mgr/container_logs.go
Outdated
| @@ -136,10 +140,52 @@ func convContainerLogsOptionsToReadConfig(logOpt *types.ContainerLogsOptions) (* | |||
| lines = -1 | |||
| } | |||
|
|
|||
| var logDetailBuffer bytes.Buffer | |||
There was a problem hiding this comment.
IIRC, the tag information should be written into the json file.
daemon/mgr/container_logs.go
Outdated
| }, nil | ||
| } | ||
|
|
||
| func buildLogDetailStr(tags string, config map[string]string) string { |
There was a problem hiding this comment.
daemon/logger/loggerutils/tag.go:GenerateLogTag maybe can help you
|
Any update on this pull request? @ZYecho |
Sorry for the delay, because of the holiday you know. Today I'll update. |
ZYecho
force-pushed
the
add-logs-detail
branch
from
95fac0a to
98c286a
Compare
ZYecho
force-pushed
the
add-logs-detail
branch
from
98c286a to
4f62b49
Compare
ZYecho
force-pushed
the
add-logs-detail
branch
from
4f62b49 to
024cef0
Compare
daemon/logger/jsonfile/encode.go
Outdated
| var extra []byte | ||
| if len(attrs) > 0 { | ||
| var err error | ||
| extra, err = json.Marshal(attrs) |
There was a problem hiding this comment.
we can't write one line with json.Marshal here. it's too expensive. since the attrs value is static, could we marshal it before start logging?
ZYecho
force-pushed
the
add-logs-detail
branch
from
024cef0 to
a31b21e
Compare
ZYecho
force-pushed
the
add-logs-detail
branch
4 times, most recently
from
8609268 to
86e1351
Compare
ZYecho
force-pushed
the
add-logs-detail
branch
from
86e1351 to
2772489
Compare
| keyJ := strings.Split(ss[j], "=") | ||
| return keyI[0] < keyJ[0] | ||
| }) | ||
| logLine = append([]byte(strings.Join(ss, ",")+" "), logLine...) |
There was a problem hiding this comment.
There is a redundant space before log string if msg.Attrs is empty.
change the condition to if opt.Details && len(msg.Attrs) > 0
| out := suite.syncLogs(c, cname, "--details") | ||
| c.Assert(len(out), check.Equals, 1) | ||
| logFields := strings.Split(out[0], " ") | ||
|
|
There was a problem hiding this comment.
add a negative case: empty log option, but run pouch logs with --details
ZYecho
force-pushed
the
add-logs-detail
branch
from
2772489 to
421e4d0
Compare
Signed-off-by: zhangyue <[email protected]>
ZYecho
force-pushed
the
add-logs-detail
branch
from
421e4d0 to
183912b
Compare
|
Does the update meet your request? @zhuangqh |
LGTM but waiting for the CI. |
|
I retrigger the cri-tools CI to re-run it. |
|
Since all the CI passes, we will merge this. Thanks again for your great work. @ZYecho |
Signed-off-by: zhangyue [email protected]
Ⅰ. Describe what this PR did
As title mentioned, as two parts:
Ⅱ. Does this pull request fix one issue?
None.
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
add more cli integration test.
Ⅳ. Describe how to verify it
$ pouch run --name=test --label foo=bar -e baz=qux -e qq=we --log-opt labels=foo --log-opt env=baz,qq busybox echo helloRun
$ pouch logs --details testGot
baz=qux qq=we foo=bar helloⅤ. Special notes for reviews
None.