Sanity check if the image name has a colon #922
Conversation
| if len(split_result) > 1 { | ||
| version = split_result[1] | ||
| } else { | ||
| return false |
There was a problem hiding this comment.
Should we add a log line here or something? I'm just thinking that if this condition is hit (no semi-colon in the image name), how would the end user know?
There was a problem hiding this comment.
We have tried to add the log line before, but it seems that a parameter that defines the logger needs to be passed to this function. However, this parameter is currently not available in this function.
There was a problem hiding this comment.
I think it would be sufficient to add an else branch here that lets the user know that logging is not enabled.
There was a problem hiding this comment.
Okay, I have added it in the latest commit.
|
Thanks for your response. We have added a unit test in our latest commit. |
| name: "verify image name without colon", | ||
| cluster: clusterBuilder.WithImage(customImageName).Cluster(), | ||
| imageName: customImageName, | ||
| }, |
There was a problem hiding this comment.
I think we're missing the case where logging is enabled here.
| if len(split_result) > 1 { | ||
| version = split_result[1] | ||
| } else { | ||
| return false |
There was a problem hiding this comment.
I think it would be sufficient to add an else branch here that lets the user know that logging is not enabled.
|
@pseudomuto Could you review this so that it can be merged? I think this is a pretty straight-forward fix |
fix #918
We fixed the issue by adding sanity check to examine whether a colon is contained in the image name in
IsLoggingAPIEnabledfunction. If there is no colon in image name, we will not retrieve index 1 and will directly return false.Checklist