accept Ctrl-C in cri log viewer #1972
Conversation
| default: | ||
| if stop || (limitedMode && limitedNum == 0) { | ||
| logrus.Debugf("Finished parsing log file, path; %s", logPath) | ||
| return nil |
There was a problem hiding this comment.
nit: Finished -> finished to be consistent; and typo ; -> :.
There was a problem hiding this comment.
According to common custom，maybe it's a good idea to let log print line ( such as logrus.Debugf(“Example”) logrus.Warnf(“Example”)) start with uppercase, and error string returned (such asfmt.Errorf("example")) start with lowercase
And It seems that there is no uniformity about uppercase and lowercase
uppercase : https://github.com/containerd/nerdctl/blob/main/pkg/composer/create.go#L169
lowercase: https://github.com/containerd/nerdctl/blob/main/cmd/nerdctl/container_cp_linux.go#L259
There was a problem hiding this comment.
For reference only: https://www.postgres-xl.org/documentation/error-style-guide.html
I think it is also necessary to unify it in our project.
There was a problem hiding this comment.
+1 Agree, maybe we should open a issue to track the uniformity in our registry，cc @djdongjin
There was a problem hiding this comment.
This SGTM, but personally I feel this uniformity is more a best-effort but difficult to achieve (or maintain after achieved), part because it's quite detailed and there is a lot of error/log messages, and also because there are new contributors (and reviewers) who're not aware of these.
Speaking of this PR, I'd suggest to lowercase all error/log messages :) since it has both uppercase (line 122, 146, 152, ...) and lowercase (118, 128, 135, ...). Thanks :)
There was a problem hiding this comment.
This SGTM, but personally I feel this uniformity is more a best-effort but difficult to achieve (or maintain after achieved), part because it's quite detailed and there is a lot of error/log messages, and also because there are new contributors (and reviewers) who're not aware of these.
You're right. However, we can add some related guides in the future.
There was a problem hiding this comment.
ok, let me lowercase error/log message in this PR
zhaojizhuang
force-pushed
the
cri-log
branch
2 times, most recently
from
1a415c6
to
b0d6871
Compare
|
updated @djdongjin |
|
/test |
|
CI failed，retest it? @djdongjin |
|
@zhaojizhuang sorry I'm a reviewer and don't have write permission to restart CI 😅. The PR will be merged by a committer who will restart the CI. You can try close & reopen the PR to force CI rerun. |
This method scares me. |
pkg/logging/cri_logger.go
Outdated
| } | ||
|
|
||
| // ReadLogs read the container log and redirect into stdout and stderr. | ||
| // Note that containerID is only needed when following the log, or else | ||
| // just pass in empty string "". | ||
| func ReadLogs(ctx context.Context, opts *LogViewOptions, stdout, stderr io.Writer) error { | ||
| func ReadLogs(ctx context.Context, opts *LogViewOptions, stdout, stderr io.Writer, stopChannel chan os.Signal) error { |
pkg/logging/cri_logger_test.go
Outdated
| @@ -85,7 +87,7 @@ func TestReadLogs(t *testing.T) { | |||
| t.Run(tc.name, func(t *testing.T) { | |||
| stdoutBuf := bytes.NewBuffer(nil) | |||
| stderrBuf := bytes.NewBuffer(nil) | |||
| err = ReadLogs(context.TODO(), &tc.logViewOptions, stdoutBuf, stderrBuf) | |||
| err = ReadLogs(context.TODO(), &tc.logViewOptions, stdoutBuf, stderrBuf, stopChan) | |||
Fixes: containerd#1956 Signed-off-by: zhaojizhuang <571130360@qq.com>
|
updated @fahedouch |
Fixes: #1956
Signed-off-by: zhaojizhuang 571130360@qq.com