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

OpenShift Deployment Wizard #7620

Merged
merged 16 commits into from Sep 7, 2016

Conversation

Projects
None yet
@dkorn
Contributor

dkorn commented Mar 31, 2016

This commit introduces the new OpenShift Deployment wizard UI.

To show the Deployment Wizard:
manageiq create containers provider

1A General
deployment_wizard_1

1B New VM Settings
deployment_wizard_2

1B Masters & Nodes
deployment_wizard_3

1C CDN Channel
deployment_wizard_4

2A Authentication Type
deployment_wizard_5

2B Authentication Details
deployment_wizard_7

2C Settings
deployment_wizard_12

3A Summary
deployment_wizard_15

3B Progress
deployment_wizard_16

@miq-bot

This comment has been minimized.

Show comment
Hide comment
@miq-bot

miq-bot Apr 21, 2016

Member

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

Member

miq-bot commented Apr 21, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@dkorn

This comment has been minimized.

Show comment
Hide comment
@dkorn

dkorn Apr 21, 2016

Contributor

@alongoldboim updated

Contributor

dkorn commented Apr 21, 2016

@alongoldboim updated

@alongoldboim

This comment has been minimized.

Show comment
Hide comment
@alongoldboim

alongoldboim Apr 21, 2016

Contributor

@dkorn thanks, ill rebase on top of it.

Contributor

alongoldboim commented Apr 21, 2016

@dkorn thanks, ill rebase on top of it.

@dkorn

View changes

Show outdated Hide outdated app/helpers/application_helper/toolbar/ems_containers_center.rb Outdated
@martinpovolny

This comment has been minimized.

Show comment
Hide comment
@martinpovolny

martinpovolny May 3, 2016

Contributor

@dkorn : so the changes from #8079 got broken by rebase on master you did. I fixed that by rebasing #8079 (there's a 2nd place to add the attribute now).

Also I added hack by @himdel to support data-function= in the toolbar as discussed on gitter.

Please, retry

Contributor

martinpovolny commented May 3, 2016

@dkorn : so the changes from #8079 got broken by rebase on master you did. I fixed that by rebasing #8079 (there's a 2nd place to add the attribute now).

Also I added hack by @himdel to support data-function= in the toolbar as discussed on gitter.

Please, retry

@serenamarie125

This comment has been minimized.

Show comment
Hide comment
@serenamarie125

serenamarie125 May 9, 2016

@dkorn Spoke with @beanh66
Please use the same icon for both menu items ( pficon-add-circle-o ), but make label changes:
Change “Add a New Containers Provider” to “Add Existing Containers Provider"
Change “Deploy a New Containers Provider” to “Create Containers Provider"

Note that at this point, PatternFly is suggesting using the same icon for both Add and Create

@simon3z please be aware of our suggestions above ^^

serenamarie125 commented May 9, 2016

@dkorn Spoke with @beanh66
Please use the same icon for both menu items ( pficon-add-circle-o ), but make label changes:
Change “Add a New Containers Provider” to “Add Existing Containers Provider"
Change “Deploy a New Containers Provider” to “Create Containers Provider"

Note that at this point, PatternFly is suggesting using the same icon for both Add and Create

@simon3z please be aware of our suggestions above ^^

@chessbyte chessbyte closed this May 9, 2016

@chessbyte chessbyte reopened this May 9, 2016

@zeari

View changes

Show outdated Hide outdated app/assets/javascripts/application.js Outdated
@zeari

View changes

Show outdated Hide outdated app/assets/javascripts/controllers/deployment/deploy-provider-controller.js Outdated
@dkorn

This comment has been minimized.

Show comment
Hide comment
@dkorn

dkorn May 10, 2016

Contributor

@dkorn Spoke with @beanh66
Please use the same icon for both menu items ( pficon-add-circle-o ), but make label changes:
Change “Add a New Containers Provider” to “Add Existing Containers Provider"
Change “Deploy a New Containers Provider” to “Create Containers Provider"

Note that at this point, PatternFly is suggesting using the same icon for both Add and Create

@simon3z please be aware of our suggestions above ^^

@serenamarie125 @simon3z changed, please approve:
manageiq create containers provider2

Contributor

dkorn commented May 10, 2016

@dkorn Spoke with @beanh66
Please use the same icon for both menu items ( pficon-add-circle-o ), but make label changes:
Change “Add a New Containers Provider” to “Add Existing Containers Provider"
Change “Deploy a New Containers Provider” to “Create Containers Provider"

Note that at this point, PatternFly is suggesting using the same icon for both Add and Create

@simon3z please be aware of our suggestions above ^^

@serenamarie125 @simon3z changed, please approve:
manageiq create containers provider2

@dkorn

This comment has been minimized.

Show comment
Hide comment
@dkorn

dkorn May 30, 2016

Contributor

@dclarizio @h-kataria this is still a WIP but I wanted to ask, is it a must to convert the .html files to .haml?

Contributor

dkorn commented May 30, 2016

@dclarizio @h-kataria this is still a WIP but I wanted to ask, is it a must to convert the .html files to .haml?

@gtanzillo gtanzillo closed this Jun 2, 2016

@gtanzillo gtanzillo reopened this Jun 2, 2016

@himdel

This comment has been minimized.

Show comment
Hide comment
@himdel

himdel Jun 14, 2016

Contributor

is it a must to convert the .html files to .haml?

@dkorn it is the currently preferred way to do i18n .. so please do :) .. and there are strings you may want to wrap in _("...") in there..

(There's a gem (also a (different) online tool) called html2haml which may help .. but it does need some cleanup after..)

Contributor

himdel commented Jun 14, 2016

is it a must to convert the .html files to .haml?

@dkorn it is the currently preferred way to do i18n .. so please do :) .. and there are strings you may want to wrap in _("...") in there..

(There's a gem (also a (different) online tool) called html2haml which may help .. but it does need some cleanup after..)

@miq-bot

This comment has been minimized.

Show comment
Hide comment
@miq-bot

miq-bot Jun 20, 2016

Member

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

Member

miq-bot commented Jun 20, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@pilhuhn

View changes

Show outdated Hide outdated app/helpers/application_helper/toolbar/ems_containers_center.rb Outdated
@dkorn

This comment has been minimized.

Show comment
Hide comment
@dkorn

dkorn Jun 27, 2016

Contributor

@dkorn it is the currently preferred way to do i18n .. so please do :) .. and there are strings you may want to wrap in _("...") in there..

(There's a gem (also a (different) online tool) called html2haml which may help .. but it does need some cleanup after..)

Thanks @himdel, .html files now converted to .haml
Also. I added i18n support for all strings

Contributor

dkorn commented Jun 27, 2016

@dkorn it is the currently preferred way to do i18n .. so please do :) .. and there are strings you may want to wrap in _("...") in there..

(There's a gem (also a (different) online tool) called html2haml which may help .. but it does need some cleanup after..)

Thanks @himdel, .html files now converted to .haml
Also. I added i18n support for all strings

@miq-bot

This comment has been minimized.

Show comment
Hide comment
@miq-bot

miq-bot Jun 29, 2016

Member

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

Member

miq-bot commented Jun 29, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@dkorn dkorn changed the title from [WIP] OpenShift Deployment Wizard to OpenShift Deployment Wizard Jul 5, 2016

@dkorn

This comment has been minimized.

Show comment
Hide comment
@dkorn

dkorn Jul 5, 2016

Contributor

@miq-bot remove_label wip

Contributor

dkorn commented Jul 5, 2016

@miq-bot remove_label wip

text-align: center;
.btn[disabled] {
pointer-events: none;

This comment has been minimized.

@himdel

himdel Aug 17, 2016

Contributor

Note that this does not prevent the user from activating the button by keyboard, and does break any cursor style and title tooltips. Sure about it?

@himdel

himdel Aug 17, 2016

Contributor

Note that this does not prevent the user from activating the button by keyboard, and does break any cursor style and title tooltips. Sure about it?

@himdel

View changes

Show outdated Hide outdated ...vascripts/controllers/container_deployment/deploy-provider-controller.js Outdated
@himdel

This comment has been minimized.

Show comment
Hide comment
@himdel

himdel Aug 17, 2016

Contributor

All the text in static/deploy_containers_provider/deploy-provider-master-nodes-review-tables.html.haml is currently broken..

_("%{master}") % {:master => "{{data.masters.length === 1 ? 'Master' : 'Masters'}}"} means that Master or Masters will never be translated

_(%{node}) means _("node") and never even touches the stuff after %.. (missing quotes, but then we're back to the master problem..)

And DNS and Storage should probably get similar logic..

The right way to fix it would be using n_() but we don't have that yet in JS.. for now, at least something like data.masters.length === 1 ? __("Master") : __("Masters") (without that left side)?

Contributor

himdel commented Aug 17, 2016

All the text in static/deploy_containers_provider/deploy-provider-master-nodes-review-tables.html.haml is currently broken..

_("%{master}") % {:master => "{{data.masters.length === 1 ? 'Master' : 'Masters'}}"} means that Master or Masters will never be translated

_(%{node}) means _("node") and never even touches the stuff after %.. (missing quotes, but then we're back to the master problem..)

And DNS and Storage should probably get similar logic..

The right way to fix it would be using n_() but we don't have that yet in JS.. for now, at least something like data.masters.length === 1 ? __("Master") : __("Masters") (without that left side)?

@himdel

This comment has been minimized.

Show comment
Hide comment
@himdel

himdel Aug 17, 2016

Contributor

Apart from those 3, I'm not seeing any other problems.. (and probably only the last one is a blocker)

Contributor

himdel commented Aug 17, 2016

Apart from those 3, I'm not seeing any other problems.. (and probably only the last one is a blocker)

@dclarizio

This comment has been minimized.

Show comment
Hide comment
@dclarizio

dclarizio Aug 17, 2016

Contributor

@dkorn will await replies to @himdel latest comments prior to merging. Can you also clarify this:

Worth mentioning - we currently decided to hide the wizard from the UI, and plan to change it during September

Does this mean it's turned off somehow and you will revisit, update, and re-enable it later?

Contributor

dclarizio commented Aug 17, 2016

@dkorn will await replies to @himdel latest comments prior to merging. Can you also clarify this:

Worth mentioning - we currently decided to hide the wizard from the UI, and plan to change it during September

Does this mean it's turned off somehow and you will revisit, update, and re-enable it later?

@cben

This comment has been minimized.

Show comment
Hide comment
@cben

cben Aug 21, 2016

Contributor

The closest we got is this:

{{data.masters.length === 1 ? __('1 Master') : sprintf(__('%s Masters'), data.masters.length)}}

The expression works in JS console but the angular template silently prints nothing :-(
Angular makes it hard to debug: https://stackoverflow.com/questions/32764895/how-to-print-to-console-log-from-inside-angular-js-inline-templates-script-tag

IIUC the problem is that templates are evalated in very sandboxed context =>sprintf is not available, would have to be explicitly exposed (in contoller, or somewhere more centralized?). I think {{__('foo')}} did work, though not 100% sure now.

Quick search suggests we currently have zero __ calls inside angular templates, so this is new ground.
Please advise.

Contributor

cben commented Aug 21, 2016

The closest we got is this:

{{data.masters.length === 1 ? __('1 Master') : sprintf(__('%s Masters'), data.masters.length)}}

The expression works in JS console but the angular template silently prints nothing :-(
Angular makes it hard to debug: https://stackoverflow.com/questions/32764895/how-to-print-to-console-log-from-inside-angular-js-inline-templates-script-tag

IIUC the problem is that templates are evalated in very sandboxed context =>sprintf is not available, would have to be explicitly exposed (in contoller, or somewhere more centralized?). I think {{__('foo')}} did work, though not 100% sure now.

Quick search suggests we currently have zero __ calls inside angular templates, so this is new ground.
Please advise.

@himdel

This comment has been minimized.

Show comment
Hide comment
@himdel

himdel Aug 21, 2016

Contributor

@cben I think the problem may be in that %s expects a string but data.masters.length is a number.. try %d instead..

Basically, if __ is available, then so is sprintf, both are properties on window..

EDIT: but you can always generate the string in the controller ;)

Contributor

himdel commented Aug 21, 2016

@cben I think the problem may be in that %s expects a string but data.masters.length is a number.. try %d instead..

Basically, if __ is available, then so is sprintf, both are properties on window..

EDIT: but you can always generate the string in the controller ;)

@dclarizio

This comment has been minimized.

Show comment
Hide comment
@dclarizio

dclarizio Aug 30, 2016

Contributor

@cben will you be addressing @himdel's comment?

Contributor

dclarizio commented Aug 30, 2016

@cben will you be addressing @himdel's comment?

@dkorn

This comment has been minimized.

Show comment
Hide comment
@dkorn

dkorn Aug 31, 2016

Contributor

@dclarizio I will, thanks.

Contributor

dkorn commented Aug 31, 2016

@dclarizio I will, thanks.

@dkorn

This comment has been minimized.

Show comment
Hide comment
@dkorn

dkorn Sep 4, 2016

Contributor

@himdel No %d doesn't help. Neither sprintf nor __ are available in angular {{templates}}.
this is the solution we (@cben) found:
= _("{{data.masters.length}} {{data.masters.length === 1 ? 'Master' : 'Masters'}}")
which is an ugly string to translate but does give translator control over word order.

And DNS and Storage should probably get similar logic..

there can only be one of each so no need of plural forms.

Does this mean it's turned off somehow and you will revisit, update, and re-enable it later?

@dclarizio yes, currently till all the backend dependencies are merged, the wizard is hidden by default. It is possible to display the wizard by going to configurations -> advanced -> :product: :container_deployment_wizard: false => true

Contributor

dkorn commented Sep 4, 2016

@himdel No %d doesn't help. Neither sprintf nor __ are available in angular {{templates}}.
this is the solution we (@cben) found:
= _("{{data.masters.length}} {{data.masters.length === 1 ? 'Master' : 'Masters'}}")
which is an ugly string to translate but does give translator control over word order.

And DNS and Storage should probably get similar logic..

there can only be one of each so no need of plural forms.

Does this mean it's turned off somehow and you will revisit, update, and re-enable it later?

@dclarizio yes, currently till all the backend dependencies are merged, the wizard is hidden by default. It is possible to display the wizard by going to configurations -> advanced -> :product: :container_deployment_wizard: false => true

@himdel

This comment has been minimized.

Show comment
Hide comment
@himdel

himdel Sep 5, 2016

Contributor

"{{data.masters.length}} {{data.masters.length === 1 ? 'Master' : 'Masters'}}"

@dkorn we can't seriously give translators that kind of a string..

As I see it, there are 3 options to fix this:

  • $rootScope.__ = __; $rootScope.sprintf = sprintf; (during app.run)- and voila, you can use both in templates
  • $scope.$watch('data.masters.length', function(len) { $scope.text = (len === 1) ? __("Master") : __("Masters") }) - in the controller, and then just {{text}} (but with a better name)
  • use ng-show/hide to show the right translation in the template..
    %div{"ng-show" => "data.masters.length === 1"}
      = _('Master')
    %div{"ng-hide" => "data.masters.length === 1"}
      = _('Masters')

I think the third one is the clearest but feel free to choose :)

Contributor

himdel commented Sep 5, 2016

"{{data.masters.length}} {{data.masters.length === 1 ? 'Master' : 'Masters'}}"

@dkorn we can't seriously give translators that kind of a string..

As I see it, there are 3 options to fix this:

  • $rootScope.__ = __; $rootScope.sprintf = sprintf; (during app.run)- and voila, you can use both in templates
  • $scope.$watch('data.masters.length', function(len) { $scope.text = (len === 1) ? __("Master") : __("Masters") }) - in the controller, and then just {{text}} (but with a better name)
  • use ng-show/hide to show the right translation in the template..
    %div{"ng-show" => "data.masters.length === 1"}
      = _('Master')
    %div{"ng-hide" => "data.masters.length === 1"}
      = _('Masters')

I think the third one is the clearest but feel free to choose :)

dkorn added some commits Aug 3, 2016

Changes and refactors following review comments
* change API requests (GET and POST)
* move all views under deploy_containers_provider subdirectory
* add response error message
* add more i18n support
* fix router checkbox bug
* remove commented out code
* remove unused functions and dependencies
@miq-bot

This comment has been minimized.

Show comment
Hide comment
@miq-bot

miq-bot Sep 5, 2016

Member

Checked commits dkorn/manageiq@6f663a1~...8c3066b with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
6 files checked, 8 offenses detected

app/views/static/deploy_containers_provider/deploy-provider-details-existing-vms.html.haml

  • ⚠️ - Line 109 - Line is too long. [369/160]
  • ⚠️ - Line 57 - Line is too long. [162/160]

app/views/static/deploy_containers_provider/deploy-provider-details-no-provider.html.haml

  • ⚠️ - Line 101 - Line is too long. [369/160]
  • ⚠️ - Line 56 - Line is too long. [162/160]

app/views/static/deploy_containers_provider/deploy-provider-masters-nodes.html.haml

  • ⚠️ - Line 4 - Line is too long. [186/160]

app/views/static/wizard.html.haml

  • ⚠️ - Line 15 - Line is too long. [176/160]
  • ⚠️ - Line 3 - Line is too long. [161/160]
  • ⚠️ - Line 4 - Line is too long. [240/160]
Member

miq-bot commented Sep 5, 2016

Checked commits dkorn/manageiq@6f663a1~...8c3066b with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
6 files checked, 8 offenses detected

app/views/static/deploy_containers_provider/deploy-provider-details-existing-vms.html.haml

  • ⚠️ - Line 109 - Line is too long. [369/160]
  • ⚠️ - Line 57 - Line is too long. [162/160]

app/views/static/deploy_containers_provider/deploy-provider-details-no-provider.html.haml

  • ⚠️ - Line 101 - Line is too long. [369/160]
  • ⚠️ - Line 56 - Line is too long. [162/160]

app/views/static/deploy_containers_provider/deploy-provider-masters-nodes.html.haml

  • ⚠️ - Line 4 - Line is too long. [186/160]

app/views/static/wizard.html.haml

  • ⚠️ - Line 15 - Line is too long. [176/160]
  • ⚠️ - Line 3 - Line is too long. [161/160]
  • ⚠️ - Line 4 - Line is too long. [240/160]
@dkorn

This comment has been minimized.

Show comment
Hide comment
@dkorn

dkorn Sep 5, 2016

Contributor

@himdel I chose option 3 as you recommended, just added the number of each role nodes:

%div{"ng-show" => "data.masters.length === 1"}
  = "{{data.masters.length}} " + _('Master')
%div{"ng-hide" => "data.masters.length === 1"}
  = "{{data.masters.length}} " + _('Masters')
Contributor

dkorn commented Sep 5, 2016

@himdel I chose option 3 as you recommended, just added the number of each role nodes:

%div{"ng-show" => "data.masters.length === 1"}
  = "{{data.masters.length}} " + _('Master')
%div{"ng-hide" => "data.masters.length === 1"}
  = "{{data.masters.length}} " + _('Masters')
@martinpovolny

This comment has been minimized.

Show comment
Hide comment
@martinpovolny

martinpovolny Sep 5, 2016

Contributor

see: http://manageiq.org/documentation/development/i18n/

n(msgid, msgid_plural, n) - returns either translated msgid or its translated plural form (msgid_plural) depending on n, a number determining the count (i.e. number above 1 means plural form)
Contributor

martinpovolny commented Sep 5, 2016

see: http://manageiq.org/documentation/development/i18n/

n(msgid, msgid_plural, n) - returns either translated msgid or its translated plural form (msgid_plural) depending on n, a number determining the count (i.e. number above 1 means plural form)
@cben

This comment has been minimized.

Show comment
Hide comment
@cben

cben Sep 5, 2016

Contributor

@martinpovolny n() is Ruby, right? That will not work as the number is only known in the browser.
javascript n_() requires 1st or 2nd option to be able to call it.
[I have no opinion among the 3 options.]

Contributor

cben commented Sep 5, 2016

@martinpovolny n() is Ruby, right? That will not work as the number is only known in the browser.
javascript n_() requires 1st or 2nd option to be able to call it.
[I have no opinion among the 3 options.]

@dkorn

This comment has been minimized.

Show comment
Hide comment
@dkorn

dkorn Sep 7, 2016

Contributor

@himdel bumping this again, I've made the change you asked (option 3)

Contributor

dkorn commented Sep 7, 2016

@himdel bumping this again, I've made the change you asked (option 3)

@himdel

This comment has been minimized.

Show comment
Hide comment
@himdel

himdel Sep 7, 2016

Contributor

Looks mergable, thanks, merging!

I will be assigning you to an issue listing some of the remaining problems I've found though :).

Contributor

himdel commented Sep 7, 2016

Looks mergable, thanks, merging!

I will be assigning you to an issue listing some of the remaining problems I've found though :).

@himdel himdel merged commit 36c9906 into ManageIQ:master Sep 7, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.0008%) to 36.644%
Details

@himdel himdel added this to the Sprint 46 Ending Sep 12, 2016 milestone Sep 7, 2016

@himdel

This comment has been minimized.

Show comment
Hide comment
@himdel

himdel Sep 7, 2016

Contributor

To enable in the UI, go to the server config, advanced, :product section, and set container_deployment_wizard to true..

Contributor

himdel commented Sep 7, 2016

To enable in the UI, go to the server config, advanced, :product section, and set container_deployment_wizard to true..

@dkorn

This comment has been minimized.

Show comment
Hide comment
@dkorn

dkorn Sep 7, 2016

Contributor

I will be assigning you to an issue listing some of the remaining problems I've found though :).

Great, Thanks!
I also wrote Dan how to enable in: #7620 (comment)

Contributor

dkorn commented Sep 7, 2016

I will be assigning you to an issue listing some of the remaining problems I've found though :).

Great, Thanks!
I also wrote Dan how to enable in: #7620 (comment)

@himdel

This comment has been minimized.

Show comment
Hide comment
@himdel

himdel Sep 7, 2016

Contributor

#11068 :)

Contributor

himdel commented Sep 7, 2016

#11068 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment