Fix logs operator command, prevent aborting streaming after 30s#14
Fix logs operator command, prevent aborting streaming after 30s#14mclasmeier wants to merge 2 commits into
logs operator command, prevent aborting streaming after 30s#14Conversation
| // New creates a new Logger instance | ||
| func New() *Logger { | ||
| return &Logger{ | ||
| type LoggerImpl struct { |
There was a problem hiding this comment.
I don't think this type needs to be exported, even if returned by another exported function.
| // getTimestamp returns the elapsed time since logger creation in MM:SS format | ||
| func (l *Logger) getTimestamp() string { | ||
| // NewWithTimestamps creates a new Logger instance with timestamps. | ||
| func NewWithTimestamps() *LoggerImpl { |
There was a problem hiding this comment.
Why are we reinventing the wheel here instead of using one of the many logging libraries available?
There was a problem hiding this comment.
This logger provides a high-level logging interface specifically built for the different messaging and formatting styles (colors, timestamps, etc.) for the roxie UX.
Do you think this would be a use-case for relying on a standard logger?
There was a problem hiding this comment.
I don't know, are you saying there are no loggers out there which support timestamps and colors? 🤔
|
|
||
| // Create kubectl command with context | ||
| kubectlCmd := exec.CommandContext(ctx, "kubectl", kubectlArgs...) | ||
| // Create kubectl command without context - allows indefinite streaming. |
There was a problem hiding this comment.
Shouldn't we use a context with cancel instead and call defer cancel() here?
There was a problem hiding this comment.
You mean something like context.Background()? What would be the advantage over not providing a context?
There was a problem hiding this comment.
I meant background wrapped in https://pkg.go.dev/context#WithCancel
Or is it not worth bothering to cancel() because the only way to exit is to die anway?
2 participants
As a prerequisite, the first commit changes the codebase to leverage a Logger interface.
Before:
After:
Doesn't terminate after 30s.
If the cluster cannot be reached, we fail early:
