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.

Sign up for GitHub

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

Jump to bottom

Support binding parameters #1987

Merged
merged 1 commit into from Sep 7, 2017
Merged

Support binding parameters #1987

merged 1 commit into from Sep 7, 2017

Conversation

spadgett
Copy link
Member

@spadgett spadgett commented Aug 29, 2017 •
edited by rhamilto

https://trello.com/c/Sws4p5jU

  • Put parameters in a secret
  • Add binding parameters to in-context flows in origin-web-catalog

Requires openshift/origin-web-common#163

Bumps origin-web-common 0.0.52 -> 0.0.53
Bumps origin-web-catalog 0.0.42 -> to 0.0.43
which also
Fixes #1979
Fixes #2002

@spadgett spadgett mentioned this pull request Aug 30, 2017
Merged
benjaminapetersen
benjaminapetersen approved these changes Sep 1, 2017
View reviewed changes
Copy link
Contributor

@benjaminapetersen benjaminapetersen left a comment •
edited

Choose a reason for hiding this comment

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

LGTM, just some passing thoughts.

app/scripts/directives/bindService.js
ctrl.serviceClass = ctrl.serviceClasses[instance.spec.serviceClassName];
ctrl.serviceClassName = instance.spec.serviceClassName;
ctrl.plan = BindingService.getPlanForInstance(instance, ctrl.serviceClass);
ctrl.parameterSchema = _.get(ctrl.plan, 'alphaBindingCreateParameterSchema');
Copy link
Contributor

Choose a reason for hiding this comment

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

They don't come up too often, but do you think we ever need to doc these alpha kinds of properties with a TODO of FIXME? Last I remember was the osb prefixes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely reasonable. We've been trying to track the changes in the card because there are so many coming:

https://trello.com/c/y1lbFrbp

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, that works for me. Something like FIXME: API ALPHA could be a nice way to search, but might not be worth it if already tracked like the Trello.

app/scripts/directives/bindService.js Outdated
ctrl.nextTitle = 'Bind';
ctrl.nextTitle = bindParametersStep.hidden ? 'Bind' : 'Next >';
if (!selectionValidityWatcher) {
selectionValidityWatcher = $scope.$watch("ctrl.selectionForm.$valid", function(isValid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm tempted to suggest we start using a pattern of var scopeWatches = [] and a loop in $onDestroy to deregister, just to make all of our components consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically the scope watches clean themselves up on destroy and don't need to be explicitly deregistered. I wonder if it's worth tracking them except in the cases where we want to disable / enable them on certain steps.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah prob not so much. I think I'm pointlessly tracking one in the drawer, should prob nix it.

@spadgett
Support binding parameters
096fc19 
Adds bind parameters to overview "Create Binding" action and in context
provision binding.

Bumps origin-web-common 0.0.52 -> 0.0.53
Bumps origin-web-catalog 0.0.42 -> to 0.0.43

https://trello.com/c/Sws4p5jU
@spadgett spadgett changed the title [WIP] Support binding parameters Support binding parameters Sep 6, 2017
@spadgett
Copy link
Member Author

spadgett commented Sep 6, 2017

[merge]

@openshift-bot
Copy link

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link

Evaluated for origin web console test up to 096fc19

@openshift-bot
Copy link

Origin Web Console Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_web_console/141/) (Base Commit: 38eb4eb) (PR Branch Commit: 096fc19)

@spadgett spadgett mentioned this pull request Sep 6, 2017
Merged
@spadgett
Copy link
Member Author

spadgett commented Sep 6, 2017

[merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to 096fc19

@openshift-bot
Copy link

openshift-bot commented Sep 6, 2017 •
edited

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/134/) (Base Commit: a6bf807) (PR Branch Commit: 096fc19)

@openshift-bot openshift-bot merged commit 8a15031 into openshift:master Sep 7, 2017
@spadgett spadgett deleted the bind-parameters branch September 7, 2017 00:23
@spadgett spadgett mentioned this pull request Oct 6, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjaminapetersen benjaminapetersen benjaminapetersen approved these changes

@jeff-phillips-18 jeff-phillips-18 jeff-phillips-18 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@spadgett @openshift-bot @benjaminapetersen @jeff-phillips-18