Floating IPs provisioning UI #12097

gildub · 2016-10-21T13:51:47Z

Adds controller actions using task queue
Adds new and edit views

https://bugzilla.redhat.com/show_bug.cgi?id=1394277

tzumainn · 2016-10-21T14:02:16Z

@miq-bot add_label euwe/yes

miq-bot · 2016-11-01T12:47:29Z

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

gildub · 2016-11-07T10:51:08Z

@martinpovolny, @himdel, @dclarizio, can you please review?

himdel · 2016-11-07T13:33:52Z

@gildub Can you please update the PR description with instructions where to find this functionality and a screenshot?

Not seeing anything wrong in the code, except for a few find_by_id which should be find or find_by.

tzumainn · 2016-11-07T15:57:58Z

tzumainn · 2016-11-07T15:58:45Z

Accessed through the left navigation - Networks > Floating IPs

tzumainn · 2016-11-07T15:59:13Z

@himdel added the info, let me know if it helps! I've tested this functionality successfully, so I think we're only really looking for code sanity.

himdel · 2016-11-10T13:16:59Z

app/controllers/floating_ip_controller.rb

+
+    floating_ips_to_delete = []
+    floating_ips.each do |s|
+      floating_ip = FloatingIp.find_by_id(s)


This should be find(s)

himdel · 2016-11-10T13:18:58Z

app/controllers/floating_ip_controller.rb

+  def floating_ip_form_fields
+    assert_privileges("floating_ip_edit")
+    # TODO: find_by_id_filtered fails
+    # floating_ip = find_by_id_filtered(FloatingIp, params[:id])


Any idea why it fails? Sounds like a security problem without it..

You say fixed, but I still see the commented out code here..

Btw seems like this is working fine in SecurityGroups so maybe it would now here too..?

himdel · 2016-11-10T13:33:05Z

Floating IP Address (optional)

So.. I'm a bit concerned about the address field being optional. In particular, @mansam has done changes to the UI in #12446 that depend on every floating ip having an address.

So how is this supposed to work? Is that a valid expectation, do the addresses get filled out during the task or something like that, or do we need that field to be required? (Sorry, I know nothing..)

himdel · 2016-11-10T13:35:25Z

Another thing, trying to save a new floating IP, I get an error that just says "Floating IP creation failed: Task start failed: ID [10000000088263]".

That's ..very unhelpful :) (What I'm seeing in the log is..., so we do have an actual error message we could show..)

[----] E, [2016-11-10T13:33:41.336507 #14977:2b0527c090d4] ERROR -- : excon.error     #<Excon::Error::Unauthorized: Expected([200, 204]) <=> Actual(401 Unauthorized)
excon.error.response
  :body          => "{\"error\": {\"message\": \"The request you have made requires authentication.\", \"code\": 401, \"title\": \"Unauthorized\"}}"
  :cookies       => [
  ]

gildub · 2016-11-17T03:09:20Z

@himdel,

A floating IP address from the pool would be automatically assigned unless provided.

I fixed the task error.

tzumainn · 2016-11-21T22:13:03Z

app/controllers/floating_ip_controller.rb

+    options[:ems_id] = params[:ems_id] if params[:ems_id] && params[:ems_id] != 'new'
+    options[:floating_ip_address] = params[:floating_ip_address] if params[:floating_ip_address]
+    options[:cloud_network_id] = params[:cloud_network_id] if params[:cloud_network_id]
+    options[:cloud_tenant_id] = params[:cloud_tenant_id] if params[:cloud_tenant_id]


This needs to be something like:

options[:cloud_tenant] = find_by_id_filtered(CloudTenant, params[:cloud_tenant_id]) if params[:cloud_tenant_id]

Because the model expects options to include :cloud_tenant, and not :cloud_tenant_id (I'm currently fixing this in other places)

martinpovolny · 2016-11-23T12:09:29Z

app/controllers/floating_ip_controller.rb

@@ -14,4 +14,260 @@ def self.display_methods
  end

  menu_section :net
+
+  def button


Can you cleanup this method a bit?

Please, make it a simple case statement and make sure that all the code is needed. E.g. you are setting the @refresh_div variable and then testing it. Is there some code between the statements that might change that value?

@martinpovolny,

All the network UI and others (cloud_tenant, cloud_volumes) use the exact same approach.
So I'm keen to make a refactor on all of them using a consistent approach.

That said, I don't see the point of a case for replacing a if/elsif.
No idea about the @refresh_div, I haven't digged into it, what's for? What's the issue here?
Thanks

himdel · 2016-11-28T15:54:34Z

So.. another problem, similar to #12101 (comment)

Go to /cloud_tenant/show/10000000000011?display=floating_ips (assuming such a tenant exists) - Compute > Clouds > Tenants - click a tenant detail, click on floating ips in the summary detail.
OR
Go to /network_port/show/10000000000389?display=floating_ips (same) - Networks > Network Ports - click on a port detail, click on floating ips in the summary detail.
(you can find these places by searching for floating_ips in the display_methods methods in each controller)
pick any ip (checkbox)
do any of the new toolbar actions
No route matches {:action=>"layouts/flash_msg", :controller=>"network_port", :id=>nil} [network_port/button] (or similar)

gildub · 2016-11-30T00:48:39Z

@himdel,

I can see that issue.
I believe this is respectively a cloud tenant and a network_port controller issues because other controllers accessing the same page are responsible for it, isn't ?
That said I'm looking into it.

gildub · 2016-11-30T01:07:02Z

@himdel,

Ok, so from the UI endpoint, why when selecting the floating IP (whether from the cloud tenant or network port entry) we don't end up in the floating IP controller?

A "normal" work flow would be:
After the user selects the floating IP, having saved where the user is coming from to be able to go back if needed later, then the floating ip controller manages any changes, ultimately the user chooses to go back or go somewhere else.

But obviously we are on a different road, where a controller (such as cloud tenant or network port, from the example above) is managing the views it's not designed for. Unless there is some logic in those button I'm missing. If that the case could you please direct me to the related documentation or at least some good practice example.

Thanks

gildub · 2016-11-30T01:12:50Z

@dclarizio, any pointers please?

himdel · 2016-11-30T15:47:35Z

@gildub exactly, the solution is essentially to handle the button action in both places, redirecting to the right controller from the other one. Or sharing the code.

For example, for the edit action, just redirecting to the right URL is usually enough (except you may need something like javascript_redirect previous_breadcrumb_url when the form is saved and done (or cancelled)) so that you get back to the right place again.

As @martinpovolny said, we're currently in the process of refactoring this so .. it has to work .. but doesn't have to be perfect right now :).

gildub · 2016-12-01T05:40:13Z

@himdel,

It seems like this is an on-going discussion and the shared controller approach might not be the one to take.

@martinpovolny also suggested to minimize the impact by not having some button appear on the cross controller. Do you have details about how to do that?

himdel · 2016-12-01T15:15:09Z

also suggested to minimize the impact by not having some button appear on the cross controller. Do you have details about how to do that?

Can't make them not appear in one controller but yes in another. But what you can do is not add the button to the list toolbar but only to the detail toolbar. So they would not be present in either of the lists, but only in the detail screen.

And that is easy because those are separate toolbars (one is in plural, the other not) .. so you just not add it to the other one :).

shared controller approach might not be the one to take.

Just to be sure, I was not proposing a shared controller, I was proposing exactly the opposite - redirecting those actions from the bad controller to the right one. (Similar to https://github.com/ManageIQ/manageiq/pull/12551/files#diff-68512128862e16616d93240055cb3a7dR77)

gildub · 2016-12-19T03:53:00Z

@himdel,

The name of a floating ip is the floating address.
The edit (and delete) buttons were not showing up any more because the filter was wrong, I've fixed it.

himdel · 2016-12-19T12:49:11Z

@gildub thanks!, going to re-review right now :)

The name of a floating ip is the floating address.

Yeaah, possibly only in the openstack land..

But I'm not blocking the PR for that, I guess that will change once you can edit other providers' floating ips too..

himdel · 2016-12-19T13:03:49Z

So, @gildub I think this one is ready to get merged 👍

But.. I'm seeing you marked a few Fixnum->Integers as fixed but haven't actually fixed them .. so I'm guessing you may have forgotten to push a commit here?

miq-bot · 2016-12-19T13:21:59Z

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

gildub · 2016-12-20T05:50:44Z

@himdel,

Sorry, yes the Fixnum -> Integer commit slipped out, I've restored it and rebased.

himdel · 2016-12-20T12:08:28Z

Sorry, yes the Fixnum -> Integer commit slipped out, I've restored it and rebased.

@gildub yeah, happens, thanks :) .. Just one more thing, can you please look at https://github.com/ManageIQ/manageiq/pull/12097/files#r93028604 (line 146 looks like it should be used instead of line 147).

But if not for some reason, at least remove the commented out code please :).

gildub · 2016-12-21T00:46:40Z

@himdel,

But if not for some reason, at least remove the commented out code please :).

sorry I forgot about that one, it's fixed, using find_by_id_filtered and without any comments ;)

Adds controller actions using wait_for_task Adds new and edit views with javascript Depends on #11819

miq-bot · 2016-12-21T07:22:47Z

Checked commits https://github.com/gildub/manageiq/compare/41341833bf86da410f33ee171aa0fbabbfa73ebf~...26cac2f757d3ca96aacacc3a220815c531c176e9 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
8 files checked, 14 offenses detected

app/controllers/floating_ip_controller.rb

himdel · 2016-12-21T13:00:08Z

@gildub Thanks, LGTM, the spec failures are probably all unrelated, I'll try to restart the tests once master is OK, and get this in before the UI split..

…ps-ui Floating IPs provisioning UI (cherry picked from commit 57b0a61) https://bugzilla.redhat.com/show_bug.cgi?id=1410588

simaishi · 2017-01-13T19:23:05Z

Euwe backport details:

$ git log -1
commit fc719649568c5b0fadc90479c88f00625decb886
Author: Martin Hradil <himdel@seznam.cz>
Date:   Wed Dec 21 14:56:01 2016 +0000

    Merge pull request #12097 from gildub/network-provisioning-floating-ips-ui
    
    Floating IPs provisioning UI
    (cherry picked from commit 57b0a61fa1d9529582056123b68c47d7707a26ca)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1410588

miq-bot added the euwe/yes label Oct 21, 2016

chessbyte added enhancement ui labels Oct 22, 2016

chessbyte assigned martinpovolny Oct 22, 2016

gildub closed this Nov 2, 2016

gildub reopened this Nov 2, 2016

gildub closed this Nov 7, 2016

gildub reopened this Nov 7, 2016

himdel mentioned this pull request Nov 7, 2016

Security Groups provisioning UI #12101

Closed

himdel reviewed Nov 10, 2016

View reviewed changes

tzumainn reviewed Nov 21, 2016

View reviewed changes

martinpovolny reviewed Nov 23, 2016

View reviewed changes

miq-bot added the unmergeable label Dec 19, 2016

miq-bot removed the unmergeable label Dec 20, 2016

gildub mentioned this pull request Dec 20, 2016

Network Router: More generic use of SupportsFeatureMixin #13073

Closed

gildub added 9 commits December 21, 2016 17:49

Floating IPs provisioning UI

4134183

Adds controller actions using wait_for_task Adds new and edit views with javascript Depends on #11819

SupportsFeatureMixin: in base class and better support

02aa9f9

Use to_s instead of inspect

3e41d9c

use CheckedIdMixin; fix glitch

aacb08f

feature uses generic update/delete

4bf53df

fix checked_item_id

b70a9c1

Task queue id use Integer

f016ab7

using generic supportfeatures method

4098ae8

form fields: using proper fetch filter

26cac2f

himdel closed this Dec 21, 2016

himdel reopened this Dec 21, 2016

himdel merged commit 57b0a61 into ManageIQ:master Dec 21, 2016

himdel added this to the Sprint 51 Ending Jan 2, 2017 milestone Dec 21, 2016

simaishi pushed a commit that referenced this pull request Jan 13, 2017

Merge pull request #12097 from gildub/network-provisioning-floating-i…

fc71964

…ps-ui Floating IPs provisioning UI (cherry picked from commit 57b0a61) https://bugzilla.redhat.com/show_bug.cgi?id=1410588

simaishi added euwe/backported and removed euwe/yes labels Jan 13, 2017

Fryguy added the providers/cloud label Jan 24, 2020

Floating IPs provisioning UI #12097

Floating IPs provisioning UI #12097

Conversation

gildub commented Oct 21, 2016 • edited Loading

tzumainn commented Oct 21, 2016

miq-bot commented Nov 1, 2016

gildub commented Nov 7, 2016

himdel commented Nov 7, 2016 • edited Loading

tzumainn commented Nov 7, 2016

tzumainn commented Nov 7, 2016

tzumainn commented Nov 7, 2016

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

himdel commented Nov 10, 2016

himdel commented Nov 10, 2016 • edited Loading

gildub commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Nov 28, 2016 • edited Loading

gildub commented Nov 30, 2016

gildub commented Nov 30, 2016

gildub commented Nov 30, 2016

himdel commented Nov 30, 2016

gildub commented Dec 1, 2016

himdel commented Dec 1, 2016 • edited Loading

gildub commented Dec 19, 2016

himdel commented Dec 19, 2016

himdel commented Dec 19, 2016

miq-bot commented Dec 19, 2016

gildub commented Dec 20, 2016

himdel commented Dec 20, 2016

gildub commented Dec 21, 2016

miq-bot commented Dec 21, 2016

himdel commented Dec 21, 2016 • edited Loading

simaishi commented Jan 13, 2017

gildub commented Oct 21, 2016 •

edited

Loading

himdel commented Nov 7, 2016 •

edited

Loading

himdel commented Nov 10, 2016 •

edited

Loading

himdel commented Nov 28, 2016 •

edited

Loading

himdel commented Dec 1, 2016 •

edited

Loading

himdel commented Dec 21, 2016 •

edited

Loading