-
Notifications
You must be signed in to change notification settings - Fork 230
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
Add delete binding action to provisioned services #1705
Add delete binding action to provisioned services #1705
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.
@benjaminapetersen Not sure if you were ready for review, but wanted to get you feedback since DCUT looms menacingly... :)
angular.module('openshiftConsole') | ||
.factory('ServiceInstancesService', function() { | ||
return { | ||
// NOTE: should this be a filter? or will shared functionality grow here? |
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.
I think this should be a filter. Maybe more than one: serviceClassDisplayName
and serviceInstanceDisplayName
?
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.
Agree. This will become serviceInstanceDisplayName
.
For a serviceClassDisplayName
filter, are you thinking we need it because the externalMetadata
field is a different than the typical metadata.name
?
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.
Moved the function to a filter, left a comment for a TODO
on serviceClassDisplayName
per my previous comment.
'use strict'; | ||
|
||
angular.module('openshiftConsole') | ||
.factory('BindingModalUtils', [ |
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.
Instead of making it specific to the binding modal, this could be a general utility that combines and (optionally?) sorts collections of API objects.
We have this need for config maps and secrets in some places where we combine them in a select.
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.
Is this only used in the bind modal and no longer needed for unbind? I suggest for now we leave the working bind code alone if we're not reusing it for unbind.
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.
Yeah, probably a premature refactor on my part, assuming I would need it for unbind
. I will return this to its previous state.
var apiObjects = deploymentConfigs.concat(deployments) | ||
.concat(replicationControllers) | ||
.concat(replicaSets) | ||
.concat(statefulSets); |
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.
Probably want to check if these are arrays before calling concat
. Or at least make a comment that the inputs must be arrays.
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.
agree, I'll start with [].concat(deploymentConfigs)
to ensure we don't error here (after moving it back to bindService
).
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.
I think it's more than this... If you pass an object as an argument to concat, does it just append the object?
Maybe a moot point if you're rolling back the retractor though
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.
Yeah... everything below has a _.toArray
wrap (original code before my change). If we do move it out in future, agree, perhaps the _.toArray
should come with it & happen internally.
.concat(replicaSets) | ||
.concat(statefulSets); | ||
ctrl.applications = _.sortByAll(apiObjects, ['metadata.name', 'kind']); | ||
ctrl.bindType = ctrl.applications.length ? "application" : "secret-only"; |
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.
You are no longer setting ctrl.bindType
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.
fixed
@@ -55,20 +59,6 @@ | |||
} | |||
}; | |||
|
|||
|
|||
var deploymentConfigs, deployments, replicationControllers, replicaSets, statefulSets; | |||
var sortApplications = function() { |
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.
Suggest leaving sortApplications
as a function then calling your new service function from here so you don't have to repeat the parameters in every list callback.
var sortApplication = function() {
ctrl.applications = BindingModalUtils.sortApplications(deploymentConfigs, deployments, replicationControllers, replicaSets, statefulSets);
ctrl.bindType = _.isEmpty(ctrl.applications) ? "secret-only" : "application";
};
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.
I moved it back to bindService
as it originally was. Def think this is a good call if we end up sharing it in the future.
// var deploymentConfigs, deployments, replicationControllers, replicaSets, statefulSets; | ||
|
||
|
||
// TODO: once we have podPreset we will be able to associate the |
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.
Remove the TODO comments?
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.
Eliminated this whole section, don't think any of it will ever be needed w/the maps from overview.
bindings: { | ||
target: '<', | ||
bindings: '<', | ||
applications: '<', |
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.
bindings
and applications
are maps. Looking at just the names, I thought these were flat lists. Can we use similar names to what we use on the overview so it's clear they are the same maps?
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.
bindingsByInstanceRef
&& applicationsByBinding
, will do... something like:
<unbind-service
target="row.overlay.state.target"
bindings-by-instance="row.bindings"
applications-by-binding="row.state.applicationsByBinding"
on-close="row.closeOverlayPanel"></unbind-service>
is a little wordy, but agree the clarity is probably better.
}; | ||
|
||
ctrl.appNameForBinding = function(bindingName) { | ||
return _.get(_.first(_.get(ctrl.applications, bindingName)), ['metadata', 'name']) || bindingName; |
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.
I find the nested lodash calls on one line here and just below hard to follow.
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.
It looks like we're showing just the first app name? Usually there is only one app, but if unbind is going to affect multiple apps, it's important to know.
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.
Updating to this:
@ncameronbritt for input if this deviates or is new. In theory, if a binding represents multiple apps, this is a bit wasteful of space as they could be listed horizontally, but I don't know of another pattern off the top of my head so going with consistency for now. We can iterate if needed.
@@ -28,6 +28,9 @@ | |||
<li ng-if="('pod_presets' | enableTechPreviewFeature) && row.state.bindableServiceInstances.length" role="menuitem"> | |||
<a href="" ng-click="row.showOverlayPanel('bindService', {target: row.apiObject})">Create Binding</a> | |||
</li> | |||
<li ng-if="('pod_presets' | enableTechPreviewFeature) && row.state.bindableServiceInstances.length" role="menuitem"> |
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.
Maybe use ng-if-start
on the li
above and ng-if-end
since the conditions are the same?
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.
will do
@spadgett yup! bring on the reviews. Not exactly ready like I'd like to be, but time is of the essence. I'll hit these items while I work on cleanup. |
3516a5a
to
780b597
Compare
@spadgett yeah good Q. We have all of these: I like your suggestion, I think it would improve density / clarity. |
<div> | ||
<div ng-if="!ctrl.error"> | ||
<h3 class="mar-top-none"> | ||
Binding for {{ctrl.appKindForBinding(ctrl.selectedBinding) | humanizeKind : true}} <strong>{{ctrl.appNameForBinding(ctrl.selectedBinding)}}</strong> has been deleted. |
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 you're still calling a method you removed, appNameForBinding
?
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.
Fixing that now, working on listing here as well.
On the last binding dialog, we show the binding name, but we don't show the name anywhere on the overview currently, so I'm not going to know which to delete. |
I think the first wizard step label should either be "Services" or "Applications" depending on what we're showing to be consistent with the Create Binding dialog. |
<div class="component-label"> | ||
Secret | ||
</div> | ||
<p>{{appForBinding.metadata.name || binding.metadata.name}}</p> |
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.
Should this be...
<p>{{binding.spec.secretName}}</p>
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.
Yup will do
Need to disable the Delete button when nothing is selected or preselect the first item. I think preselecting the first is OK? @ncameronbritt @jwforres |
I think preselecting if there is only one makes sense. I think if there is more than one we wouldn't preselect, and the "Delete" button would be disabled. I believe that is consistent with the Bind flow. |
@spadgett @ncameronbritt if first should be "Applications" or "Services", should last step also be "Results"? The word "Confirmation" implies to me I would have one more changes to back out of the action. |
Yes, should be "Results". I believe that's what we use everywhere else. |
@spadgett @ncameronbritt working on the pre-select / disable now. I think that requires this |
780b597
to
4e3ef43
Compare
@@ -337,6 +337,7 @@ <h2 class="text-center" id="temporary-loading-message" style="display: none;">Lo | |||
<script src="scripts/directives/buildStatus.js"></script> | |||
<script src="scripts/directives/routeServiceBarChart.js"></script> | |||
<script src="scripts/directives/bindService.js"></script> | |||
<script src="scripts/directives/unbindService.js"></script> |
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.
I prob should rename this file since we no longer call it "unbind"
app/views/overview/_list-row.html
Outdated
<div ng-if="row.overlay.panelName === 'unbindService'"> | ||
<unbind-service | ||
target="row.overlay.state.target" | ||
bindings-by-instance="row.bindings" |
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.
Ah, so it's not a map. I'd call the attribute bindings if it's just the array of bindings. Sorry if I misunderstood.
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.
Ok, here will be:
<unbind-service
target="row.overlay.state.target"
binding="row.bindings"
on-close="row.closeOverlayPanel"></unbind-service>
and in the service instance row template:
<unbind-service
target="row.overlay.state.target"
bindings="row.bindings"
applications-by-binding="row.state.applicationsByBinding"
on-close="row.closeOverlayPanel"></unbind-service>
So we will have bindings
and applicationsByBinding
.
{{appForBinding.metadata.name}} <small class="text-muted">– {{ appForBinding.kind | humanizeKind : true}}</small> | ||
</div> | ||
<div ng-if="!(ctrl.appsForBinding(ctrl.selectedBinding))"> | ||
{{ctrl.selectedBinding}} <small class="text-muted">– Secret</small> |
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.
This is always the binding name here and not a secret correct?
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.
I think you can put Binding
here for now and we can circle back. I have some concerns that we don't show the binding name anywhere in the UI, but we can fix as a follow on to get the basic functionality in.
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.
I'll swap this & leave a TODO:
because its weird to select "secret" then see "binding" deleted, agree. :/
4e3ef43
to
f3a3f5b
Compare
- Update delete binding from service-instance-row to use app names - Add unbind service to list-row - Update unbindService to support multiple apps per binding & display app names - Add serviceInstanceDisplayName filter
Thanks for the fast turnaround @benjaminapetersen [merge] |
Evaluated for origin web console merge up to f3a3f5b |
Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1536/) (Base Commit: b93ac53) |
woohoo! 😋 |
https://trello.com/c/JMKxStDt
unbind-service
branch w/spadgett'sUpdate delete binding from service-instance-row to use app names
cherry-pick.I may close the previous
unbind-service
PR & keep this on to avoid the need to fiddle around with merge conflicts again.Prev #1505