Move serviceBroker references to clusterServiceBroker in controllers #1911

eriknelson · 2018-04-04T15:33:13Z

This PR renames controller_broker.go to controller_clusterservicebroker.go and updates relevant controller references to make room for the namespaced servicebroker controller.

eriknelson · 2018-04-04T16:31:57Z

Investigating the travis failure.

nilebox · 2018-04-05T15:05:14Z

pkg/controller/controller_clusterservicebroker.go

@@ -39,7 +39,7 @@ const (
 	errorFetchingCatalogReason  string = "ErrorFetchingCatalog"
 	errorFetchingCatalogMessage string = "Error fetching catalog. "


Since you removed trailing space below, do it there too?

nilebox · 2018-04-05T15:06:43Z

pkg/controller/controller_clusterservicebroker.go

@@ -39,7 +39,7 @@ const (
 	errorFetchingCatalogReason  string = "ErrorFetchingCatalog"
 	errorFetchingCatalogMessage string = "Error fetching catalog. "
 	errorSyncingCatalogReason   string = "ErrorSyncingCatalog"
-	errorSyncingCatalogMessage  string = "Error syncing catalog from ServiceBroker. "
+	errorSyncingCatalogMessage  string = "Error syncing catalog from ServiceBroker."


s/ServiceBroker/ClusterServiceBroker

nilebox · 2018-04-05T15:08:44Z

pkg/controller/controller_clusterservicebroker.go

@@ -39,7 +39,7 @@ const (
 	errorFetchingCatalogReason  string = "ErrorFetchingCatalog"
 	errorFetchingCatalogMessage string = "Error fetching catalog. "
 	errorSyncingCatalogReason   string = "ErrorSyncingCatalog"
-	errorSyncingCatalogMessage  string = "Error syncing catalog from ServiceBroker. "
+	errorSyncingCatalogMessage  string = "Error syncing catalog from ServiceBroker."


Also I suspect we will have the same reasons/messages for namespaced service brokers, so we should probably prefix the variable names with cluster- too.

nilebox · 2018-04-05T15:11:06Z

pkg/controller/controller_clusterservicebroker.go

@@ -39,7 +39,7 @@ const (
 	errorFetchingCatalogReason  string = "ErrorFetchingCatalog"
 	errorFetchingCatalogMessage string = "Error fetching catalog. "
 	errorSyncingCatalogReason   string = "ErrorSyncingCatalog"
-	errorSyncingCatalogMessage  string = "Error syncing catalog from ServiceBroker. "
+	errorSyncingCatalogMessage  string = "Error syncing catalog from ServiceBroker."

 	errorListingClusterServiceClassesReason  string = "ErrorListingServiceClasses"


Probably missed in #1910: should error reason and message have Cluster?

eriknelson · 2018-04-05T20:10:00Z

@nilebox Updated a bunch of the reason/messages with Cluster language. Let me know if that's what you had in mind?

nilebox · 2018-04-06T00:06:46Z

@eriknelson looks good, but the tests are failing now, you need to update constants in them as well

Waited unsuccessfully for occurrence of "FetchedCatalog" in: "apiVersion: servicecatalog.k8s.io/v1beta1

eriknelson · 2018-04-13T19:05:00Z

rebased

n3wscott · 2018-04-16T23:01:11Z

pkg/controller/controller_clusterservicebroker.go

-	errorDeletingClusterServiceClassMessage  string = "Error deleting service class."
-	errorDeletingClusterServicePlanReason    string = "ErrorDeletingServicePlan"
-	errorDeletingClusterServicePlanMessage   string = "Error deleting service plan."
+	errorFetchingClusterCatalogReason  string = "ErrorFetchingClusterCatalog"


I think the error should still be ErrorFetchingCatalog

same comment for the other errors

Same - I don't think these constants should change.

pmorie

I don't think the constants should be changing here.

pmorie · 2018-04-17T14:53:02Z

cmd/svcat/testdata/responses/clusterservicebrokers.json

@@ -30,8 +30,8 @@
            "type": "Ready",
            "status": "True",
            "lastTransitionTime": "2018-01-11T20:53:31Z",
-            "reason": "FetchedCatalog",
-            "message": "Successfully fetched catalog entries from broker."
+            "reason": "FetchedClusterCatalog",


We shouldn't change this - it will break existing users of the API and I doubt this distinction needs to surface to users in any event.

pmorie · 2018-04-17T14:53:39Z

cmd/svcat/testdata/responses/clusterservicebrokers/ups-broker.json

-        "reason": "FetchedCatalog",
-        "message": "Successfully fetched catalog entries from broker."
+        "reason": "FetchedClusterCatalog",
+        "message": "Successfully fetched cluster catalog entries from broker."


Same thing with 'cluster' here - I don't think the additional wording is going to make a difference to users beyond confusing them.

pmorie · 2018-04-17T14:53:58Z

contrib/jenkins/test_walkthrough.sh

@@ -131,7 +131,7 @@ USER_PROVIDED_SERVICE_ID="4f6e6cf6-ffdd-425f-a2c7-3c9258ad2468"
 kubectl create -f "${ROOT}/contrib/examples/walkthrough/ups-broker.yaml" \
  || error_exit 'Error when creating ups-broker.'

-wait_for_expected_output -e 'FetchedCatalog' \
+wait_for_expected_output -e 'FetchedClusterCatalog' \


pmorie · 2018-04-17T14:54:03Z

docs/cli.md

@@ -59,7 +59,7 @@ $ svcat get brokers

 ```console
 $ svcat sync broker ups-broker
-Successfully fetched catalog entries from the ups-broker broker
+Successfully fetched cluster catalog entries from the ups-broker broker


pmorie · 2018-04-17T14:54:45Z

pkg/controller/controller_clusterservicebroker.go

-	errorDeletingClusterServiceClassMessage  string = "Error deleting service class."
-	errorDeletingClusterServicePlanReason    string = "ErrorDeletingServicePlan"
-	errorDeletingClusterServicePlanMessage   string = "Error deleting service plan."
+	errorFetchingClusterCatalogReason  string = "ErrorFetchingClusterCatalog"


Same - I don't think these constants should change.

pmorie · 2018-04-19T16:02:44Z

cmd/svcat/testdata/output/describe-broker.txt

-  Status:   Ready - Successfully fetched catalog entries from broker @ 2018-01-11 20:53:31 +0000 UTC  
+  Name:     ups-broker                                                                                        
+  URL:      http://ups-broker-ups-broker.ups-broker.svc.cluster.local                                         
+  Status:   Ready - Successfully fetched cluster catalog entries from broker @ 2018-01-11 20:53:31 +0000 UTC  


This still has 'cluster' in it, we should remove this.

eriknelson · 2018-04-26T15:34:11Z

@pmorie @MHBauer Rebased this and I think I hit your feedback items.

pmorie · 2018-04-27T18:33:21Z

pkg/controller/controller_clusterservicebroker.go

-	errorDeletingClusterServiceClassMessage  string = "Error deleting service class."
-	errorDeletingClusterServicePlanReason    string = "ErrorDeletingServicePlan"
-	errorDeletingClusterServicePlanMessage   string = "Error deleting service plan."
+	errorSyncingCatalogMessage  string = "Error syncing catalog from ClusterServiceBroker."


Also thought we agreed not to change these constants

* Updating broker messages/reasons with Cluster prefix

eriknelson · 2018-04-29T20:25:17Z

@pmorie reversed the const changes and rebased again

pmorie

Just a couple minor nits and we're good on this one.

pmorie · 2018-04-30T15:34:15Z

docs/walkthrough.md

@@ -88,8 +88,8 @@ spec:
 status:
  conditions:
  - lastTransitionTime: 2017-11-01T14:12:30Z
-    message: Successfully fetched catalog entries from broker.
-    reason: FetchedCatalog
+    message: Successfully fetched cluster catalog entries from broker.


Now this doc should have the changes reverted and we'll be good to go.

pmorie · 2018-04-30T15:35:08Z

pkg/controller/controller_clusterservicebroker.go

@@ -38,9 +38,9 @@ import (
 // be easily combined with a follow on specific message.
 const (
 	errorFetchingCatalogReason  string = "ErrorFetchingCatalog"
-	errorFetchingCatalogMessage string = "Error fetching catalog. "
+	errorFetchingCatalogMessage string = "Error fetching catalog."


Note the trailing spaces; those are intentional because these strings are use to construct error messages. Would you restore the trailing spaces?

👍 Didn't realize those were deliberate; that makes sense.

* Update walkthrough doc to remove references to "cluster catalog" * Restore trailing spaces used in composite error messages

pmorie · 2018-04-30T15:45:49Z

Thanks, I'll merge on green

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

nilebox reviewed Apr 5, 2018

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 5, 2018

eriknelson force-pushed the controller-csb branch from 21d9b64 to bdbb550 Compare April 6, 2018 01:34

nilebox added the LGTM1 label Apr 6, 2018

eriknelson force-pushed the controller-csb branch from bdbb550 to 3571c47 Compare April 13, 2018 17:38

n3wscott reviewed Apr 16, 2018

View reviewed changes

pmorie reviewed Apr 17, 2018

View reviewed changes

pmorie reviewed Apr 19, 2018

View reviewed changes

MHBauer added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2018

eriknelson force-pushed the controller-csb branch from 3571c47 to ceb3557 Compare April 24, 2018 15:43

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 24, 2018

eriknelson force-pushed the controller-csb branch 2 times, most recently from 8fb113e to 7860a5b Compare April 24, 2018 17:50

eriknelson mentioned this pull request Apr 26, 2018

Add ServiceBroker controller loop #1993

Merged

pmorie reviewed Apr 27, 2018

View reviewed changes

eriknelson added 6 commits April 29, 2018 12:44

Rename broker controller files

d9079e8

Remove extra space

7a685d7

Update controller refs to ClusterServiceBroker

d6b60d6

Add forgotten rename update

4e5b28b

Fix doc href after file rename

648ae00

PR feedback

9536ef4

* Updating broker messages/reasons with Cluster prefix

eriknelson added 4 commits April 29, 2018 12:44

Add missing gofmt

d8eb2df

Update status text matching new reason/messages

f91c5fe

Revert "ClusterCatalog" to "Catalog"

1dd741a

Revert const val changes

da2d2dd

eriknelson force-pushed the controller-csb branch from 7c11a82 to da2d2dd Compare April 29, 2018 16:45

pmorie reviewed Apr 30, 2018

View reviewed changes

Additional feedback items

bb917d1

* Update walkthrough doc to remove references to "cluster catalog" * Restore trailing spaces used in composite error messages

pmorie added the LGTM2 label Apr 30, 2018

pmorie merged commit 57eea81 into kubernetes-retired:master Apr 30, 2018

eriknelson deleted the controller-csb branch April 30, 2018 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move serviceBroker references to clusterServiceBroker in controllers #1911

Move serviceBroker references to clusterServiceBroker in controllers #1911

eriknelson commented Apr 4, 2018

eriknelson commented Apr 4, 2018

nilebox Apr 5, 2018

nilebox Apr 5, 2018

nilebox Apr 5, 2018

nilebox Apr 5, 2018

eriknelson commented Apr 5, 2018

nilebox commented Apr 6, 2018

eriknelson commented Apr 13, 2018

n3wscott Apr 16, 2018

n3wscott Apr 16, 2018

pmorie Apr 17, 2018

pmorie left a comment

pmorie Apr 17, 2018

pmorie Apr 17, 2018

pmorie Apr 17, 2018

pmorie Apr 17, 2018

pmorie Apr 17, 2018

pmorie Apr 19, 2018

eriknelson commented Apr 26, 2018

pmorie Apr 27, 2018

eriknelson commented Apr 29, 2018

pmorie left a comment

pmorie Apr 30, 2018

pmorie Apr 30, 2018

eriknelson Apr 30, 2018

pmorie commented Apr 30, 2018

		@@ -39,7 +39,7 @@ const (
		errorFetchingCatalogReason string = "ErrorFetchingCatalog"
		errorFetchingCatalogMessage string = "Error fetching catalog. "

Move serviceBroker references to clusterServiceBroker in controllers #1911

Move serviceBroker references to clusterServiceBroker in controllers #1911

Conversation

eriknelson commented Apr 4, 2018

eriknelson commented Apr 4, 2018

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

eriknelson commented Apr 5, 2018

nilebox commented Apr 6, 2018

eriknelson commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmorie 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

eriknelson commented Apr 26, 2018

Choose a reason for hiding this comment

eriknelson commented Apr 29, 2018

pmorie 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

pmorie commented Apr 30, 2018