-
Notifications
You must be signed in to change notification settings - Fork 382
Return dashboard url in responses to update service-instance requests #1901
Return dashboard url in responses to update service-instance requests #1901
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -4524,7 +4524,9 @@ func TestResolveReferencesNoClusterServicePlan(t *testing.T) { | |||
func TestReconcileServiceInstanceUpdatePlan(t *testing.T) { | |||
fakeKubeClient, fakeCatalogClient, fakeClusterServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ | |||
UpdateInstanceReaction: &fakeosb.UpdateInstanceReaction{ | |||
Response: &osb.UpdateInstanceResponse{}, | |||
Response: &osb.UpdateInstanceResponse{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've added feature gates for other validating-through-implementation features - will you add one for this feature? Check out pkg/features
.
@@ -536,6 +536,7 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns | |||
return c.processServiceInstanceOperationError(instance, readyCond) | |||
} | |||
|
|||
instance.Status.DashboardURL = response.DashboardURL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feature gate can be tested here.
a51df74
to
519d068
Compare
Updated this, moved the tests around. @pmorie tell me what you think |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like some stuff is inadvertently commented out.
Also, would you write an integration test for this that demonstrates that the status.dashboardUrl
changes?
@@ -536,6 +536,11 @@ func (c *controller) reconcileServiceInstanceUpdate(instance *v1beta1.ServiceIns | |||
return c.processServiceInstanceOperationError(instance, readyCond) | |||
} | |||
|
|||
if utilfeature.DefaultFeatureGate.Enabled(scfeatures.UpdateDashboardURL) { | |||
// if response.DashboardURL != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to leave these commented out?
519d068
to
2296487
Compare
@pmorie cleaned this up, wrote some more tests. I'm not 100% sure I did the integration tests right, would appreciate another look. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import the new version of the lib, then use the new lib.
LGTM, but maybe see what @pmorie thinks if he has time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM2, this looks straightforward and tests look 👍
This is a validation through implementation of OSB #437.
Also bumps pmorie/go-open-service-broker-client.