Ensure that ObjectMeta field selectors (e.g. name and namespace) can be used appropriately for all types #2214

jimmidyson · 2018-07-19T10:46:33Z

Noticed that the ObjectMeta fields metadata.name and metadata.namespace do not work as they haven't been registered in conversions.

I also took the liberty of tidying up the fields methods in registry to make them consitent in their approach.

carolynvs

Thanks for adding this! Once it is in we can use it to clean up some of our existing code around filtering by name vs external name. I'll create a follow-on issue once this is merged.

Just need a few new tests and this should be all set.

carolynvs · 2018-07-20T16:44:03Z

pkg/registry/servicecatalog/clusterservicebroker/storage.go

@@ -89,8 +89,7 @@ func Match(label labels.Selector, field fields.Selector) storage.SelectionPredic

 // toSelectableFields returns a field set that represents the object for matching purposes.
 func toSelectableFields(broker *servicecatalog.ClusterServiceBroker) fields.Set {
-	objectMetaFieldsSet := generic.ObjectMetaFieldsSet(&broker.ObjectMeta, true)
-	return generic.MergeFieldsSets(objectMetaFieldsSet, nil)
+	return generic.ObjectMetaFieldsSet(&broker.ObjectMeta, false)


👍 Thanks for fixing the "hasnamespacefield" arg

carolynvs · 2018-07-20T16:48:22Z

test/integration/clientset_test.go

@@ -542,6 +542,15 @@ func testClusterServiceClassClient(sType server.StorageType, client servicecatal
 		return fmt.Errorf("should have two ClusterServiceClasses, had %v ClusterServiceClasses", len(serviceClasses.Items))
 	}

+	serviceClasses, err = serviceClassClient.List(metav1.ListOptions{FieldSelector: "metadata.name=" + name})


Looks like we are missing existing tests for ServiceClasses and ServicePlan which were also modified in this PR.

Let's use those existing tests in this file and add tests for these resources as well so that we can test all of your changes.

I can't find any existing tests for these non-namespaced resources so just to clarify: would you like me to add tests for them in this PR? Or have I missed them somehow?

I'm suggesting that you copy the existing test testClusterServiceClassClient and tweak it to test the namespaced resource ServiceClasss so that you can also test your changes made to that as well. Same for plans (TestClusterServicePlanClient).

carolynvs · 2018-07-23T19:59:16Z

test/integration/clientset_test.go

 			client, _, shutdownServer := getFreshApiserverAndClient(t, sType.String(), func() runtime.Object {
-				return &servicecatalog.ClusterServicePlan{}
+				return &servicecatalog.ClusterServiceClass{}


I think this should be ServiceClass{}, no?

Yep good catch thanks! Updated.

carolynvs · 2018-07-23T20:00:05Z

test/integration/clientset_test.go

@@ -568,17 +770,23 @@ func testClusterServiceClassClient(sType server.StorageType, client servicecatal
 	return nil
 }

-// TestClusterServicePlanClient exercises the ClusterServicePlan client.
-func TestClusterServicePlanClient(t *testing.T) {
+// TestClusterServiceClassClient exercises the ClusterServiceClass client.


Is this test for namespaced classes or cluster scoped class?

Aligned all types/names/comments now (I hope!).

carolynvs · 2018-07-23T20:01:10Z

test/integration/clientset_test.go

 			})
 			defer shutdownServer()

-			if err := testClusterServicePlanClient(sType, client, name); err != nil {
+			if err := testClusterServiceClassClient(sType, client, name); err != nil {


Let's get the comment, the function name and this call to all align. 😀

Done - sorry!

carolynvs · 2018-07-23T20:49:23Z

test/integration/clientset_test.go

-	// check that the plan is the same from get and list
-	servicePlanListed := &servicePlans.Items[0]
-	if !reflect.DeepEqual(servicePlanAtServer, servicePlanListed) {
+	// check that the broker is the same from get and list


shouldn't this say class instead of broker?

carolynvs · 2018-07-23T20:50:19Z

test/integration/util.go

@@ -33,3 +49,19 @@ func falsePtr() *bool {
 	b := false
 	return &b
 }
+
+func enableNamespacedResources() (resetFeaturesFunc func(), err error) {


MHBauer · 2018-07-24T04:48:03Z

/test pull-service-catalog-xbuild

jimmidyson · 2018-08-01T07:33:59Z

Any chance of a review of this?

jboyd01 · 2018-08-01T14:40:28Z

@jimmidyson I see @carolynvs left some review comments & requested some changes.

jimmidyson · 2018-08-02T04:07:49Z

Which I'm pretty sure I've addressed...

jboyd01 · 2018-08-03T20:55:15Z

fixes #2260

MHBauer · 2018-08-03T21:13:06Z

test/integration/clientset_test.go

+	//			t.Errorf("%q test failed", sType)
+	//		}
+	//	}
+	sType := server.StorageTypeEtcd


if this is copy and paste of another test, it should be fixed up as well and remove the CRD comments.

Fixed up and removed comments.

…be used appropriately for all types

jboyd01 · 2018-08-06T14:00:57Z

@jimmidyson This looks solid to me. A couple of things still outstanding here: @carolynvs called out issues in #2214 about inconsistencies between the Name Spaced ServiceClass vs ClusterServiceClass and a nit on an old comment. and @MHBauer asking to clean up some dead CRD comments in #2214 Thanks for the work, I look forward to getting this merged.

jimmidyson · 2018-08-06T14:37:52Z

Sorry I missed those things... I'm on it later today, will be ready for merge then.

jimmidyson · 2018-08-06T18:36:12Z

@jboyd01 @carolynvs Ready for another review round - thanks for your comments!

jboyd01 · 2018-08-06T19:11:42Z

Thanks @jimmidyson!
/lgtm
/approve

k8s-ci-robot · 2018-08-06T19:11:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jboyd01

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:

~~OWNERS~~ [jboyd01]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from jpeeler and nilebox July 19, 2018 10:46

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 19, 2018

carolynvs suggested changes Jul 20, 2018

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 23, 2018

carolynvs suggested changes Jul 23, 2018

View reviewed changes

jimmidyson mentioned this pull request Aug 3, 2018

add metadata.name to the FieldLabel Conversion functions #2261

Closed

MHBauer reviewed Aug 3, 2018

View reviewed changes

Ensure that ObjectMeta field selectors (e.g. name and namespace) can …

ad332ca

…be used appropriately for all types

Add client tests for namespaced resources

87b3f4b

k8s-ci-robot assigned jboyd01 Aug 6, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 6, 2018

jboyd01 added the LGTM1 label Aug 6, 2018

k8s-ci-robot merged commit 3984a95 into kubernetes-retired:master Aug 6, 2018

jboyd01 mentioned this pull request Aug 7, 2018

get -w gives an error on Plans, Classes, Instances and Bindings #2260

Closed

cblecker unassigned jboyd01 Jun 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that ObjectMeta field selectors (e.g. name and namespace) can be used appropriately for all types #2214

Ensure that ObjectMeta field selectors (e.g. name and namespace) can be used appropriately for all types #2214

jimmidyson commented Jul 19, 2018

carolynvs left a comment

carolynvs Jul 20, 2018

carolynvs Jul 20, 2018

jimmidyson Jul 23, 2018

carolynvs Jul 23, 2018

jimmidyson Jul 23, 2018

carolynvs Jul 23, 2018

jimmidyson Aug 6, 2018

carolynvs Jul 23, 2018

jimmidyson Aug 6, 2018

carolynvs Jul 23, 2018

jimmidyson Aug 6, 2018

carolynvs Jul 23, 2018

carolynvs Jul 23, 2018

MHBauer commented Jul 24, 2018

jimmidyson commented Aug 1, 2018

jboyd01 commented Aug 1, 2018

jimmidyson commented Aug 2, 2018

jboyd01 commented Aug 3, 2018

MHBauer Aug 3, 2018

jimmidyson Aug 6, 2018

jboyd01 commented Aug 6, 2018

jimmidyson commented Aug 6, 2018

jimmidyson commented Aug 6, 2018

jboyd01 commented Aug 6, 2018

k8s-ci-robot commented Aug 6, 2018

Ensure that ObjectMeta field selectors (e.g. name and namespace) can be used appropriately for all types #2214

Ensure that ObjectMeta field selectors (e.g. name and namespace) can be used appropriately for all types #2214

Conversation

jimmidyson commented Jul 19, 2018

carolynvs left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MHBauer commented Jul 24, 2018

jimmidyson commented Aug 1, 2018

jboyd01 commented Aug 1, 2018

jimmidyson commented Aug 2, 2018

jboyd01 commented Aug 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jboyd01 commented Aug 6, 2018

jimmidyson commented Aug 6, 2018

jimmidyson commented Aug 6, 2018

jboyd01 commented Aug 6, 2018

k8s-ci-robot commented Aug 6, 2018