Security Groups provisioning UI #12101
Conversation
|
@miq-bot add_label euwe/yes |
|
<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush. |
|
@martinpovolny could you please re-review? |
|
@himdel, @dclarizio, can you please do a review? |
| else | ||
| render_flash | ||
| end | ||
| end |
There was a problem hiding this comment.
we really need to make the "button" and "x_button" actions simple like here: https://github.com/ManageIQ/guides/blob/master/ui/button_actions.md
or at least make it a simple case so that it can be easily refactored later.
Someone is going to have a hell of a job refactoring the remaining button methods so let's not make it worse with new code.
Just make a simple case based on params[:pressed]. And remove especially the part and below.
elsif !flash_errors? && @refresh_div == "main_div" && @lastaction == "show_list"
i
Btw similar code:
app/controllers/cloud_network_controller.rb:24
app/controllers/cloud_subnet_controller.rb:16
app/controllers/network_router_controller.rb:16
There was a problem hiding this comment.
I understand the idea, meanwhile that's not fair, this is not introducing anything new code at all and certainly not making it worse either, just using plain old approach like 99% of the current existing def button code. I'm happy to help with the refactoring but into another PR.
There was a problem hiding this comment.
It would be nice if everyone picked the cleaner examples for new stuff but I understand your point. Sure we can do cleanup in a follow up PR, actually I created several PRs that try to deal with this in several controllers. I just want to make sure that we don't create refactoring TODOs faster than we resolve them.
|
Same here, can you please provide info where to find this, and a screenshot? :) |
|
@gildub I think this PR may be missing the _center.rb updates that update the toolbars... ? |
|
|
||
| security_groups_to_delete = [] | ||
| security_groups.each do |s| | ||
| security_group = SecurityGroup.find_by_id(s) |
|
@tzumainn, you're right the toolbar/*_center got lost during the PR backend/UI split! Thanks |
|
@himdel, I updated PR commit indicating the menu path for the feature. |
| options[:name] = params[:name] if params[:name] | ||
| options[:description] = params[:description] if params[:description] | ||
| options[:ems_id] = params[:ems_id] if params[:ems_id] | ||
| options[:cloud_tenant_id] = params[:cloud_tenant_id] if params[:cloud_tenant_id] |
There was a problem hiding this comment.
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)
|
@himdel, @martinpovolny, could you please review? Thanks |
|
So.. is not a great error message, but definitely better than nothing :). However, I'm also getting a Furthermore, this allows editing non-openstack security groups, which is nice, except it fails while loading the data for the edit screen with a EDIT: I've also updated the description to include the toolbar name and a screenshot, please try to do so in the future ;) |
|
@himdel , I'll run new set of tests ASAP. |
|
I've re-tested, Security Group create/delete/update works fine. Regarding the error message you got earlier (Unauthorized):
Thanks |
Sorry but it has to be. They don't need to have the full functionality, but they can't just crash. Right now, the toolbar button can only check for feature support in the detail screen so you can't probably use that .. but make the code at least detect the situation and redirect you back to the list screen with an error flash about the operation not being supported or something.
No problem :) and you just drag a image to the textarea ..and that's it :). (And it is expected for every UI change PR.)
Uh.. that .. sounds bad. Outside my area, so I'll suppose that's how it's supposed to be then, but it's very inconsistent with the rest of the UI. (Though maybe the success message could mention that the update will be visible only after the next refresh?)
Agreed completely, parsing out those errors needs to be done systematically, I'm also seeing some code in #12551 that could use better exception details :). |
Well, I have two openstack providers, with one, it always succeeds and never appears on the list - there I'll assume everything works and you just need the refresh. With the other one, creating any security group always causes that message, but indeed, trying to revalidate my login fails - so I guess it correctly fails, and that just that message can use more love in a separate PR :). (This one is infra whereas the other one is cloud, but .. my bet is more on the authentication thing there..) |
|
@himdel Out of curiosity, do you have examples of functionality where update - or create - update the MIQ object outside of refresh? The core team told that coding things that way was very bad. |
I'm not arguing with that, I don't really know about the backend.. But on most screens, adding an object actually causes such object to be added, editing an object's name causes such object to be renamed, etc.. |
|
Just to summarize.. the only blocker I'm seeing here is the fact that the code is not prepared to handle editing security groups it can't actually edit and needs to handle such failure gracefully.. |
|
Aah.. found another problem..
(If you're looking for how, @ZitaNemeckova is fixing these for all the existing code in #12551.) |
|
According to my tests, I cannot reproduce any of the aforementioned issues:
Regarding filtering out non Openstack Providers, wouldn't the pattern be for a button class We effectively need to have better wrapping of raw errors, I'm tempted to create an issue on this matter, meanwhile I believe some discussion might be needed by the providers team. |
|
@gildub @himdel I think the button in lists is using the supports only at a class level, it cannot check each element in the list for supporting of the feature now. (It would require a check based on selection of the item) We should have a similar check as assert_privileges, for support, for each action. So e.g. assert_supported, @durandom do we have such method for UI use yet? |
|
@gildub..
Agreed, that part is on my end, please disregard :).
Separate comment..
Exactly as @Ladas said, the toolbars currently support such feature only for the detail screen, not for the list screen. (It is in the works though.. :))
Agreed, definitely warrants an issue :). |
Retested... from
OK, doesn't change in the UI after saving, but seems that's the expected behaviour.
Seems like the culprit is Exactly the same goes for Amazon security groups, and indeed for one openstack group as well. |
Now from
Works fine, no issues with missing tenant there, no non-openstack groups .. no problem.
|
|
Ok, I see what you mean, I was using the link to the item not the buttons. As I commented in #12097 (comment) Maybe there is some button trick I'm missing, could you please provide pointers (@dclarizio) ? |
|
I removed the buttons from the show list. I was hoping there was some flag available to do it in order to avoid loosing the feature. But that's okay until we have the cross controller solution fully finished. I've also replaced the button statements with a case as now that I can see a better road path with the buttons. |
| render :json => { | ||
| :name => security_group.name, | ||
| :description => security_group.description, | ||
| :cloud_tenant_name => security_group.cloud_tenant.name, |
There was a problem hiding this comment.
this is still a problem, isn't it? (#12101 (comment))
There was a problem hiding this comment.
Guessing it should be security_group.cloud_tenant.try(:name)
There was a problem hiding this comment.
That seems to be the last bit preventing this from being merged.
| render :json => { | ||
| :name => security_group.name, | ||
| :description => security_group.description, | ||
| :cloud_tenant_name => security_group.cloud_tenant.try(name), |
There was a problem hiding this comment.
It does and I even read the reference manual! :(
Thanks
There was a problem hiding this comment.
Name without the colon is a variable or a method call. You really need to pass a symbol to try. Please fix this.
|
@gildub thanks, one more thing .. editing an existing security group and clicking Save.. |
|
Editing a security group work now for Openstack. |
|
Associated BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1394278 |
|
@gildub Please add any new changes as commits, I don't want (and will stop at some point :)) to have to read the whole thing again and again.. That said, it's still broken for Azure.. I do see a in the evm.log (after pushing the Save button), but in the UI, the spinner keeps spinning and spinning.
Not sure what you mean, I'm still seeing all the security groups in the list. |
Adds controller actions with task queueing Validation using supports feature mixin. Adds new and edit views with javascript Access through left navigation: Networks -> Security Groups
|
Checked commit https://github.com/gildub/manageiq/commit/978f9f3b525bd96e4aaacb4b1181aadb2c2a7b78 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 app/controllers/security_group_controller.rb
|
|
I'm not adding any new features, but if so then I use new commits. Regarding the issue about broken provider (such as Azure), there is nothing much I can do any more. On top of that I can't test any other providers but Openstack. The important part resides with the support feature mixin [1] which checks if the create/update/delete feature are respectively supported. So if [1] isn't working, either something is missing (or I'm missing it) or MIQ has a structural issue. In any case could the UI team provide guidance? @dclarizio, @Ladas what do you think? (@tzumainn) [1] https://github.com/ManageIQ/manageiq/pull/12101/files#diff-fb5d22d19133a44dd2514b60ccf6a16cR75 |
|
@himdel @dclarizio some added context - @mansam tried to implement this sort of "disablement" for other providers for a different case in #12597. In the discussion there you'll see that no good solution was reached. So I think what we need here is some official guidance from the UI team as to the appropriate way to block providers which don't support feature X from accessing X? |
|
@gildu there's an example of disabling a button based on feature that @sseago pointed out, and it's pretty straightforward: https://github.com/ManageIQ/manageiq/pull/12101/files Can you add that to this and the floating IP PRs - in a separate commit, if that's the preference? Then this may be ready to go! |
|
@gildub ^ whoops, typo |
|
@tzumainn, sorry but I can't see the pointer, where is this? Thanks |
|
@gildub I think this is what @tzumainn was referring to: https://github.com/ManageIQ/manageiq/blob/master/app/helpers/application_helper/toolbar/host_aggregate_center.rb#L15-L16 |
|
I closed it by mistake. Then before I realized that I forced-pushed the corresponding branch which makes it not able to be re-opened as I can't go back to previous SHA commit, moral of story this one gets replaced by a new PR: #13165. Sorry about the inconvenience. |
Adds controller actions with task queueing
Validation using supports feature mixin.
Adds new and edit views with javascript
https://bugzilla.redhat.com/show_bug.cgi?id=1394278
Access through left navigation: Networks -> Security Groups - toolbar Configuration
