Add the ability to edit options settings for providers #1652

enoodle · 2017-07-05T15:06:40Z

Per provider options were added in [1][2] to all providers. It is now possible to save values in ems.options to use as options.
This PR adds the ability to set options from the UI. To tell which options are supported for each provider type an API request is done [3] which expects the provider to implement the provider_settings method, for example [3],[4].
It will look for two sections in the returned value "proxy_settings" and "advanced_settings" and will display them in their own tab. for each section there are expected to be a "label" and "help_text" fields and a "settings" field with all of its options (or sub sections). The options themselves should also have "label" and "help_text" fields and also a "global_default" field that may exist.

based on:
[1] ManageIQ/manageiq#15398 - merged
[2] ManageIQ/manageiq-schema#23 - merged
[3] ManageIQ/manageiq-providers-kubernetes#45 - merged
[4] ManageIQ/manageiq-providers-openshift#32 - merged
[5] ManageIQ/manageiq#15799 - merged
[6] ManageIQ/manageiq-api#3 - merged

This aims to provide solution to a few RFEs:
https://bugzilla.redhat.com/show_bug.cgi?id=1379185
https://bugzilla.redhat.com/show_bug.cgi?id=1459189 (might be a duplicate of the previous one?)
https://bugzilla.redhat.com/show_bug.cgi?id=1459555
https://bugzilla.redhat.com/show_bug.cgi?id=1462835

enoodle · 2017-07-18T14:51:36Z

I am now having a problem with creating a new provider: the ems type that is chosen is the basic ManageIQ::Providers::ContainerManager which doesn't have any options. When the user chooses a type, I want to update the options, but all the pages are already generated. Is it possible to refresh the page with the chosen container manager type? Is it even the right solution?
@martinpovolny @himdel @mzazrivec

himdel · 2017-07-19T11:03:11Z

@enoodle The provider edit screen is always an angular screen now... So we can't easily re-render any HTML that gets rendered when opening the form.

So I think you'll need to move the logic to angular, query a json endpoint to get those options after the provider type is selected, and do all the option-dependent rendering in angular instead of server-side.

(A workaround would be to create a component that can recieve HTML from the server and render it, and route it through there, but.. I think it's easier to do it properly on the js side :).)

enoodle · 2017-07-31T10:32:35Z

@himdel This is now rendering the HTML for the options forms in angular but I am sure that there is a much better way to do it. Can you take a look?

himdel · 2017-08-07T13:45:58Z

app/assets/javascripts/controllers/ems_common/ems_common_form_controller.js

+      $scope.emsCommonModel.provider_options_description = response;
+      if ($scope.emsCommonModel.provider_options_description.hasOwnProperty('advanced_settings')) {
+        var advanced_settings = $scope.emsCommonModel.provider_options_description.advanced_settings.settings
+        for (section in advanced_settings) {


Careful about creating global variables here ;) var section

himdel · 2017-08-07T13:46:17Z

app/assets/javascripts/controllers/ems_common/ems_common_form_controller.js

+    settings_group[section_name] = options_section;
+    settings_group[section_name].section_name = section_name;
+    var section = settings_group[section_name].settings;
+    for (option in section){


Another global ;)

himdel · 2017-08-07T13:47:11Z

app/assets/javascripts/controllers/ems_common/ems_common_form_controller.js

+    }
+  };
+
+  $scope.toSnakeCase = function(st) {


You can probably use _.snakeCase() instead..

himdel · 2017-08-07T13:51:24Z

app/assets/javascripts/controllers/ems_common/ems_common_form_controller.js

@@ -11,6 +11,8 @@ ManageIQ.angular.app.controller('emsCommonFormController', ['$http', '$scope', '
      default_hostname: '',
      amqp_hostname: '',
      hawkular_hostname: '',
+      provider_options_values: {},


Not quite sure what these are, but looks like they are generic options, not related to the specific instance you're editing, is that so?

If so, you probably want $scope.somethingElse :) - the idea is that the model object should be just the data of that particular instance (not necessarily quite 1:1 with the database, but as close as possible, so that you can just post the whole thing to the server, and not have to delete fields when saving.).

I kind of need the same object to hold both the descriptions of the options and the values. This way it is much easier to iterate over the descriptions in the haml file and find the object that should hold the value.
Another option can be to hold it outside of emsCommonModel and only copy the value of the options back to it when the user clicks "save". WDYT?

Ah, got it :).

Well, it will probably work just fine as it is, I'm a bit reluctant to try to force you to make this super-clean when that whole screen is super-dirty already :).

That said, yes I would like that other approach better. If the structure is relatively flat, you could also $watch it for changes and update the model value on any change.

himdel · 2017-08-07T13:52:35Z

app/views/layouts/angular-bootstrap/_provider_advanced_settings.html.haml

+  %p
+    {{ settings_section.help_text }}
+  %div{"ng-repeat" => "option in settings_section.settings"}
+    %div{"ng-show" => "true"}


ng-show true .. Probably can get rid of it?

himdel · 2017-08-07T13:52:40Z

app/views/layouts/angular-bootstrap/_provider_proxy_settings.html.haml

+%p
+  {{ emsCommonModel.provider_options_description.proxy_settings.help_text }}
+%div{"ng-repeat" => "option in emsCommonModel.proxy_settings.settings"}
+  %div{"ng-show" => "true"}


same here..

himdel · 2017-08-07T13:55:36Z

app/views/layouts/angular-bootstrap/_provider_advanced_settings.html.haml

+                              "placeholder"   => "{{ option.global_default }}",
+                              "title"         => "{{ option.help_text }}",
+                              "prefix"        => "provider_options"}
+          %span.help-block{"ng-attr-ng-show" => "{{ \"angularForm.provider_options_\" + settings_section.section_name + \"_\" + option.name + \".$error.detectedSpaces\" }}"}


You should never need to do this... (ng-attr- with another angular directive)

"ng-show" => 'angularForm["provider_options_" + settings_section.section_name + "_" + option.name].$error.detectedSpaces' is probably what you want..

(We should really start putting these as sub-objects, and drop the idea of using prefixed fields, but .. I guess that's too big a change for the PR.)

himdel · 2017-08-07T14:07:08Z

app/views/layouts/angular-bootstrap/_provider_advanced_settings.html.haml

+    {{ settings_section.help_text }}
+  %div{"ng-repeat" => "option in settings_section.settings"}
+    %div{"ng-show" => "true"}
+      .form-group{"ng-attr-ng-class" => "{{ \"{'has_error': angularForm.provider_options_\" + settings_section.section_name + \"_\" + option.name + \".$invalid}\" }}"}


"ng-class" => '{ "has_error": angularForm["provider_options_" + settings_section.section_name + "_" + option.name].$invalid }'

himdel · 2017-08-07T14:27:58Z

@enoodle Looks like you're on the right track..

Maybe since this is a new functionality, you may want to wrap this in a component, but not sure if this is separate enough from the rest to make that practical..

We actually already have a component which takes a definition object with a list of fields, and renders those fields, but not sure if this is close enough so that you could actually reuse it, or if it's more like a place to take inspiration from.. https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/assets/javascripts/components/ansible-credential-options.js

But I have no better idea than what you're already doing, so the component thing is mostly about future tech debt and minimizing the dependency on shared angularForm, etc., it probably wouldn't make anything easier for you..

enoodle · 2017-08-09T09:55:37Z

@himdel as you can see I am using two pretty similar "haml" files here. one of them, the _provider_proxy_settings.html.haml, is kind of included in the other one.
Is there a way that I can re-use this logic but still use different angular objects?

enoodle · 2017-10-11T12:41:39Z

@AparnaKarve @himdel I fixed the CodeClimate error, PTAL

ohadlevy · 2017-10-15T09:17:29Z

app/views/layouts/angular/_per_provider_settings.html.haml

+  %ul.nav.nav-tabs
+    = miq_tab_header('proxy_settings', nil, {'ng-click' => "changeAuthTab('proxy_settings')",
+                                             'ng-if'    => "emsOptionsModel.provider_options.hasOwnProperty('proxy_settings')"}) do
+      %i{"error-on-tab" => "hawkular", :style => "color:#cc0000"}


just wondering, is this file suppose to be generic? if thats the case, why do you have hawkular here?

Thanks, It was a mistake, I changed it to "proxy_settings" and "advanced_settings" below.

enoodle · 2017-10-16T10:46:09Z

ping @AparnaKarve @himdel @ohadlevy can you take another look?

AparnaKarve · 2017-10-16T16:35:03Z

@enoodle Thank you for your patience.
I have been tied up with other deliverables required for the release and hence was not able to review this sooner.
I checked out your latest update and the above issue seems resolved, which is good.

I did see something with the way the tabs are positioned when Prometheus is selected from the Alerts dropdown, but it is purely an aesthetic issue and may not be even related to the code changes in this PR.

Notice the gap between the Default and the Alerts tab. That gap gets filled up when I select a value from the Metrics dropdown.

I think we can treat that as a separate (and unrelated) issue and get this PR merged.

@dclarizio IMO, this is GTG. Thanks.

dclarizio · 2017-10-17T16:54:04Z

@enoodle There's a lot of new code here, could you add some tests, at least basic ones? @AparnaKarve can you suggest some tests that can be added?

AparnaKarve · 2017-10-18T00:22:28Z

@enoodle On the backend side, can you write a spec that will create a Kubernetes record taking into account all those newly added fields for Proxy and Advanced?

On the JS side, I see that you have included a few tests.
Could you also write a basic GET test for Kubernetes?(you can refer to the existing $httpBackend.whenGET tests)

The emsCommonFormController angular controller would be undergoing a refactor in a couple of weeks from now. What would really help us at that point, are the JS specs that test the code for various use cases - the specs will be an assurance for us that we have not broken anything while refactoring.

I guess some of the basic use cases for this PR would be how the emsCommonModel gets populated based on what the user selects from the dropdown: Prometheus, Hawkular etc.
So a few JS specs along those lines would be nice.
Thanks.

miq-bot · 2017-10-18T11:34:06Z

This pull request is not mergeable. Please rebase and repush.

Reading the details about the provider's options from the api endpiont OPTIONS /api/providers and creating tabs for them.

This section will only be seen if the settings exists for that provider

enoodle · 2017-10-18T15:51:57Z

@AparnaKarve

can you write a spec that will create a Kubernetes record taking into account all those newly added fields for Proxy and Advanced?

I am not sure where this should be, can you direct me to the spec file?

Could you also write a basic GET test for Kubernetes?(you can refer to the existing $httpBackend.whenGET tests)

I added a few tests in similar to other examples in the spec file, PTAL

I guess some of the basic use cases for this PR would be how the emsCommonModel gets populated based on what the user selects from the dropdown: Prometheus, Hawkular etc.

This PR is for the Options only, I don't want to create Prometheus/Hawkular data that I am not sure about here. I added a test in ems_common_controller_sepc.rb that I hope covers it for the options. PTAL

AparnaKarve · 2017-10-18T18:09:33Z

I am not sure where this should be, can you direct me to the spec file?

You could add specs for Kubernetes here - spec/controllers/ems_container_controller_spec.rb

Check out some similar examples in spec/controllers/ems_infra_controller_spec.rb where there are specs written for Create/Update/Validate for SCVMM, OpenStack, RHEV, VMWare

miq-bot · 2017-10-19T08:23:54Z

Checked commits enoodle/manageiq-ui-classic@8c67c49~...0340e80 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 0 offenses detected
Everything looks fine. ⭐

enoodle · 2017-10-19T10:46:29Z

@AparnaKarve I added a create test with provider options to spec/controllers/ems_container_controller_spec.rb
PTAL

AparnaKarve · 2017-10-19T21:00:54Z

@enoodle Thanks for the specs.

@dclarizio This can be merged now. Thanks.

enoodle changed the title ~~Add the ability to edit options settings for container (?) providers~~ [WIP] Add the ability to edit options settings for container (?) providers Jul 5, 2017

miq-bot added the wip label Jul 5, 2017

enoodle closed this Jul 10, 2017

enoodle reopened this Jul 10, 2017

enoodle force-pushed the container_provider_settings branch from 1afcc76 to 1ab3b9b Compare July 11, 2017 12:03

enoodle force-pushed the container_provider_settings branch from 753aad9 to 9825a92 Compare July 18, 2017 13:58

enoodle force-pushed the container_provider_settings branch 2 times, most recently from 4f10baa to b3e8136 Compare July 31, 2017 10:21

enoodle force-pushed the container_provider_settings branch 3 times, most recently from 08e5299 to 69336b8 Compare August 2, 2017 09:08

enoodle changed the title ~~[WIP] Add the ability to edit options settings for container (?) providers~~ [WIP] Add the ability to edit options settings for providers Aug 2, 2017

enoodle force-pushed the container_provider_settings branch 2 times, most recently from 6e4febc to 6385987 Compare August 6, 2017 08:03

himdel reviewed Aug 7, 2017

View reviewed changes

enoodle force-pushed the container_provider_settings branch 3 times, most recently from 11db7d3 to 220fa90 Compare August 10, 2017 13:48

enoodle force-pushed the container_provider_settings branch from 2ddafd5 to 3b32e17 Compare October 11, 2017 11:44

ohadlevy reviewed Oct 15, 2017

View reviewed changes

enoodle force-pushed the container_provider_settings branch 2 times, most recently from 2ea25db to 1ea094b Compare October 15, 2017 13:42

dclarizio requested a review from AparnaKarve October 17, 2017 16:55

enoodle force-pushed the container_provider_settings branch from 1ea094b to 1f6202e Compare October 18, 2017 10:38

miq-bot added the unmergeable label Oct 18, 2017

enoodle force-pushed the container_provider_settings branch from 1f6202e to 2cbee6c Compare October 18, 2017 13:24

miq-bot removed the unmergeable label Oct 18, 2017

Erez Freiberger added 4 commits October 18, 2017 18:12

Add the ability to edit options settings for container providers

8c67c49

Reading the details about the provider's options from the api endpiont OPTIONS /api/providers and creating tabs for them.

create components for provider options

581faba

Moving Settings to its own section

762b2a4

This section will only be seen if the settings exists for that provider

add settings tabs to ems_container validateBasicInfo

5075dd3

enoodle force-pushed the container_provider_settings branch from 2cbee6c to c94ad9e Compare October 18, 2017 15:12

adding more tests

0340e80

enoodle force-pushed the container_provider_settings branch from c94ad9e to 0340e80 Compare October 19, 2017 08:20

dclarizio merged commit ab3f23e into ManageIQ:master Oct 19, 2017

dclarizio added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 19, 2017

enoodle mentioned this pull request Nov 14, 2017

Use Prometheus timeouts from settings ManageIQ/manageiq-providers-kubernetes#167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to edit options settings for providers #1652

Add the ability to edit options settings for providers #1652

enoodle commented Jul 5, 2017 •

edited

Loading

enoodle commented Jul 18, 2017

himdel commented Jul 19, 2017

enoodle commented Jul 31, 2017

himdel Aug 7, 2017

himdel Aug 7, 2017

himdel Aug 7, 2017

himdel Aug 7, 2017

enoodle Aug 9, 2017

himdel Aug 18, 2017 •

edited

Loading

himdel Aug 7, 2017

himdel Aug 7, 2017

himdel Aug 7, 2017

himdel Aug 7, 2017

himdel Aug 7, 2017 •

edited

Loading

himdel commented Aug 7, 2017 •

edited

Loading

enoodle commented Aug 9, 2017

enoodle commented Oct 11, 2017

ohadlevy Oct 15, 2017

enoodle Oct 15, 2017

enoodle commented Oct 16, 2017

AparnaKarve commented Oct 16, 2017

dclarizio commented Oct 17, 2017

AparnaKarve commented Oct 18, 2017

miq-bot commented Oct 18, 2017

enoodle commented Oct 18, 2017

AparnaKarve commented Oct 18, 2017

miq-bot commented Oct 19, 2017

enoodle commented Oct 19, 2017

AparnaKarve commented Oct 19, 2017

Add the ability to edit options settings for providers #1652

Add the ability to edit options settings for providers #1652

Conversation

enoodle commented Jul 5, 2017 • edited Loading

enoodle commented Jul 18, 2017

himdel commented Jul 19, 2017

enoodle commented Jul 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

himdel commented Aug 7, 2017 • edited Loading

enoodle commented Aug 9, 2017

enoodle commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enoodle commented Oct 16, 2017

AparnaKarve commented Oct 16, 2017

dclarizio commented Oct 17, 2017

AparnaKarve commented Oct 18, 2017

miq-bot commented Oct 18, 2017

enoodle commented Oct 18, 2017

AparnaKarve commented Oct 18, 2017

miq-bot commented Oct 19, 2017

enoodle commented Oct 19, 2017

AparnaKarve commented Oct 19, 2017

enoodle commented Jul 5, 2017 •

edited

Loading

himdel Aug 18, 2017 •

edited

Loading

himdel Aug 7, 2017 •

edited

Loading

himdel commented Aug 7, 2017 •

edited

Loading