Sanity check if the image name has a colon#922
Sanity check if the image name has a colon#922hoyhbx wants to merge 4 commits intocockroachdb:masterfrom
Conversation
pseudomuto
left a comment
pseudomuto
left a comment
There was a problem hiding this comment.
Overall this LGTM, thanks for addressing this 👍
Would you mind adding a test for IsLoggingEnabled here to avoid any regressions in the future?
| 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. |
pseudomuto
left a comment
pseudomuto
left a comment
There was a problem hiding this comment.
Thanks for the updates. Just a few comments below. This is looking good.
| 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 |
2 participants
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