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

Now kubectl through https uses gce #5

Merged

Conversation

ashcrow
Copy link

@ashcrow ashcrow commented Sep 24, 2015

GCE spdy proxy e2e tests

This is a modification of #4 . The changes are the same but much of the internals have been updated to utilize testContext instead of environment variables, using SSL and tokens for netexec's kubectl calls, and unversioned.Client for proxying through the api server to pods. No new arguments have been added to the e2e.test binary.

Notes

Images

The pods.yaml for both netexec and goproxy have been temporarily changed to point to dockerhub where I have the test images posted.

Running

This is called when running the e2e tests which insinuates that kubectl will be available on the system running the tests.

  • ---host must be $SCHEME$HOST:$PORT/api: https://127.0.0.1:443/api
  • --kubectl-path must be a real path to the kubectl binary
  • ---kubeconfig must be a valid kubeconfig with an e2e user that has a token
[e2e/]$ e2e.test --host="https://API_SERVER:443/api" --provider="gce" --ginkgo.v=true --ginkgo.focus="support exec through an HTTP proxy" --kubeconfig="$HOME/.kube/config" --kubectl-path=`which kubectl`

PREFIX = gcr.io/google_containers
TAG = 1.2
#PREFIX = gcr.io/google_containers
PREFIX = stevemilnerrh
Copy link
Owner

Choose a reason for hiding this comment

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

TODO replace with gcr.io once the new version has been pushed

@@ -161,6 +165,7 @@ var _ = Describe("Kubectl client", func() {
It("should support exec", func() {
By("executing a command in the container")
execOutput := runKubectl("exec", fmt.Sprintf("--namespace=%v", ns), simplePodName, "echo", "running", "in", "container")

Copy link

Choose a reason for hiding this comment

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

I have found it to be more idiomatic to strike lines of this class in golang. You don't have to do it though.

Copy link
Owner

Choose a reason for hiding this comment

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

Whitespace is a friend, not an enemy

@pmorie
Copy link

pmorie commented Sep 25, 2015

LGTM module those nits, this is ready to get some feedback from upstream on.

@ashcrow
Copy link
Author

ashcrow commented Sep 25, 2015

@pmorie thank you for the review! I've updated the code to address your feedback. I've also squashed this down into 3 commits per @ncdc's request.

@ncdc
Copy link
Owner

ncdc commented Sep 25, 2015

We might want to have the new goproxy image be in a separate commit. WDYT @pmorie?

@pmorie
Copy link

pmorie commented Sep 25, 2015

@ncdc You're going to wind up squashing anyway once you have more commits to address feedback, don't worry about it for now.

@ncdc
Copy link
Owner

ncdc commented Sep 25, 2015

@pmorie well, it's not really about squashing. Right now there is 1 commit that adds e2es for exec+proxy and adds the goproxy image. I'm suggesting we split this 1 commit into 2.

@pmorie
Copy link

pmorie commented Sep 25, 2015

@ncdc I understand, and I'm saying that odds are you'll have feedback that cuts across all of it anyway that you'll want to capture in those same commits after you squash in new commits that contain the cross-cutting feedback.

@ncdc ncdc merged this pull request into ncdc:add-spdy-proxy-support Sep 25, 2015
ncdc pushed a commit that referenced this pull request Nov 7, 2017
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Remove redundant call to StartLogging in service_controller. Fixes #5…

…4339



**What this PR does / why we need it**:
Removes redundant call to StartLogging introduced in 96b48d4#diff-1f7f903e25ab8bcbc514bb1e532e997e

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #
kubernetes#54339 

**Special notes for your reviewer**:

**Release note**:

```release-note
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants