Add vm security group operations #1997

tzumainn · 2017-08-25T17:45:27Z

Adds VM security group operations

tzumainn · 2017-08-25T17:46:48Z

tzumainn · 2017-08-25T17:47:20Z

@h-kataria Thanks for the guidance, let me know if I did this right!

tzumainn · 2017-08-25T17:48:21Z

@miq-bot add_label UX/Review

tzumainn · 2017-09-07T01:00:23Z

@miq-bot remove_label wip

tzumainn · 2017-09-07T01:00:47Z

@h-kataria I believe this is ready for review now!

dclarizio · 2017-09-19T17:16:23Z

@tzumainn we relaxed the codeclimate checks, can you see if you can address any of the 18 issues that are reported?

tzumainn · 2017-09-20T14:33:05Z

@dclarizio thanks for pointing that out! I took a closer look at the codeclimate issues, and I'm not sure any of them can be addressed. The duplicated code is extremely minor, and feels dangerous to pull out. It looks like I already follow the suggested angular chaining - I think the lint is wrong. And the 'unless' statements look correct to me.

Let me know if there's something specific in there you think I missed!

dclarizio · 2017-09-20T15:55:09Z

@martinpovolny Can you review the CC issues? I know we relaxed CC checks, does CC need to be rerun?

serenamarie125 · 2017-09-21T21:15:32Z

@tzumainn how are they accessing this operation?

tzumainn · 2017-09-22T00:17:29Z

@serenamarie125 From a single VM view, under the Configuration menu - same place where associate/disassociate floating IP and similar operations,

martinpovolny · 2017-09-25T07:44:36Z

@tzumainn, @dclarizio : More that about CC issues -- yes, there are 18 -- I am concerned about creating more and more new RJS code.

This PR introduces a lot of new technical debt. Is there a reason to do this? Are we in a hurry? Is this a Sev 1 fix?

martinpovolny · 2017-09-25T07:49:42Z

To be more specific:

Why is the API not used?
There should be no new render :update code at all unless you are adding a small thing to a complex controller where everything is done that way.
app/controllers/mixins/actions/vm_actions/remove_security_group.rb and app/controllers/mixins/actions/vm_actions/add_security_group.rb look like c-n-ped.

martinpovolny

Please, see comments I have added.

martinpovolny · 2017-09-25T07:50:50Z

app/assets/javascripts/controllers/vm_cloud/vm_cloud_remove_security_group_form_controller.js

+  vm.resetClicked = function(angularForm) {
+    vm.CloudModel = angular.copy( vm.modelCopy );
+    angularForm.$setPristine(true);
+    miqService.miqFlash("warn", "All changes have been reset");


martinpovolny · 2017-09-25T07:55:11Z

app/assets/javascripts/controllers/vm_cloud/vm_cloud_remove_security_group_form_controller.js

@@ -0,0 +1,40 @@
+ManageIQ.angular.app.controller('vmCloudRemoveSecurityGroupFormController', ['$http', 'vmCloudRemoveSecurityGroupFormId', 'miqService', function($http, vmCloudRemoveSecurityGroupFormId, miqService) {


We are trying to convert to Angular 1.6 so that we can move forward with Angular 2 and React. This should be a component. The partials below shoud not be created, those should be part of the component or separate componets too.

Ping @himdel, @AparnaKarve, @Hyperkid123.

There are no controllers in Angular 2+, so i completely agree that we should try to write any new angular controllers as components, since we will have to refactor them anyway... And that applies especially for partials, because refactoring them later after they go through several modifications is pain in the ass.

You can use this article as reference. There are also some components already in ui-classic(and more comming).

Working on the guide, but for now, pretty much all you need to do is:

rename init to $onInit - that gets called automagically by angular, so you no longer need to call it manually.

move the templates under app/views/static/ - that way, you can reference it via templateUrl: '/static/....' from angular

remove any controller-dependent logic from the moved template - that just means you can't use @record.id in your case

make it into a component...

ManageIQ.angular.app.controller("foo", **controller**);

should become

ManageIQ.angular.app.component("foo", { bindings: { recordId: "@", } templateUrl: "/static/foo.html.haml", controller: **controller**, });

then you no longer need to pass the id using value, you can provide it to the component as a parameter - hence the bindings entry - so no injecting vmCloudRemoveSecurityGroupFormId any more ;)

replace the original template with just something that uses the component .. so, %my-component{:record-id => @record.id}, and initializes angular (so that miq_bootstrap stays here, not in static - we usually use the component name as the first arg in those cases (so you don't need an extra #div))

don't leave the code in app/assets/javascripts/controllers, use the components dir for that - or if you're up to speed with the recent webpacker changes (talk), you can move it to app/javascript/ and use newer JS features.

EDIT: oh, and remove ng-controller from the template too

Also, that miqAjaxButton is an anti-pattern now, with angular components, you should no longer rely on the server generating javascript to redirect you/show a flash message/whatever... instead, you should be using render :json => ... in ruby, and read that json in JS to do the right thing.

(But, it's not miqAjaxButton(url, true) which is the worst, so.. if the server-side logic to do that is not trivial, feel free to ignore this bit for now.)

tzumainn · 2017-09-25T10:37:00Z

@martinpovolny Thanks for looking at this! I'm pretty new at JS so it's difficult for me unless I'm following existing patterns. Do you have an example of a controller being a component?

martinpovolny · 2017-09-25T11:22:20Z

app/controllers/application_controller/ci_processing.rb

@@ -1049,6 +1051,8 @@ def process_vm_buttons(pfx)
    when "#{pfx}_live_migrate"              then livemigratevms
    when "#{pfx}_associate_floating_ip"     then associate_floating_ip_vms
    when "#{pfx}_disassociate_floating_ip"  then disassociate_floating_ip_vms
+    when "#{pfx}_add_security_group"        then add_security_group_vms
+    when "#{pfx}_remove_security_group"     then remove_security_group_vms


Is this going to be relevant for multiple different entities?

What I mean: you are adding some "#{pfx}_action" here. It seems to be relevant for VMs, right? Is it going to be relevant for other entities?

Or is it just that you saw many of the VM button action being handled here?

If it's going to be relevant just for VMs then I'd prefer not to do the pfx magic.

But if you actually rewrite this to be pure client-side throught the API then you probably will not need to change this file (and the one below) at all.

@martinpovolny hi! to be honest, I don't know what this code is for; would you suggest just removing this bit?

tzumainn · 2017-10-21T05:11:54Z

depends on ManageIQ/manageiq-api#137

tzumainn · 2017-10-21T05:12:12Z

@martinpovolny I believe I've addressed your comments. Let me know if anything else should be changed.

martinpovolny · 2017-10-22T09:44:40Z

@tzumainn : Thanks! It looks much better ;-) I really appreciate that you did these changes and worked with the API.

Moving forward we should figure how to avoid all the changes you did in:

app/controllers/mixins/actions/vm_actions/add_security_group.rb
app/controllers/mixins/actions/vm_actions/remove_security_group.rb
app/controllers/vm_common.rb

and some more. As this is really just routing from one Angular-enabled page to another one by doing a HTTP request. That should not be necessary and it should be simpler.

But we can do that in a separate PR.

@himdel, @h-kataria : what do you think?

@himdel, @ZitaNemeckova: Maybe we can take this for our Monday's "workshop" with @gildub and @aufi and make it into an example on how to progress with moving on the client. A blogpost maybe?

himdel · 2017-10-23T09:56:39Z

app/helpers/application_helper/toolbar/openstack_vm_cloud_center.rb

+          t = N_('Add a Security Group to this Instance'),
+          t,
+          :klass   => ApplicationHelper::Button::GenericFeatureButtonWithDisable,
+          :options => {:feature => :add_security_group}


This is probably not needed.. instance_add_security_group is the needed rbac feature, because of the toolbar button's identifier.

Hi! I had thought that the preference in feature names was to use non-entity specific names when possible. It's certainly possible that we'll be adding security groups to other things in the future, so isn't add_security_group preferable?

Well until that happens I'd go with the simple case.

Other that that I think this is ready to merge.

I understand the concern, but I've been explicitly told to do the opposite by reviewers who reviewed changes to the feature list in the supports_feature_mixin. If you look at https://github.com/ManageIQ/manageiq/blob/master/app/models/mixins/supports_feature_mixin.rb, none of the features are entity specific.

Oh well @himdel and I both mixed up RBAC feature and product feature.

martinpovolny · 2017-10-23T18:29:15Z

I know I said "last" but before pressing the merge button I took a look at the CC and besides the similar code warning, that I don't push you to address, there are 2 trivial ones:

Trailing spaces not allowed. NEW
Expected indentation of 8 spaces but found 7. NEW

Please, fix these 2.

tzumainn · 2017-10-24T13:25:08Z

@miq-bot add_label pending/core

miq-bot · 2017-10-24T13:26:15Z

@tzumainn Cannot apply the following label because they are not recognized: pending/core

miq-bot · 2017-10-25T22:18:46Z

Checked commits tzumainn/manageiq-ui-classic@cb40a5c~...01932e2 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
7 files checked, 2 offenses detected

app/controllers/mixins/actions/vm_actions/add_security_group.rb

❗ - Line 14, Col 15 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.

app/controllers/mixins/actions/vm_actions/remove_security_group.rb

❗ - Line 14, Col 15 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.

tzumainn · 2017-10-25T22:29:49Z

@martinpovolny fixed the issues (and updated the API call in accordance to a request on the API PR). I'll let you know when that API PR is merged, thanks!

tzumainn · 2017-10-25T23:24:54Z

@martinpovolny backend API PR is merged, so I think this should be ready! The test failures look unrelated... ?

h-kataria

looks like most of the comments/concerns from @martinpovolny @himdel were addressed. Looks good

miq-bot added wip ux/review labels Aug 25, 2017

tzumainn mentioned this pull request Sep 5, 2017

Add instance security group management to product features ManageIQ/manageiq#15915

Merged

tzumainn changed the title ~~[WIP] Add vm security group operations~~ Add vm security group operations Sep 7, 2017

tzumainn closed this Sep 7, 2017

tzumainn reopened this Sep 7, 2017

miq-bot removed the wip label Sep 7, 2017

tzumainn force-pushed the vm-security-group-operations branch from 273bed6 to fd7294f Compare September 11, 2017 16:44

dclarizio requested a review from h-kataria September 19, 2017 17:11

dclarizio added compute/cloud enhancement labels Sep 19, 2017

dclarizio assigned h-kataria Sep 19, 2017

martinpovolny self-requested a review September 25, 2017 07:49

martinpovolny suggested changes Sep 25, 2017

View reviewed changes

martinpovolny reviewed Sep 25, 2017

View reviewed changes

tzumainn added 11 commits October 20, 2017 15:39

add tests for vm security group operations

5513f0b

updated vm security group operations to use api calls

ed65ab7

updated js controllers to be components

f139300

refactored according to comments

3e47249

updated javascript tests

199d283

stylistic fixes

7c82f66

fixed whitespace issue

fc81e02

updated prefixes in ci_processing

93c3b85

use javascript_redirect

ba66b7d

updated cancelClicked

d859d2e

refactored to use API for add/remove

6751442

tzumainn force-pushed the vm-security-group-operations branch from d726a73 to 6751442 Compare October 21, 2017 05:11

himdel reviewed Oct 23, 2017

View reviewed changes

martinpovolny approved these changes Oct 23, 2017

View reviewed changes

updated api parameter

01932e2

tzumainn closed this Oct 26, 2017

tzumainn reopened this Oct 26, 2017

h-kataria approved these changes Oct 26, 2017

View reviewed changes

h-kataria added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 26, 2017

h-kataria added the fine/no label Oct 26, 2017

h-kataria merged commit 1cd88de into ManageIQ:master Oct 26, 2017

		@@ -0,0 +1,40 @@
		ManageIQ.angular.app.controller('vmCloudRemoveSecurityGroupFormController', ['$http', 'vmCloudRemoveSecurityGroupFormId', 'miqService', function($http, vmCloudRemoveSecurityGroupFormId, miqService) {

Add vm security group operations #1997

Add vm security group operations #1997

Conversation

tzumainn commented Aug 25, 2017

tzumainn commented Aug 25, 2017

tzumainn commented Aug 25, 2017

tzumainn commented Aug 25, 2017

tzumainn commented Sep 7, 2017

tzumainn commented Sep 7, 2017

dclarizio commented Sep 19, 2017

tzumainn commented Sep 20, 2017

dclarizio commented Sep 20, 2017

serenamarie125 commented Sep 21, 2017

tzumainn commented Sep 22, 2017

martinpovolny commented Sep 25, 2017

martinpovolny commented Sep 25, 2017

martinpovolny left a comment

Choose a reason for hiding this comment

martinpovolny Sep 25, 2017

Choose a reason for hiding this comment

martinpovolny Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Hyperkid123 Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

himdel Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

himdel Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

tzumainn commented Sep 25, 2017

martinpovolny Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

tzumainn Oct 11, 2017

Choose a reason for hiding this comment

tzumainn commented Oct 21, 2017

tzumainn commented Oct 21, 2017

martinpovolny commented Oct 22, 2017

himdel Oct 23, 2017

Choose a reason for hiding this comment

tzumainn Oct 23, 2017

Choose a reason for hiding this comment

martinpovolny Oct 23, 2017

Choose a reason for hiding this comment

tzumainn Oct 23, 2017

Choose a reason for hiding this comment

martinpovolny Oct 23, 2017

Choose a reason for hiding this comment

martinpovolny commented Oct 23, 2017 • edited Loading

tzumainn commented Oct 24, 2017

miq-bot commented Oct 24, 2017

miq-bot commented Oct 25, 2017

tzumainn commented Oct 25, 2017

tzumainn commented Oct 25, 2017

h-kataria left a comment

Choose a reason for hiding this comment

martinpovolny Sep 25, 2017 •

edited

Loading

Hyperkid123 Sep 25, 2017 •

edited

Loading

himdel Sep 25, 2017 •

edited

Loading

himdel Sep 25, 2017 •

edited

Loading

martinpovolny Sep 25, 2017 •

edited

Loading

martinpovolny commented Oct 23, 2017 •

edited

Loading