[d8] Add d8 collect-debug-info #81
Open
+347
−51
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
d2285
force-pushed
the
d8-collect-debug-info
branch
from
4200c8f to
a29a9ac
Compare
d2285
force-pushed
the
d8-collect-debug-info
branch
2 times, most recently
from
ca010b1 to
581ed5d
Compare
d2285
force-pushed
the
d8-collect-debug-info
branch
3 times, most recently
from
bfca89d to
07b5b93
Compare
mvasl
reviewed
Feb 13, 2025
internal/platform/cmd/collect-debug-info/createtarball/createtarball.go
Outdated
Show resolved
Hide resolved
mvasl
requested changes
Feb 13, 2025
internal/platform/cmd/collect-debug-info/createtarball/createtarball.go
Outdated
Show resolved
Hide resolved
internal/platform/cmd/collect-debug-info/createtarball/createtarball.go
Outdated
Show resolved
Hide resolved
internal/platform/cmd/collect-debug-info/createtarball/createtarball.go
Outdated
Show resolved
Hide resolved
d2285
force-pushed
the
d8-collect-debug-info
branch
from
7be2faa to
7e546d9
Compare
mvasl
reviewed
Feb 24, 2025
Comment on lines
+166
to
+168
| if term.IsTerminal(int(os.Stdout.Fd())) { | ||
| return fmt.Errorf("Please provide output tar.gz to dump debug logs, ex. \"> dump-logs.tar.gz\"") | ||
| } |
There was a problem hiding this comment.
This can be done early, there is no point in reaching to the kube-apiserver if we know that we are not going to output to file and throw error anyway
There was a problem hiding this comment.
PreRunE seems like a good place to put such checks
https://pkg.go.dev/github.com/spf13/cobra#Command
There was a problem hiding this comment.
fixed, moved it to PreRunE
There was a problem hiding this comment.
It seems that you forgot to remove it here
mvasl
reviewed
Feb 24, 2025
name212
approved these changes
Feb 26, 2025
mvasl
requested changes
Feb 26, 2025
Comment on lines
+42
to
+46
| var err error | ||
| if term.IsTerminal(int(os.Stdout.Fd())) { | ||
| return fmt.Errorf("Please provide output tar.gz to dump debug logs, ex. \"> dump-logs.tar.gz\"") | ||
| } | ||
| return err |
There was a problem hiding this comment.
var err error is not required, return nil explicitly. Doing it in this way may cause nasty bugs that are hard to debug.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pr add d8 platform collect-debug-info option to save output debug info to tar.gz archive.
e.g.