Introduce UI for Nuage events #2186
Conversation
|
@miha-plesko This one is related to the Nuage PR. Can you please have a look? |
gberginc
force-pushed
the
add_nuage_events_ui
branch
from
2844384
to
0e84aa6
Compare
There was a problem hiding this comment.
Generally looks great @gberginc , please see the two minor comments below. Other than that I've also noticed that if I input an invalid endpoint in AMQP tab and then click Validate, then I get notification complaining about
VM802:4 SyntaxError: Unexpected token < in JSON at position 0
But I believe this is a backend problem and we're not able to solve it here, right?
| @@ -394,6 +394,10 @@ ManageIQ.angular.app.controller('emsCommonFormController', ['$http', '$scope', ' | |||
| if ($scope.emsCommonModel.emstype === 'openstack') { | |||
| $scope.emsCommonModel.tenant_mapping_enabled = false; | |||
| } | |||
| } else if ($scope.emsCommonModel.emstype === 'nuage_network') { | |||
| $scope.emsCommonModel.default_api_port = $scope.getDefaultApiPort($scope.emsCommonModel.emstype); | |||
| $scope.emsCommonModel.event_stream_selection = "none"; | |||
There was a problem hiding this comment.
Please use single quotes 'none' here as well. I see we're following a pattern here, but we shouldn't IMO.
| @@ -218,7 +222,7 @@ | |||
| :prefix => "amqp", | |||
| :basic_info_needed => true} | |||
| .form-group | |||
| .col-md-12{"ng-if" => "#{ng_model}" == "emsCommonModel" && "#{ng_model}.emstype == 'openstack' || #{ng_model}.emstype == 'openstack_infra'"} | |||
| .col-md-12{"ng-if" => "#{ng_model}" == "emsCommonModel" && "#{ng_model}.emstype == 'openstack' || #{ng_model}.emstype == 'openstack_infra' || #{ng_model}.emstype == 'nuage_network'"} | |||
There was a problem hiding this comment.
This prints the wrong message:
May I suggest to add additional .col-md-12 here for Nuage provider? Similarly as there is one for OpenStack in L225 and one for VMWare in L231. I think you will need something more similar to VMWare than OpenStack.
Alternatively, you could make VMWare's message little more general and then hook Nuage there.
There was a problem hiding this comment.
I completely missed this message there - thanks for spotting!
With the introduction of Nuage event catcher, users need to be able to configure the AMQP connection for their Nuage network manager. This patch adds a secondary tab for the Events allowing users to opt-in and create authentication for the AMQP. Complete validation of all credentials is supported. Signed-off-by: Gregor Berginc <gregor.berginc@xlab.si>
gberginc
force-pushed
the
add_nuage_events_ui
branch
from
0e84aa6
to
d1e49f3
Compare
|
Thanks @miha-plesko for the review! Regarding the |
|
Checked commit gberginc@d1e49f3 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 app/views/layouts/angular/_multi_auth_credentials.html.haml |
There was a problem hiding this comment.
Thanks for the update @gberginc , LGTM now. @AparnaKarve I leave it to you to either comment or merge.
|
@AparnaKarve : please, take a look at this one. I am not familiar with this form. Thx! |
There was a problem hiding this comment.
@gberginc Changes look good and relating them with your video demo helped me. So LGTM.
(I did not test this PR myself since it is dependent on the other Nuage PR - ManageIQ/manageiq-providers-nuage#9)
With the introduction of Nuage event catcher, users need to be able to configure the AMQP connection for their Nuage network manager. This patch adds a secondary tab for the Events allowing users to opt-in and create authentication for the AMQP. Complete validation of all credentials is supported.
This PR is based on a contribution from @VojkoR in this commit. It adds additional logic to configure the endpoint and authentications.
Links [Optional]