Skip to content
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

Use OS-specific libs when computing client User-Agent in kubectl, etc. #44423

Merged
merged 1 commit into from
Apr 15, 2017

Conversation

monopole
Copy link
Contributor

@monopole monopole commented Apr 13, 2017

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

Use OS-specific libs when computing client User-Agent in kubectl, etc.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 13, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Apr 13, 2017
@monopole
Copy link
Contributor Author

@pwittrock @mml

@k8s-reviewable
Copy link

This change is Reviewable

@pwittrock
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 13, 2017
@k8s-github-robot k8s-github-robot added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Apr 13, 2017
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 13, 2017
@smarterclayton
Copy link
Contributor

/approve

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 13, 2017
// 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"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test would be broken if run on windows. You need to use filepath.Join to construct a file path.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see you even left a comment to that effect. Better to just fix the test, I think. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about filepath.join("home", "bob", "Downloads", "junk", "beans")

@monopole
Copy link
Contributor Author

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…

What about filepath.join("home", "bob", "Downloads", "junk", "beans")

Done, thanks

Also - lemme know if the import of "github.com/stretchr/testify" is objectionable,
or if there's a preferred alternative. Only seven other tests in all of k8s import it.


Comments from Reviewable

@lavalamp
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 13, 2017
@pwittrock
Copy link
Member

Ginkgo / Gomega is another one:
https://onsi.github.io/ginkgo/

Not sure if one is preferred generally. I like Ginkgo.

@monopole
Copy link
Contributor Author

@k8s-bot verify test this

@monopole
Copy link
Contributor Author

@k8s-bot cvm gce e2e test this

@k8s-github-robot
Copy link

[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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@pwittrock pwittrock removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Apr 15, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit f05ce1e into kubernetes:master Apr 15, 2017
@mml
Copy link
Contributor

mml commented Apr 17, 2017

@monopole I can't get this to cherry-pick cleanly onto 1.6. Can you take a look?

monopole added a commit to monopole/kubernetes that referenced this pull request Apr 18, 2017
…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.
k8s-github-robot pushed a commit that referenced this pull request Apr 18, 2017
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).
```
k8s-publish-robot pushed a commit to kubernetes/client-go that referenced this pull request Apr 18, 2017
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
caesarxuchao pushed a commit to caesarxuchao/client-go that referenced this pull request Apr 19, 2017
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
@monopole monopole deleted the fixUserAgent branch October 27, 2019 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows kubectl sends full path as User-Agent
9 participants