Updating integration tests to test both API versions - v1beta1 and 3 #5587

nikhiljindal · 2015-03-18T08:24:24Z

googlebot · 2015-03-18T08:24:26Z

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

bgrant0607 · 2015-03-18T14:16:09Z

--- FAIL: TestExampleObjectSchemas (0.23s)
    examples_test.go:159: ../cmd/integration/v1beta1-controller.json does not have a test case defined
    examples_test.go:159: ../cmd/integration/v1beta3-controller.json does not have a test case defined
    examples_test.go:175: Expected 1 examples, Got 0
FAIL
coverage: 0.0% of statements
FAIL    github.com/GoogleCloudPlatform/kubernetes/examples  0.380s

nikhiljindal · 2015-03-18T18:44:56Z

Thats a cool test that I didnt know existed!
Fixed.

bgrant0607 · 2015-03-18T20:40:50Z

cmd/integration/integration.go

 	if err != nil {
 		glog.Fatalf("Failed listing service with supplied self link '%v': %v", svc.SelfLink, err)
 	}

-	var svcList api.ServiceList
-	err = c.Get().NamespaceIfScoped(namespace, len(namespace) > 0).Resource("services").Do().Into(&svcList)
+	svcList, err := services.List(labels.Everything())


Why did you remove the namespacing?

It looks like its still there, but services is now ServiceNamespacer that has already been scoped to the namespace in question.

nikhiljindal · 2015-03-18T21:02:28Z

Replied to all comments. PTAL.

bgrant0607 · 2015-03-18T21:10:45Z

pkg/master/master.go

@@ -65,7 +65,6 @@ import (

 // Config is a structure used to configure a Master.
 type Config struct {
-	Client            *client.Client


Again, why this change?

Nevermind. I see your answer above now.

bgrant0607 · 2015-03-18T22:26:49Z

LGTM. Waiting for tests.

nikhiljindal · 2015-03-18T22:42:52Z

Shippable passed :)

Updating integration tests to test both API versions - v1beta1 and 3

smarterclayton · 2015-03-20T21:35:21Z

pkg/client/services.go

@@ -77,7 +77,13 @@ func (c *services) Get(name string) (result *api.Service, err error) {
 // Create creates a new service.
 func (c *services) Create(svc *api.Service) (result *api.Service, err error) {
 	result = &api.Service{}
-	err = c.r.Post().Namespace(c.ns).Resource("services").Body(svc).Do().Into(result)
+	// v1beta3 does not allow POST without a namespace.
+	needNamespace := !api.PreV1Beta3(c.r.APIVersion())


Why is this needed? I think this is a bug in higher level code, and doesn't look right here.

If not here, then we will need such conditions everywhere at higher level code.

If (apiVersion = v1beta3 && ns == "") { ns = api.NamespaceDefault } servicesInterface = client.services(ns)

Is that better?

I'm saying that anyone who has an empty string either got it from client input (in which case the server will catch it) or it will be caught lower in the code base. No one should be passing an empty namespace, ever. I don't think we need to test in the client (I have a pull incoming which normalizes all our clients now that the server is normalized).

----- Original Message -----

@@ -77,7 +77,13 @@ func (c *services) Get(name string) (result
*api.Service, err error) {
// Create creates a new service.
func (c *services) Create(svc *api.Service) (result *api.Service, err
error) {
result = &api.Service{}

err =
c.r.Post().Namespace(c.ns).Resource("services").Body(svc).Do().Into(result)

// v1beta3 does not allow POST without a namespace.

needNamespace := !api.PreV1Beta3(c.r.APIVersion())

If not here, then we will need such conditions everywhere at higher level
code.

If (apiVersion = v1beta3 && ns == "") { ns = api.NamespaceDefault } servicesInterface = client.services(ns)

Is that better?

Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/5587/files#r26879365

ok. What do you mean by "normalizing the server and the client"?

The RESTStorage objects and the api_installer now properly check a lot of the conditions. If there is client code that is allowing Namespace to be wrong when it passes to the Services code, we should either fix the client code or reject it on the server (the standard api/rest.go code should do that now).

Opened #5738 to clean some of this up.

----- Original Message -----

@@ -77,7 +77,13 @@ func (c *services) Get(name string) (result
*api.Service, err error) {
// Create creates a new service.
func (c *services) Create(svc *api.Service) (result *api.Service, err
error) {
result = &api.Service{}

err =
c.r.Post().Namespace(c.ns).Resource("services").Body(svc).Do().Into(result)

// v1beta3 does not allow POST without a namespace.

needNamespace := !api.PreV1Beta3(c.r.APIVersion())

ok. What do you mean by "normalizing the server and the client"?

Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/5587/files#r26881133

ok. Thanks for the explanation!
I looked at the PR and it looks good.
Thanks for working on it.

googlebot added the cla: no label Mar 18, 2015

nikhiljindal assigned bgrant0607 Mar 18, 2015

nikhiljindal force-pushed the enable3 branch from bdac075 to 084a705 Compare March 18, 2015 08:29

nikhiljindal force-pushed the enable3 branch from 084a705 to cfeeadf Compare March 18, 2015 18:43

bgrant0607 reviewed Mar 18, 2015
View reviewed changes

Updating integration tests to test both API versions - v1beta1 and 3

7e36bba

nikhiljindal force-pushed the enable3 branch from cfeeadf to 7e36bba Compare March 18, 2015 22:24

bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 18, 2015

bgrant0607 added a commit that referenced this pull request Mar 18, 2015

Merge pull request #5587 from nikhiljindal/enable3

11f9733

Updating integration tests to test both API versions - v1beta1 and 3

bgrant0607 merged commit 11f9733 into kubernetes:master Mar 18, 2015

nikhiljindal mentioned this pull request Mar 19, 2015

Test all api versions #1473

Closed

smarterclayton reviewed Mar 20, 2015
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating integration tests to test both API versions - v1beta1 and 3 #5587

Updating integration tests to test both API versions - v1beta1 and 3 #5587

nikhiljindal commented Mar 18, 2015

googlebot commented Mar 18, 2015

bgrant0607 commented Mar 18, 2015

nikhiljindal commented Mar 18, 2015

bgrant0607 Mar 18, 2015

bgrant0607 Mar 18, 2015

derekwaynecarr Mar 18, 2015

nikhiljindal commented Mar 18, 2015

bgrant0607 Mar 18, 2015

bgrant0607 Mar 18, 2015

bgrant0607 commented Mar 18, 2015

nikhiljindal commented Mar 18, 2015

smarterclayton Mar 20, 2015

nikhiljindal Mar 20, 2015

smarterclayton Mar 20, 2015

nikhiljindal Mar 20, 2015

smarterclayton Mar 20, 2015

nikhiljindal Mar 23, 2015

Updating integration tests to test both API versions - v1beta1 and 3 #5587

Updating integration tests to test both API versions - v1beta1 and 3 #5587

Conversation

nikhiljindal commented Mar 18, 2015

googlebot commented Mar 18, 2015

bgrant0607 commented Mar 18, 2015

nikhiljindal commented Mar 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhiljindal commented Mar 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Mar 18, 2015

nikhiljindal commented Mar 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment