replace TrimRight with TrimSuffix #8041
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/assign @chrisz100 |
|
/test pull-kops-e2e-kubernetes-aws |
|
/cc @justinsb |
|
/cc @rifelpet |
| @@ -160,7 +160,7 @@ func (a *AssetBuilder) RemapImage(image string) (string, error) { | |||
| } | |||
|
|
|||
| if a.AssetsLocation != nil && a.AssetsLocation.ContainerProxy != nil { | |||
| containerProxy := strings.TrimRight(*a.AssetsLocation.ContainerProxy, "/") | |||
There was a problem hiding this comment.
I'm not sure this is a safe change to make.
If I understand the differences correctly, this would mean that if a ContainerProxy contained a path after the host like proxy.example.com/path/, the old behavior would trim it down to proxy.example.com but the new behavior will trim it down to proxy.example.com/path. I don't know if having a path in ContainerProxy is valid, but if it is this would break existing behavior. Do we know if its valid or not?
There was a problem hiding this comment.
/cc @justinsb
some works need to to check this change.
There was a problem hiding this comment.
I think, with a single character in the second arg, TrimRight will remove multiple occurrences, whereas TrimSuffix will only remove one. So I think TrimRight is actually probably more correct here (just in case ContainerProxy ends with e.g. //), but I don't think it's likely to be a significant issue either way.
Is there a particular reason to make the change, @tanjunchen ? I agree we haven't been consistent, but I'm not sure which is right. Maybe we need a helper function that we use for all these url-style joins.
There was a problem hiding this comment.
@rifelpet @justinsb Thanks your review.
Yes, i think it's likely to be a significant issue either way too.
I just found that only strings.TrimRight is used here in the kops project, and strings.TrimSuffix is used everywhere else.
The Only one strings.TrimRight in kops here.
Golang officially recommends this strings.TrimSuffix.
|
@tanjunchen: GitHub didn't allow me to request PR reviews from the following users: for, decide. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
tanjunchen
changed the title
|
/test pull-kops-verify-staticcheck |
|
/cc @justinsb maybe i need to make more convey about it |
|
@tanjunchen: GitHub didn't allow me to request PR reviews from the following users: need, convey, make, more, it, maybe, i, to. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Let's merge it, but not cherry-pick it back unless there's a reason to... I'm pretty sure we meant TrimSuffix anyway! /approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb, tanjunchen The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/test pull-kops-e2e-kubernetes-aws |
In string based trim scenarios,we shall always use string based TrimPrefix/TrimSuffix instead of char based TrimLeft/TrimRight.
See the diff here: https://play.golang.org/p/S7cBaoIxPQy