Service instance details configuration and edit #2237

jeff-phillips-18 · 2017-10-09T18:39:58Z

Depends on openshift/origin-web-catalog#474

Fixes #2278 with catalog version bump

Bumped origin-web-catalog to 0.0.56

spadgett · 2017-10-09T19:10:27Z

app/scripts/controllers/serviceInstance.js

+
+    $scope.showConfigValues = false;
+
+    $scope.toggleConfigValues = function() {


nit: I have a small preference for calling them parameter values in the code since it's what they're called in the API.

spadgett · 2017-10-09T19:12:15Z

app/scripts/controllers/serviceInstance.js

+      $scope.parameterData = angular.extend($scope.parameterData, _.get($scope.serviceInstance, 'spec.parameters', {}));
+
+      _.each(_.get($scope.serviceInstance, 'spec.parametersFrom'), function(parametersSource) {
+        secretWatcher = DataService.watchObject("secrets", _.get(parametersSource, 'secretKeyRef.name'), $scope.projectContext, function (secret) {


If there's more than one secret, you'll lose the previous secretWatcher reference and leak watches

spadgett · 2017-10-09T19:13:28Z

app/scripts/controllers/serviceInstance.js

-    var updatePlan = function() {
-      if (ServiceInstancesService.isCurrentPlan($scope.serviceInstance, $scope.plan)) {
+    var updateParameterSchema = function() {
+      $scope.parameterFormDefinition = angular.copy(_.get($scope.plan, 'spec.externalMetadata.schemas.service_instance.create.openshift_form_definition'));


For update I think this should be

spec.externalMetadata.schemas.service_instance.update.openshift_form_definition

spadgett · 2017-10-09T19:13:58Z

app/scripts/controllers/serviceInstance.js

+    var updateParameterSchema = function() {
+      $scope.parameterFormDefinition = angular.copy(_.get($scope.plan, 'spec.externalMetadata.schemas.service_instance.create.openshift_form_definition'));
+
+      $scope.parameterSchema = angular.copy(_.get($scope.plan, 'spec.instanceCreateParameterSchema'));


spec.instanceUpdateParameterSchema

spadgett · 2017-10-09T19:16:03Z

app/scripts/controllers/serviceInstance.js

+      $scope.displayName = serviceInstanceDisplayName($scope.serviceInstance, $scope.serviceClass);
+      updateBreadcrumbs();
+
+      Catalog.getServicePlans().then(function(plans) {


We should be able to only request plans for a specific service class, but OK for now. I worry down the road that this might be a lot of data, though.

spadgett · 2017-10-09T19:23:38Z

app/views/browse/service-instance.html

+        <overlay-panel show-panel="editDialogShown" handle-close="closeEditDialog">
+          <update-service
+            project="project"
+            display-name="serviceInstance | serviceInstanceDisplayName:serviceClasses"


You'll need to rebase. This now takes in one service class instead of a map.

spadgett · 2017-10-09T19:25:41Z

app/views/directives/_service-binding.html

@@ -48,4 +48,18 @@
      View Secret
    </a>
  </div>
+  <div class="service-binding-parameters" ng-if="$ctrl.bindParameterSchema.properties && (true || !($ctrl.binding | isBindingFailed) && ($ctrl.binding | isBindingReady))">


Is there a reason not to show parameter for failed bindings? It might help the user debug.

spadgett · 2017-10-09T19:26:33Z

app/views/directives/_service-binding.html

@@ -48,4 +48,18 @@
      View Secret
    </a>
  </div>
+  <div class="service-binding-parameters" ng-if="$ctrl.bindParameterSchema.properties && (true || !($ctrl.binding | isBindingFailed) && ($ctrl.binding | isBindingReady))">
+    <span class="parameters-heading">PARAMETERS</span>


<span class="parameters-heading">Parameters</span>

and use text-transform to make it uppercase in CSS

spadgett · 2017-10-09T19:26:56Z

app/views/directives/_service-binding.html

@@ -48,4 +48,18 @@
      View Secret
    </a>
  </div>
+  <div class="service-binding-parameters" ng-if="$ctrl.bindParameterSchema.properties && (true || !($ctrl.binding | isBindingFailed) && ($ctrl.binding | isBindingReady))">
+    <span class="parameters-heading">PARAMETERS</span>
+    <a href="" ng-click="$ctrl.toggleShowParameterValues()">{{$ctrl.showParameterValues ? 'Hide Values' : 'Reveal Values'}}</a>


role="button"

spadgett · 2017-10-09T19:27:15Z

app/views/directives/_service-binding.html

+  <div class="service-binding-parameters" ng-if="$ctrl.bindParameterSchema.properties && (true || !($ctrl.binding | isBindingFailed) && ($ctrl.binding | isBindingReady))">
+    <span class="parameters-heading">PARAMETERS</span>
+    <a href="" ng-click="$ctrl.toggleShowParameterValues()">{{$ctrl.showParameterValues ? 'Hide Values' : 'Reveal Values'}}</a>
+    <a ng-if="!$ctrl.binding.metadata.deletionTimestamp" href="" ng-click="$ctrl.editParameters()">Edit</a>


role="button"

jeff-phillips-18 · 2017-10-11T21:46:08Z

Believe I have addressed @spadgett comments.

Have not been able to test with multiple plans and/or editable plans.

spadgett · 2017-10-16T16:22:21Z

@jmontleon FYI, this is the plan and parameter update PR. We saw some problems (in our code) when working against your ASB changes that we're debugging now.

jeff-phillips-18 · 2017-10-17T11:11:05Z

Fixed updating the plan editability while being updated. No known issues atm, but have not tested updating the service instance's parameters with an instanceUpdateParameterSchema.

spadgett · 2017-10-18T12:25:42Z

app/scripts/controllers/serviceInstance.js

+
+      $scope.parameterData = angular.extend($scope.parameterData, _.get($scope.serviceInstance, 'spec.parameters', {}));
+
+      if (AuthorizationService.canI('secrets', 'get', $scope.projectName)) {


This means that you will not be able to see that there's a parameter at all when it comes from a secret and you can't get the secret, correct?

We could possibly fill in the redacted values using status.externalProperties.parameters when users can't list secrets (without letting them reveal). I'm OK handling this as a follow on, however.

@spadgett What should we do when the user does reveal and we have redacted values for some of the parameters? Continue to show ***** for those parameters?

Maybe remove the "reveal" action?

I think we're close enough here it should be a follow-on anyway.

spadgett · 2017-10-18T14:25:12Z

Checkboxes look a little odd to me in read only mode:

jeff-phillips-18 · 2017-10-18T19:12:48Z

The checkboxes issue is fixed in the catalog-parameters component

rhamilto · 2017-10-19T18:53:13Z

app/styles/_core.less

@@ -105,6 +105,32 @@
    border-bottom: 0;
    padding-bottom: 0;
  }
+
+  .config-parameters-form {


Nit: alpha of this rule and the rules nested within

@rhamilto do we alpha ignoring the . or are there any preferences on selector type ordering?

Generally ignore the non-alpha characters as I think most folks don't think about 'em when they're scanning the code. The overall goal is to make the Less more human-readable. You would not believe the number of times I've seen rules declared multiple times or even the same declaration twice within the same rule. Either way this rule should come first in this block. :)

rhamilto · 2017-10-19T19:03:03Z

app/views/browse/service-instance.html

+                        </dl>
+                        <div class="hidden-lg">
+                          <h3 ng-if-start="serviceClass.spec.description || serviceClass.spec.externalMetadata.longDescription">Description</h3>
+                          <dl ng-if-end class="dl-horizontal left">


A dl should only contain dt, dd, or div/script/template elements. So either don't use a dl or change the p elements to divs and add a margin using a helper class (e.g., mar-bottom-md)? I think I prefer the former as that maintains the semantic value of the p.

No need for the dl. Just remove it and render the p as individual elements.

rhamilto · 2017-10-19T19:03:50Z

app/views/browse/service-instance.html

+                      <div class="col-lg-6">
+                        <div class="hidden-xs hidden-sm hidden-md">
+                          <h3 ng-if-start="serviceClass.spec.description || serviceClass.spec.externalMetadata.longDescription">Description</h3>
+                          <dl ng-if-end class="dl-horizontal left">


Same dl issue as before.

jeff-phillips-18 · 2017-10-19T19:37:58Z

Addressed @rhamilto comments. Removed WIP status 🙌 🤘

spadgett · 2017-10-19T19:40:44Z

app/scripts/controllers/serviceInstance.js

-      if (ServiceInstancesService.isCurrentPlan($scope.serviceInstance, $scope.plan)) {
-        return;
-      }
+        Catalog.getServicePlans().then(function (plans) {


When we fix the removed plans, let's use a field filter on list to get just the plans for this service class and filter removed plans client side (follow on)

spadgett · 2017-10-19T19:43:01Z

app/scripts/directives/serviceBinding.js

+        _.each(_.get(ctrl.binding, 'spec.parametersFrom'), function (parametersSource) {
+          DataService.get('secrets', _.get(parametersSource, 'secretKeyRef.name'), context).then(function (secret) {
+            try {
+              _.extend(ctrl.parameterData, SecretsService.decodeSecretData(secret.data).parameters);


This is not always parameters. It is the secretKeyRef.key

Surprised you don't need a JSON.parse here

Yes. Thanks, I do need a JSON.parse here.

rhamilto

🥇

spadgett · 2017-10-19T19:51:22Z

Looks like a dist mismatch

jeff-phillips-18 · 2017-10-19T20:30:46Z

/retest

spadgett · 2017-10-19T21:28:04Z

/lgtm

openshift-merge-robot · 2017-10-19T21:55:47Z

Automatic merge from submit-queue.

jeff-phillips-18 requested review from rhamilto, spadgett and cdcabrera October 9, 2017 18:39

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 9, 2017

spadgett reviewed Oct 9, 2017

View reviewed changes

jeff-phillips-18 force-pushed the parameters branch from 278a077 to 33a944f Compare October 11, 2017 21:26

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 11, 2017

jeff-phillips-18 force-pushed the parameters branch from 33a944f to 24901be Compare October 11, 2017 21:43

jeff-phillips-18 force-pushed the parameters branch from 24901be to 5f705f4 Compare October 12, 2017 16:55

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 12, 2017

jeff-phillips-18 force-pushed the parameters branch from 5f705f4 to 314219b Compare October 13, 2017 17:57

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2017

jeff-phillips-18 force-pushed the parameters branch from 314219b to 4aa04c1 Compare October 13, 2017 21:09

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2017

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2017

jeff-phillips-18 force-pushed the parameters branch from 4aa04c1 to 2e578cf Compare October 17, 2017 11:08

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2017

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 17, 2017

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2017

jeff-phillips-18 force-pushed the parameters branch 3 times, most recently from 7fe74fb to c237c65 Compare October 17, 2017 19:47

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2017

jeff-phillips-18 force-pushed the parameters branch from c237c65 to e0ce938 Compare October 17, 2017 20:51

spadgett reviewed Oct 18, 2017

View reviewed changes

jeff-phillips-18 force-pushed the parameters branch from e0ce938 to 9699099 Compare October 19, 2017 18:36

rhamilto reviewed Oct 19, 2017

View reviewed changes

rhamilto suggested changes Oct 19, 2017

View reviewed changes

jeff-phillips-18 force-pushed the parameters branch from 9699099 to 56b9d70 Compare October 19, 2017 19:34

jeff-phillips-18 changed the title ~~[WIP - DO NOT MERGE] Service instance details configuration and edit~~ Service instance details configuration and edit Oct 19, 2017

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 19, 2017

spadgett requested changes Oct 19, 2017

View reviewed changes

rhamilto approved these changes Oct 19, 2017

View reviewed changes

jeff-phillips-18 force-pushed the parameters branch from 56b9d70 to cbe5aba Compare October 19, 2017 20:03

This was referenced Oct 19, 2017

Need to handle removed plans when updating a service instance. #2316

Closed

Need to handle parameters display when a user cannot view secrets #2317

Closed

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2017

Service instance details configuration and edit

ce5a3ef

jeff-phillips-18 force-pushed the parameters branch from cbe5aba to ce5a3ef Compare October 19, 2017 20:33

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2017

spadgett approved these changes Oct 19, 2017

View reviewed changes

openshift-ci-robot assigned spadgett Oct 19, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2017

openshift-merge-robot merged commit f127c70 into openshift:master Oct 19, 2017


		$scope.showConfigValues = false;

		$scope.toggleConfigValues = function() {


		$scope.parameterData = angular.extend($scope.parameterData, _.get($scope.serviceInstance, 'spec.parameters', {}));

		if (AuthorizationService.canI('secrets', 'get', $scope.projectName)) {

Service instance details configuration and edit #2237

Service instance details configuration and edit #2237

Conversation

jeff-phillips-18 commented Oct 9, 2017 • edited

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

jeff-phillips-18 commented Oct 11, 2017

spadgett commented Oct 16, 2017

jeff-phillips-18 commented Oct 17, 2017

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

spadgett commented Oct 18, 2017

jeff-phillips-18 commented Oct 18, 2017

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

jeff-phillips-18 commented Oct 19, 2017

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

rhamilto left a comment

Choose a reason for hiding this comment

spadgett commented Oct 19, 2017

jeff-phillips-18 commented Oct 19, 2017

spadgett commented Oct 19, 2017

openshift-merge-robot commented Oct 19, 2017

jeff-phillips-18 commented Oct 9, 2017 •

edited