fix: trace-ebpf flag output #632
Conversation
krol3
force-pushed
the
issue-560-flag
branch
3 times, most recently
from
2e47521
to
4fc14ce
Compare
tracee-ebpf/main.go
Outdated
| @@ -175,16 +175,24 @@ Use this flag multiple times to choose multiple capture options | |||
| outputParts := strings.SplitN(o, ":", 2) | |||
| numParts := len(outputParts) | |||
| if numParts == 1 { | |||
| if !validateOutputFormatValue(outputParts[0]) { | |||
There was a problem hiding this comment.
Instead of validating when we get inside prepareOutput it will be easier to validate outside, prior to reaching here. So I suggest that before we call prepareOutput we can pass c.StringSlice(output) to another function that can validate all types of possible inputs that can be passed to --output flag.
This will keep the abstraction of prepareOutput to assume the inputs it receives are valid and also make it easier to read and test as it will have less branches. Furthermore testing will be simplified as well as we can just test both functions individually.
There was a problem hiding this comment.
regarding validation, I don't think that preparer functions should have validation logic. the XXXConfig struct already has the validation logic
tracee/tracee-ebpf/tracee/tracee.go
Line 114 in f8df7da
also I think other configs should follow this pattern. wdyt @simar7 ?
There was a problem hiding this comment.
Thanks @simar7 . I used the Validate() method and removed the other method that I created.
tracee-ebpf/main.go
Outdated
| } else if outputParts[0] == "out-file" { | ||
| err := res.Validate() | ||
| if err != nil { | ||
| return res, fmt.Errorf("%s. Valid format values: 'table', 'table-verbose', 'json', 'gob' or 'gotemplate='. Use '--output help' for more info.", err) |
There was a problem hiding this comment.
nit: I think this error string should be returned in Validate() itself.
There was a problem hiding this comment.
One nit from me, otherwise this looks good! Will merge after that small change.
Thanks @krol3!
|
@krol3 looks like there's a few compile time issues. If you fix those and tests pass, we can merge this in as the rest of the code is good. |
Related issue #560