feat: Reading Logs in daignose cli command #1521
Conversation
cli/cmd/diagnose.go
Outdated
| } | ||
|
|
||
| func createAllDirs() error { | ||
| if err := os.RemoveAll(mainDir); err != nil { |
There was a problem hiding this comment.
Also related to Eden's comment, you can have a look at the TempDir function.
I think having the files created in a temp location and only saving the final zip in the CWD is the accepted behavior
There was a problem hiding this comment.
TempDir is deprecated from go 1.16 - the new way is with os.MkdirTemp
There was a problem hiding this comment.
ohhh my bad, ioutil.TempDir() is deprecated
cli/cmd/diagnose.go
Outdated
|
|
||
| // Define the log file path for saving compressed logs | ||
| logFilePath := filepath.Join(logDir, pod.Name+"_"+container.Name+"_"+pod.Spec.NodeName+".log.gz") | ||
| logFile, err := os.OpenFile(logFilePath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0666) |
There was a problem hiding this comment.
should add here
defer logFile.Close()
Instead of the current code which only closes the file at the happy-path where no errors occur.
This will guarantee the file is close when this function returns.
There was a problem hiding this comment.
Why Not Use defer Inside the Loop?
In a loop, using defer to close a file might seem like a convenient option, but deferred calls are not executed until the surrounding function returns. If you use defer inside the loop, it will accumulate all the defer calls, and the files will remain open until the function (or main program) exits. This can lead to excessive memory or resource usage, especially with a large number of iterations.
There was a problem hiding this comment.
Ok I read a bit and this is a real cool way to solve this.. The anonymous function will be called after opening the file and because it uses defer and created inside the scope of the for-loop -> it will trigger in the end of the loop (succesfully / with error)
func() {
defer func() {
if err := file.Close(); err != nil {
fmt.Printf("Error closing file %s: %v\n", fileName, err)
}
}()
There was a problem hiding this comment.
You can also extract the code in the loop body to a function and use a defer within that function, that way there is no risc for a long queue of defer functions.
No description provided.