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

Create service bindings from the overview #1395

Merged
merged 1 commit into from
Apr 7, 2017

Conversation

jwforres
Copy link
Member

@jwforres jwforres commented Apr 4, 2017

No description provided.

@jwforres
Copy link
Member Author

jwforres commented Apr 4, 2017

Extremely rough first pass at this, just getting something started. Note: does not actually set PodPreset yet but should be trivial to add that piece once we can send it to the API.

2017-04-04 00 16 58

@spadgett @serenamarie125 @ncameronbritt @pmorie

@ncameronbritt
Copy link

Nice work @jwforres! No that I see this, I wonder if it might make sense to add some text on the first step about what it means to create a binding. The design sort of explains it after you've done it, which is backwards.
@serenamarie125

@pmorie
Copy link

pmorie commented Apr 4, 2017

ship it

var newestReady;
var newestNotReady;
_.each(ctrl.serviceInstances, function(instance) {
var ready = _.get(statusCondition(instance, 'Ready'), 'status', 'False') === 'True';
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't need to provide a default since you're checking === 'True' anyway.

var ready = _.get(statusCondition(instance, 'Ready'), 'status') === 'True';

}

// wait till both service instances and service classes are available so that the sort is stable and items dont jump around
if (changes.serviceInstances && ctrl.serviceClasses || changes.serviceClasses && ctrl.serviceInstances) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to be...

if ((changes.serviceInstances || changes.serviceClasses) && ctrl.serviceClasses && ctrl.serviceInstances) {
   // ...
}

$onChanges is called for initial values, which might be uninitialized.

// TODO - would be better if generateName could take in an optional maxlength
var truncatedSvcInstanceName = _.trunc(serviceInstanceName, DNS1123_SUBDOMAIN_VALIDATION.maxlength - 6);
ctrl.generatedSecretName = generateName(truncatedSvcInstanceName + '-');
let binding = {
Copy link
Member

Choose a reason for hiding this comment

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

var binding = {

@@ -9,3 +9,12 @@
font-size: @donut-font-size-big * .75; // ~ 22.5px;
font-weight: 300;
}

.wizard-pf-steps-indicator .wizard-pf-step-number {
Copy link
Member

Choose a reason for hiding this comment

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

TODO to remove when we update Patternfly?

<button
type="button"
class="btn btn-primary wizard-pf-next"
ng-disabled="ctrl.currentStep.id === 'results'"
Copy link
Member

Choose a reason for hiding this comment

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

Should we hide the button on the last page instead of just disable? Currently what we do for order service.

<div class="sub-title">
<h3><span class="pficon pficon-ok mar-right-sm"></span>Configuration Created</h3>
The binding operation created the secret
<a ng-href="{{ctrl.generatedSecretName | navigateResourceURL : 'Secret' : ctrl.target.metadata.namespace}}">{{ctrl.generatedSecretName}}</a>
Copy link
Member

Choose a reason for hiding this comment

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

Trying to decide if it's a worth a canI check here. Presumably if you can bind, you can view secrets.

<input type="radio" ng-model="ctrl.serviceToBind" value="{{serviceInstance.metadata.name}}">
{{ctrl.serviceClasses[serviceInstance.spec.serviceClassName].osbMetadata.displayName || serviceInstance.spec.serviceClassName}}
<span ng-if="(serviceInstance | statusCondition : 'Ready').status !== 'True'" class="mar-left-sm">
<span class="pficon pficon-info" data-content="This service is not yet ready. If you bind to it, then the binding will be pending until the service is ready." class="pficon pficon-warning-triangle-o" data-toggle="popover" data-trigger="hover"></span>
Copy link
Member

Choose a reason for hiding this comment

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

You have class twice on this element.

@jwforres jwforres changed the title [DO-NOT-MERGE] [WIP] Create service bindings from the overview Create service bindings from the overview Apr 6, 2017
@jwforres
Copy link
Member Author

jwforres commented Apr 6, 2017

review comments addressed, built, and squashed

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, not sure about the travis failure

@jwforres
Copy link
Member Author

jwforres commented Apr 6, 2017

Suspect it's something with adding the catalog to the module dependency list, let's try Jenkins [test]

@jwforres
Copy link
Member Author

jwforres commented Apr 7, 2017

karma.conf.js was pretty outdated as far as the list of dependency JS files to load, went ahead and made it match the state of index.html

@openshift-bot
Copy link

Evaluated for origin web console test up to 4f53b14

@jwforres
Copy link
Member Author

jwforres commented Apr 7, 2017

travis is passing now [merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to 4f53b14

@openshift-bot
Copy link

openshift-bot commented Apr 7, 2017

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1201/) (Base Commit: 722fec3)

@openshift-bot
Copy link

Origin Web Console Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1200/) (Base Commit: 722fec3)

@openshift-bot openshift-bot merged commit 1268da0 into openshift:master Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants