-
Notifications
You must be signed in to change notification settings - Fork 390
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: code refactor to add assertion #4014
chore: code refactor to add assertion #4014
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit comment, otherwise LGTM
case 4: | ||
return pb.Severity_CRITICAL | ||
severityValue, ok := metadata["Severity"].(int) | ||
if ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
if ok { | |
if !ok { | |
return -1 | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review. If we do so we still need do add another return in the final in case the switch is not satisfied (e.g: severityValue is 5).
Another thing, just realized it, but I think this func should follow the same logic as threat.pb.go:
tracee/api/v1beta1/threat.pb.go
Lines 136 to 141 in cd6759e
func (x *Threat) GetSeverity() Severity { | |
if x != nil { | |
return x.Severity | |
} | |
return Severity_INFO | |
} |
If any error is found, we should return Severity_INFO (0) and not (-1). If so, it should be:
func getSeverity(metadata map[string]interface{}) pb.Severity {
severityValue, ok := metadata["Severity"].(int)
if ok {
switch severityValue {
case 0:
return pb.Severity_INFO
case 1:
return pb.Severity_LOW
case 2:
return pb.Severity_MEDIUM
case 3:
return pb.Severity_HIGH
case 4:
return pb.Severity_CRITICAL
}
}
return pb.Severity_INFO
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. The nit here was just about removing the extra indentation of the switch case
3946f37
to
958ff58
Compare
} | ||
} | ||
|
||
return pb.Severity_INFO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yanivagman rework on this function and now is return pb.Severity_INFO
as default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense
Refactored code to fix the unchecked type cast issue by incorporating error handling after the type assertion.
958ff58
to
77fc124
Compare
1. Explain what the PR does
fix: #3475
3645eaa chore: code refactor to add assertion
e1ec4b2 chore: enable unchecked-type-assertion
3645eaa chore: code refactor to add assertion
2. Explain how to test it
3. Other comments