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

Fix bugzilla 1507822 - Update plan info on service instance update #2409

Merged
merged 1 commit into from
Oct 31, 2017

Conversation

jeff-phillips-18
Copy link
Member

Fixes bugzilla 1507822 where the service plan information is not updated automatically when the service instance is edited.

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 31, 2017
return;
}
var updateServicePlan = function() {
Catalog.getServicePlansForServiceClass($scope.serviceClass).then(function (plans) {
Copy link
Member

Choose a reason for hiding this comment

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

This re-requests plans for the service class on every watch update, correct? It would be better to only request them once on page load and remember them. Watches can trigger often due to status updates on the instance when it's being provisioned.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

@openshift-ci-robot openshift-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 Oct 31, 2017
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

LGTM, just one nit

var servicePlanName = _.get($scope.serviceInstance, 'spec.clusterServicePlanRef.name');
$scope.plan = _.find($scope.servicePlans, function(plan) {
return _.get(plan, 'metadata.name') === servicePlanName;
});
Copy link
Member

Choose a reason for hiding this comment

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

You can use the shorthand here:

$scope.plan = _.find($scope.servicePlans, { metadata: { name: servicePlanName } });

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, updated.

var servicePlanName = _.get($scope.serviceInstance, 'spec.clusterServicePlanRef.name');
$scope.servicePlans = _.reject(plans, function(plan) {
$scope.servicePlans = _.reject(plans.by('metadata.name'), function(plan) {
return _.get(plan, 'status.removedFromBrokerCatalog') && (plan.metadata.name !== servicePlanName);
Copy link
Member

Choose a reason for hiding this comment

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

I just realized if you change the instance plan twice without leaving the page, you'll probably be able to select a removed plan again if it was initially selected. Nothing related to this PR, but we might want to test and open an issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I thought of the same thing but really really really edge case. Can't think of a good fix except to re-request the plans on a change.

Copy link
Member

Choose a reason for hiding this comment

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

Check if current instance plan != $scope.plan on instance watch updates and re-filter the existing $scope.servicePlans list?

Copy link
Member Author

Choose a reason for hiding this comment

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

True, I was actually thinking of plans that were removed after the original fetch.

@spadgett
Copy link
Member

/kind bug
/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. kind/bug Categorizes issue or PR as related to a bug. labels Oct 31, 2017
@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit a3423f8 into openshift:master Oct 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants