Network Router: Use API; Fix fixed IPs subnet #2422

gildub · 2017-10-17T08:24:05Z

Networks > Network Routers

Replaces http calls in RoR controller by API calls
See #737
Also:

Filtering only external network and subsequently better proper subnet (floating IPs) handling.
External gateway displays only when external management provider id has been selected
Changes source NAT field position as it depends on the network id
Adds administrative state field

Before:

Now:

gildub · 2017-10-19T07:26:23Z

@mansam, @himdel, could you please assign/review? Thanks

mansam · 2017-10-20T17:46:14Z

Looks like maybe the routes for network_router_form_fields and other removed controller methods should be removed as part of this PR.

gildub · 2017-10-21T05:56:37Z

@mansam, right, was on my list but forgot. Thanks

gildub · 2017-10-24T06:36:40Z

@mansam, I removed the routes, thanks.

gildub · 2017-10-24T07:04:38Z

More PRs to follow for the controller to component' and then move the rest of the http requests (create/update/delete) to API only.

@martinpovolny, @himdel, @ZitaNemeckova

gildub · 2017-11-01T09:48:54Z

@miq-bot add_label refactoring

gildub · 2017-11-03T02:58:40Z

@mansam, @himdel, @martinpovolny, could you please assign someone for review please. Thanks

martinpovolny · 2017-11-04T21:12:07Z

@AparnaKarve, @Hyperkid123, @ZitaNemeckova : please, review

ZitaNemeckova · 2017-11-06T12:28:04Z

app/assets/javascripts/controllers/network_router/network_router_form_controller.js

-      .catch(miqService.handleFailure);
+    API.get("/api/network_routers/" + networkRouterFormId + "?attributes=name,ems_id,admin_state_up,cloud_network_id,extra_attributes,cloud_tenant,ext_management_system,cloud_subnets").then(function(data) {
+      Object.assign(vm.networkRouterModel, data);
+      if (data.extra_attributes.external_gateway_info && data.extra_attributes.external_gateway_info !== {}) {


Be careful {} === {} is always false . You can use lodash function _.isEmpty(data.extra_attributes.external_gateway_info) to find out if it is an empty hash.

Ok, using lodash, thanks

ZitaNemeckova · 2017-11-06T12:29:37Z

Before:

After:

Buttons Save and Reset shouldn't be enabled if no change was made. vm.modelCopy is not equal to vm.networkRouterModel. Will be resolved by #2637 (merged and backported to Gaprindashvili, please rebase this PR)

Edit: Resolved.

Code climate issues can be ignored.

Code wise:

Looking for values at angularForm is not good. Please save those values in controller so they can be accessed via vm.

Not sure how much it should be enforced... But there is missing init. You can see it here and here It will make component conversion easier :)

ZitaNemeckova · 2017-11-06T12:31:30Z

app/assets/javascripts/controllers/network_router/network_router_form_controller.js

+
+  var getCloudSubnetsByNetworkID = function(id) {
+    if (id) {
+      API.get("/api/cloud_subnets?expand=resources&attributes=name,ems_ref&filter[]=cloud_network_id=" + id).then(function(data) {


Using better vm.modelCopy after rebase.

I've started using init() in other places, done it here too.

Not using angularForm data.

Missing return

ZitaNemeckova · 2017-11-06T12:32:15Z

app/assets/javascripts/controllers/network_router/network_router_form_controller.js

  }

+  var getCloudNetworksByEms = function(id) {
+    if (id) {
+      API.get("/api/cloud_networks?expand=resources&attributes=name,ems_ref&filter[]=external_facing=true&filter[]=ems_id=" + id).then(function(data) {


If you want to wait for API call to be finished before the next then is called it should return Promise like this return API.get(... and return getCloudNetworksByEms(... when you call it.

Using return with Promise allows to pass the value to the next then, there is not need here for passing such data which only populate fields (or select lists).

I wasn't clear, sorry :) It's not about the data being sent back but about having control over them and knowing that you already have them. You want to populate fields before you display form and switch off spinner. Without suggested returns there is no guarantee that all fields will be populated before user sees them and they may change under his nose. Please add returns :)

I see what you mean with the chaining happening only if Promise is returned. Thank you!

Misssing return

ZitaNemeckova · 2017-11-06T12:32:25Z

app/assets/javascripts/controllers/network_router/network_router_form_controller.js

+    }
+  };
+
+  var getSubnetByRef = function(ref) {


Same as above too :)

Missing return

ZitaNemeckova · 2017-11-06T12:43:07Z

app/assets/javascripts/controllers/network_router/network_router_form_controller.js

+      }
+    }).then(function() {
+      if (vm.networkRouterModel.external_gateway) {
+        getSubnetByRef(vm.networkRouterModel.extra_attributes.external_gateway_info.external_fixed_ips[0].subnet_id);


return getSubnetByRef(... so it waits on Promise that you should return from this function. If you want one .then to be finished before next one starts.

ZitaNemeckova · 2017-11-06T14:02:40Z

app/assets/javascripts/controllers/network_router/network_router_form_controller.js

+      }).catch(miqService.handleFailure);
+    }
+  };
+
  vm.addClicked = function() {
    var url = 'create/new' + '?button=add';


This should be one string. No need for +. Could you please fix it? :)

gildub · 2017-11-09T03:47:03Z

@ZitaNemeckova, please review all suggestions have been addressed. I can't wait to have migrated all those networks controller to components, especially from a test view point.

ZitaNemeckova · 2017-11-13T14:43:03Z

app/assets/javascripts/controllers/network_router/network_router_form_controller.js

+        vm.available_tenants = data.resources;
+      })(id);
+    }
+    miqService.sparkleOff();


Also here is no use of Promise and no guarantee that all data will be set before miqService.sparkleOff().

var providerTenants = miqService.getProviderTenants(function(data) { vm.available_tenants = data.resources; })(id); Promise.all([getCloudNetworksByEms(id), providerTenants]) .then(miqService.sparkleOff)

But both must return API calls.

Yes the sparkleOn/Off flow was really bad, and this is actually quite messy in general. Since API request are already Promise I have simplified it.

This sparkleOn/sparkleOff pair is still not doing anything and can be removed :)

(Or better yet, fixed to do the right thing :))

ZitaNemeckova · 2017-11-17T13:06:11Z

@gildub I cannot add new network Router. Always ends with this error:

F, [2017-11-17T14:01:19.193444 #44722] FATAL -- : Error caught: [ActiveRecord::RecordNotFound] Couldn't find ExtManagementSystem without an ID
/usr/local/lib/ruby/gems/2.3.0/gems/activerecord-5.0.6/lib/active_record/relation/finder_methods.rb:456:in `find_with_ids'
/usr/local/lib/ruby/gems/2.3.0/gems/activerecord-5.0.6/lib/active_record/relation/finder_methods.rb:66:in `find'
/usr/local/lib/ruby/gems/2.3.0/gems/activerecord-5.0.6/lib/active_record/querying.rb:3:in `find'
/usr/local/lib/ruby/gems/2.3.0/gems/activerecord-5.0.6/lib/active_record/core.rb:151:in `find'
/Users/zita/Desktop/ManageIQ/manageiq-ui-classic/app/controllers/network_router_controller.rb:68:in `create'

I see only difference in send data ems_id: 10000014 => ext_management_system[]: 10000014. Is this on purpose?

ZitaNemeckova · 2017-11-17T13:34:01Z

@gildub There are still returns of Promises missing.

gildub · 2017-11-22T00:44:31Z

@ZitaNemeckova,

Added missing returns
Fixed id issue blocking creation
Placement is showing only after ems_id is provided since it depends upon it
Renamed getSubnetByRef -> getCloudSubnetsByRef
Fixed Id when Network changes

Retested create/update

miq-bot · 2017-11-23T10:24:12Z

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

miq-bot · 2018-07-09T02:22:41Z

Checked commits https://github.com/gildub/manageiq-ui-classic/compare/f7747a1a774e1b833b0b2a58ea923d2cb8f01dde~...3633a9fdc4db85921d55eadb0639062f8e7cf3d2 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

gildub · 2018-07-09T02:24:43Z

@ZitaNemeckova,

The reason is: at creation time there are no Provider relationship yet and therefore there are no router.external_management_system object yet so we use ems_id for creation. Conversely in the update case the relation exists and we can get router.external_management_system.id.

The same approach has been used in other places.

ZitaNemeckova · 2018-07-18T13:31:09Z

app/views/network_router/_common_new_edit.haml

-                "ng-change"                   => "vm.filterCloudNetworkChanged(vm.networkRouterModel.cloud_network_id)",
-                "pf-select"                   => true,
-                "ng-selected"                 => "vm.networkRouterModel.cloud_network_id",
-                "ng-required"                 => "vm.networkRouterModel.enable_snat == true",


@gildub Did you remove ng-required on purpose?

@ZitaNemeckova, yes the network is part of the gateway information which is optional

Hyperkid123 · 2018-07-19T08:15:57Z

The form works fine. I could not find anything that is not mentioned here. But i would appreciate some specs for the JavaScript side. Even though there is just a small amount of logic, i think it would be nice to cover it and make it a bit more future proof.

gildub · 2018-07-19T12:46:55Z

@Hyperkid123, the driver here is to re-factor to add API get, the next step will be to replace with controller to component conversion (such as [1] for example) where the framework for testing is much better therefore I think it might not worth the effort.

[1] #2828

Hyperkid123 · 2018-07-19T12:49:25Z

@gildub that sounds great

gildub · 2018-07-19T22:35:16Z

@ZitaNemeckova, @Hyperkid123, then if everybody is happy any chance we can merge? Thanks

Hyperkid123 · 2018-07-20T09:44:03Z

I think its fine. There some codeclimate issues that should be fixed though (the missing semicolons for example, not the similar code warnings etc.).

But i think it should be merged.

miq-bot added the wip label Oct 17, 2017

gildub changed the title ~~[WIP] Network Router: Use API~~ Network Router: Use API; Fix fixed IPs subnet Oct 19, 2017

miq-bot removed the wip label Oct 19, 2017

chessbyte added the networks label Oct 31, 2017

miq-bot added the refactoring label Nov 1, 2017

martinpovolny requested a review from AparnaKarve November 4, 2017 21:11

ZitaNemeckova reviewed Nov 6, 2017

View reviewed changes

martinpovolny assigned ZitaNemeckova Nov 6, 2017

ZitaNemeckova mentioned this pull request Nov 6, 2017

Add watch to modelCopy in form_changed #2637

Merged

ZitaNemeckova reviewed Nov 6, 2017

View reviewed changes

ZitaNemeckova reviewed Nov 13, 2017

View reviewed changes

miq-bot added the unmergeable label Nov 23, 2017

gildub and others added 19 commits July 9, 2018 12:02

Using bracket for literals

a39d274

Needs , multiline params and flow fix

565a222

rebased after #3736, using static maxlength

7ee595f

DRY url for add/cancel/save

1921dc6

JS better flow and cleansing

f2a9e02

Use ems_id only

7b80a4e

Added admin state up

aaa26ff

Single quotes preferrably

b5cb691

move vm functions

6b1c9d0

Placement depends upon ems

79da90c

fix wrong field names

1be914b

Promise: return needed; Admin up/down

801922d

Consistent ext_management_system and default snat

a2bdcf8

Use getCloudSubnetsByNetworkID directly

eab2980

function scope

77c7aa5

edit: default enable_snat is true

8c6a438

Use nesting fetch; Fixed mandatory field in form

479b15b

Use ems_id only

b11a636

No options filtering in cancel mode

3633a9f

ZitaNemeckova reviewed Jul 18, 2018

View reviewed changes

himdel merged commit 6a4c870 into ManageIQ:master Jul 20, 2018

himdel added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 20, 2018

gildub deleted the Network_Router-API branch July 24, 2018 07:59

Network Router: Use API; Fix fixed IPs subnet #2422

Network Router: Use API; Fix fixed IPs subnet #2422

Conversation

gildub commented Oct 17, 2017 • edited Loading

gildub commented Oct 19, 2017 • edited Loading

mansam commented Oct 20, 2017

gildub commented Oct 21, 2017

gildub commented Oct 24, 2017

gildub commented Oct 24, 2017

gildub commented Nov 1, 2017

gildub commented Nov 3, 2017

martinpovolny commented Nov 4, 2017

ZitaNemeckova Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZitaNemeckova commented Nov 6, 2017 • edited Loading

ZitaNemeckova Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

gildub Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZitaNemeckova Nov 6, 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

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

Choose a reason for hiding this comment

gildub commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gildub Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

himdel Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZitaNemeckova commented Nov 17, 2017 • edited Loading

ZitaNemeckova commented Nov 17, 2017

gildub commented Nov 22, 2017

miq-bot commented Nov 23, 2017

miq-bot commented Jul 9, 2018

gildub commented Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hyperkid123 commented Jul 19, 2018

gildub commented Jul 19, 2018

Hyperkid123 commented Jul 19, 2018

gildub commented Jul 19, 2018

Hyperkid123 commented Jul 20, 2018

gildub commented Oct 17, 2017 •

edited

Loading

gildub commented Oct 19, 2017 •

edited

Loading

ZitaNemeckova Nov 6, 2017 •

edited

Loading

ZitaNemeckova commented Nov 6, 2017 •

edited

Loading

ZitaNemeckova Nov 6, 2017 •

edited

Loading

gildub Nov 9, 2017 •

edited

Loading

ZitaNemeckova Nov 6, 2017 •

edited

Loading

gildub Nov 14, 2017 •

edited

Loading

himdel Apr 17, 2018 •

edited

Loading

ZitaNemeckova commented Nov 17, 2017 •

edited

Loading

gildub commented Jul 9, 2018 •

edited

Loading