-
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
Check services for bindability before creating bindings #1599
Conversation
@@ -25,7 +25,7 @@ | |||
</li> | |||
<!-- FIXME: Can't enable canI checks on svc cat resources until we have aggregation | |||
<li ng-if="(row.state.serviceInstances | hashSize) > 0 && {resource: 'bindings', group: 'servicecatalog.k8s.io'} | canI : 'create'" role="menuitem"> --> | |||
<li ng-if="('pod_presets' | enableTechPreviewFeature) && (row.state.serviceInstances | hashSize) > 0" role="menuitem"> | |||
<li ng-if="('pod_presets' | enableTechPreviewFeature) && (row.state.bindableServiceInstances | hashSize) > 0" 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.
bindableServiceInstances should be an array now, since you used _.filter, so you can just check row.state.bindableServiceInstances.length
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.
Updated, thanks.
@@ -71,6 +71,7 @@ function BindService($scope, | |||
.concat(replicaSets) | |||
.concat(statefulSets); | |||
ctrl.applications = _.sortByAll(apiObjects, ['metadata.name', 'kind']); |
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.
Curious about reason for making these bools into strings "true" : "false"
?
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 see https://github.com/openshift/origin-web-common/blob/master/src/components/binding/bindServiceForm.html#L20, but not sure why a string here as well.
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.
There are three options:
- 'true' = bind to a selected application
- 'false' = bind, but not to an application
- 'none' = do not bind
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.
Curious if that is something that can be clarified. "false" meaning "yes, but no" and none meaning "false" (ie, what you thought false would have meant) seems confusing.
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 something like
bindingType = 'application' | 'secret-only' | 'none'
to avoid confusion around types?
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.
right, something that clearly indicates intent.
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, I can update https://github.com/openshift/origin-web-common/blob/master/src/components/binding/bindServiceForm.js to use these values.
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.
c67e694
to
659256d
Compare
Removed WIP status, ready for final reviews. |
app/scripts/controllers/overview.js
Outdated
return; | ||
} | ||
state.orderedServiceInstances = _.toArray(state.serviceInstances).sort(function(left, right) { | ||
|
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.
Curious, rather than creating a duplicate array serviceInstancesArray
, _.filter
works fine on a hash as does _.sortBy
instead of [].sort
. Possible to use the _
version?
I've seen this done a few times now but haven't looked closely before.
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.
Updated to use _.sortByAll. Thanks!
@@ -168,6 +169,8 @@ | |||
}); | |||
} | |||
else { | |||
ctrl.bindType = 'application'; | |||
ctrl.appToBind = ctrl.target; |
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.
Lines 173
and 184
seem to be the same & I believe one can be removed.
I'd actually prefer to see some of this logic broken out into separate functions. Its a gnarly big if/else & there might be some long term maintenance benefits to breaking it down into smaller pieces.
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.
Updated, thanks!
|
||
var context = { | ||
namespace: _.get(svcToBind, 'metadata.namespace') | ||
}; | ||
|
||
BindingService.bindService(context, _.get(svcToBind, 'metadata.name'), _.get(appToBind, 'metadata.name')).then(function(binding){ | ||
BindingService.bindService(context, _.get(svcToBind, 'metadata.name'), application).then(function(binding){ |
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.
So DataService patterns don't lead with context
, they tend towards something like (resource, name, context, opts)
. We missed this & should prob open an issue in common to keep these consistent. I'm curious if application
even ends up as part of context in the end, but I'd have to look.
We should prioritize getting this in because binding is currently broken in the web console. We updated common without making the corresponding the corresponding console change for openshift/origin-web-common#90 |
@spadgett ah yes, sorry about that. |
[merge] |
Evaluated for origin web console merge up to c79065a |
Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1488/) (Base Commit: 23aa410) |
No description provided.