Use OS-specific libs when computing client User-Agent in kubectl, etc. #44423
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-github-robot
added
size/M
|
This change is |
pwittrock
assigned mml, pwittrock and lavalamp and unassigned krousey and smarterclayton
|
/lgtm |
k8s-ci-robot
added
the
lgtm
k8s-github-robot
added
do-not-merge
k8s-github-robot
removed
the
lgtm
|
/approve |
k8s-github-robot
added
the
approved
| // path like C:\Users\Bob\Downloads\beans since its impl is os dependent. | ||
| func TestAdjustCommand(t *testing.T) { | ||
| assert := assert.New(t) | ||
| assert.Equal("beans", adjustCommand("/home/bob/Downloads/junk/beans")) |
There was a problem hiding this comment.
This test would be broken if run on windows. You need to use filepath.Join to construct a file path.
There was a problem hiding this comment.
Oh, I see you even left a comment to that effect. Better to just fix the test, I think. :)
There was a problem hiding this comment.
What about filepath.join("home", "bob", "Downloads", "junk", "beans")
|
Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion. staging/src/k8s.io/client-go/rest/config_test.go, line 122 at r1 (raw file): Previously, pwittrock (Phillip Wittrock) wrote…
Done, thanks Also - lemme know if the import of "github.com/stretchr/testify" is objectionable, Comments from Reviewable |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
Ginkgo / Gomega is another one: Not sure if one is preferred generally. I like Ginkgo. |
|
@k8s-bot verify test this |
|
@k8s-bot cvm gce e2e test this |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lavalamp, monopole, pwittrock, smarterclayton
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
pwittrock
removed
the
do-not-merge
|
Automatic merge from submit-queue |
|
@monopole I can't get this to cherry-pick cleanly onto 1.6. Can you take a look? |
…nch. The original full PR kubernetes#44423 came after a large PR kubernetes#40777 that split /vendor/BUILD into hundreds of BUILD files. Thus PR 44423's version of rest/BUILD does not exist in the 1.6 release branch, and had to be tweaked here.
Automatic merge from submit-queue Use OS-specific libs when computing client User-Agent in kubectl, etc. This PR fixes issue 44419 in the release branch. This PR is a patchable version of mainline PR #44423 for 1.6 release branch The original full PR #44423 came after a large PR #40777 that split /vendor/BUILD into hundreds of BUILD files. Thus PR 44423's version of rest/BUILD does not exist in the 1.6 release branch, and had to be tweaked here. ```release-note Fix for [Windows kubectl sending full path to binary in User Agent](#44419). ```
The original full PR kubernetes/kubernetes#44423 came after a large PR kubernetes/kubernetes#40777 that split /vendor/BUILD into hundreds of BUILD files. Thus PR 44423's version of rest/BUILD does not exist in the 1.6 release branch, and had to be tweaked here. Kubernetes-commit: a26452b2da054b1326eb809e08b6ae8a6096e758
The original full PR kubernetes/kubernetes#44423 came after a large PR kubernetes/kubernetes#40777 that split /vendor/BUILD into hundreds of BUILD files. Thus PR 44423's version of rest/BUILD does not exist in the 1.6 release branch, and had to be tweaked here. Kubernetes-commit: a26452b2da054b1326eb809e08b6ae8a6096e758
What this PR does / why we need it:
The User-Agent reported by clients (e.g. kubectl) in request
headers should include the name of the client executable
but not the full path to that executable.
This PR changes how this name is determined by using the
operating-system specific package "path/filepath" (meant for
working with file system paths) instead of the "path" package
(meant for URL paths).
This fixes a problem on the Windows OS in the case where, if the
user has not set their PATH to point to the location of their
client executable, the User-Agent includes the full path - which
is unnecessary.
Fixes: #44419