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

feature: finish the CLI logs part #1472

Merged
merged 1 commit into from
Jun 12, 2018
Merged

feature: finish the CLI logs part #1472

merged 1 commit into from
Jun 12, 2018

Conversation

fuweid
Copy link
Contributor

@fuweid fuweid commented Jun 5, 2018

Signed-off-by: Wei Fu [email protected]

Ⅰ. Describe what this PR did

Finish the CLI logs part in the pouch

Ⅱ. Does this pull request fix one issue?

Fixed #1371

Ⅲ. Describe how you did it

Since the API parts has been done, the CLI part is just to fill the logic. However, during the testing, we found the IO issue like #1471 .

Some case maybe fail and it will be flaky case if #1471 doesn't merge.

Ⅳ. Describe how to verify it

➜  pouch run -d busybox sh -c 'for i in $(seq 1 3); do sleep 1; echo hello-$i; done;'
7ba6e5b25b961841145fd8aacaa4d9269bb38ef55519cf74265480326fd779d9
➜  pouch logs --timestamps 7ba6e5b25b961841145fd8aacaa4d9269bb38ef55519cf74265480326fd779d9
2018-06-05T14:05:31.082112571Z hello-1
2018-06-05T14:05:32.084694723Z hello-2
2018-06-05T14:05:33.09511301Z hello-3
➜  pouch logs --since 2018-06-05T14:05:32Z --until 2018-06-05T14:05:32.084694724Z 7ba6e5b25b961841145fd8aacaa4d9269bb38ef55519cf74265480326fd779d9
hello-2
➜  pouch logs --until 2018-06-05T14:05:32.084694724Z 7ba6e5b25b961841145fd8aacaa4d9269bb38ef55519cf74265480326fd779d9
hello-1
hello-2

Ⅴ. Special notes for reviews

@allencloud
Copy link
Collaborator

Unfortunately, CI fails with golint, @fuweid :

/go/bin/golint
pkg/utils/utils.go:141:4: should replace numColons -= 1 with numColons--
Makefile:57: recipe for target 'lint' failed

Interesting.

@fuweid
Copy link
Contributor Author

fuweid commented Jun 5, 2018

@allencloud I want to add the make check in the git pre-hook. just reminder haha

@fuweid
Copy link
Contributor Author

fuweid commented Jun 7, 2018

@HusterWan Please help me to check the failed case 😄 thanks

@codecov-io
Copy link

codecov-io commented Jun 7, 2018

Codecov Report

Merging #1472 into master will increase coverage by 24.37%.
The diff coverage is 76.05%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1472       +/-   ##
===========================================
+ Coverage    16.5%   40.87%   +24.37%     
===========================================
  Files         210      253       +43     
  Lines       13889    16539     +2650     
===========================================
+ Hits         2292     6760     +4468     
+ Misses      11441     8922     -2519     
- Partials      156      857      +701
Impacted Files Coverage Δ
cli/logs.go 0% <0%> (ø) ⬆️
daemon/logger/jsonfile/utils.go 71.54% <100%> (+1.54%) ⬆️
pkg/utils/utils.go 88.67% <100%> (+15.62%) ⬆️
client/container_logs.go 76.92% <55.55%> (+76.92%) ⬆️
apis/metrics/metrics.go 100% <0%> (ø)
storage/quota/set_diskquota.go 9.09% <0%> (ø)
cri/stream/httpstream/spdy/upgrade.go 0% <0%> (ø)
pkg/debug/debug.go 22.22% <0%> (ø)
storage/quota/quota.go 12.61% <0%> (ø)
internal/generator.go 100% <0%> (ø)
... and 117 more

@allencloud
Copy link
Collaborator

Quite interesting, the test coverage decreases. 😄

left = 0
}

b = make([]byte, blockSize)
b = make([]byte, readN)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the content don't end with \n like 1\n2\n3\n4, seekOffsetByTailLines(, 2) will return the index of 2. The tail content would be 2\n3\n4. Compared with command tail, echo "1\n2\n3\n4" > a.txt; tail -n2 a.txt is 3\n4.
Does this function works as expected?😁 @fuweid

@fuweid
Copy link
Contributor Author

fuweid commented Jun 7, 2018 via email

@zhuangqh
Copy link
Contributor

zhuangqh commented Jun 8, 2018

haha. I got it. Thanks for your reply at midnight~ @fuweid

@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jun 12, 2018
@allencloud allencloud merged commit 578caa9 into AliyunContainerService:master Jun 12, 2018
@fuweid fuweid deleted the feature_finish_logs_client_part branch August 3, 2018 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
areas/log kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pouch logs command doesn't work
5 participants