Support operation `create` on CloudObjectStoreContainer #651

miha-plesko · 2017-03-10T15:08:27Z

This commit adds a new page /cloud_object_store_container/new where user can create new CloudObjectStoreContainer. Page is fully functional and extendable. Tested on Amazon S3 provider.

Button that redirects to /cloud_object_store_container/new appears in center toolbar:

And the /cloud_object_store_container/new page itself:

Depends on:

@miq-bot add_label enhancement
@miq-bot assign @h-kataria

miq-bot · 2017-03-20T17:28:22Z

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

miha-plesko · 2017-03-21T15:06:18Z

@h-kataria I would kindly ask for a review. This PR is very similar to the PR that @gberginc and yourself discussed in detail (and then merged) a week ago or so: #517. At the moment only Amazon provider supports this functionality (supports? :cloud_object_store_container_create).

/cc @gberginc

gberginc · 2017-03-21T15:38:42Z

@miha-plesko I was working on those PRs with @AparnaKarve.

miha-plesko · 2017-03-21T18:43:05Z

Huh, when you try to do something quick at the end of working day when your girlfriend's waiting at the door...

@h-kataria, @AparnaKarve, ladies, my deepest apologies for the confusion. I'm all trembling now how rigorous your review might be 😄

AparnaKarve · 2017-03-21T18:50:21Z

ladies, my deepest apologies for the confusion. I'm all trembling now how rigorous your review might be 😄

@miha-plesko Relax, take a deep breath :)
I'll get to the review sometime today.

gberginc

Few comments in the code. Appologies for typos (working on phone).

gberginc · 2017-03-21T21:38:38Z

app/views/cloud_object_store_container/new.html.haml

+                         "prefix"                      => "default",
+                         "reset-validation-status"     => "default_auth_status")
+
+  %table{:width => '100%'}


No other button here? Perhaps you could reuse the layout partial layouts/angular/x_edit_buttons_angular, proposed in #676.

That is correct...you'll need Cancel button as well.
Please use the layouts/angular/x_edit_buttons_angular partial

nice one 👍

gberginc · 2017-03-21T21:40:18Z

app/helpers/application_helper/button/cloud_object_store_container_new.rb

+
+  def disabled?
+    # TODO: disable button if no active providers support create action
+    false


Shouldn't you have a consition that would ensure you have at least one object storage manager supporting creation of buckets?

gberginc · 2017-03-21T21:46:54Z

app/controllers/cloud_object_store_container_controller.rb

@@ -32,8 +32,98 @@ def self.display_methods
    %w(cloud_object_store_objects)
  end

+  def new
+    assert_privileges("cloud_object_store_container_new")


Out of curiosity, I wonder if the cloud prefix is intentional in the name cloud_object_store_container? To me, object storage is something that is not related to the "cloud" (contrary to, for example cloud volumes and cloud networks).

Models app/models/cloud_object_store_container.rb and app/models/cloud_object_store_object existed in the miq core before and I never thought to change it. I understand what you're saying, but there is reasoning behing cloud prefix as well: the storage is in cloud, not on local machine. I must admin, however, that the name really is rather long :D

gberginc · 2017-03-21T21:54:39Z

app/controllers/cloud_object_store_container_controller.rb

+    # Depending on the storage manager type, collect required form params.
+    case params[:emstype]
+    when "ManageIQ::Providers::Amazon::StorageManager::S3"
+      options[:bucket] = params[:name] if params[:name]


I understand that S3 API requires :bucket in options, but I wonder if you could resolve this in the backend? You are setting the :name above and the provider could perhaps check if only name is present and set the :bucket on its own.

Also, I understand that you need :name to show proper flash messages; this is why I am asking to use name instead of bucket.

Thanks for rising this question, let me argument my thinking. I had to decide where to perform mapping:

A. pass entire HTML form inputs to the backend (1 mapping)

frontend: no mapping backend: HTML form fields -> options

B. slightly modified HTML form inputs to the backend (2 mappings)

frontend: HTML form fields -> slightly modified options backend: slightly modified options-> options

C. pass fully-mapped options to backend (1 mapping)

frontend: HTML form fields -> options backend: no mapping

My thinking here was that A should be avoided since backend shouldn't care much about how the HTML form inputs are named. Then B is not very reasonable since we only want to map once. So C is the way to go and is also employed in cloud_volume_controller.rb where we give each provider what it actually needs.

To sum up: I would prefer to perform all the mapping on frontend. This being said, please let me know of your opinion.

Understood; the only concern was that name and bucket are duplicated. Name is used for flash messages, bucket for s3 provider. That's why I thought it may be worth considering that the name is checked in the backend if bucket is not provided.: so, if bucket is in options in the backend, use it, otherwise check if there is name and set it to bucket.

Updating backend with ManageIQ/manageiq-providers-amazon#198

gberginc · 2017-03-21T22:00:26Z

app/controllers/cloud_object_store_container_controller.rb

+    managers = ManageIQ::Providers::CloudManager.supported_subclasses.select(&:supports_regions?)
+    managers.each_with_object({}) do |manager, provider_regions|
+      regions = manager.parent::Regions.all.sort_by { |r| r[:description] }
+      provider_regions[manager.name] = regions.map do |region|


Make in single line?

Done, thanks.

gberginc · 2017-03-21T22:04:00Z

app/views/cloud_object_store_container/new.html.haml

+                      "ng-model"                    => "cloudContainerModel.storage_manager_id",
+                      "ng-change"                   => "storageManagerChanged(cloudContainerModel.storage_manager_id)",
+                      "required"                    => "",
+                      :miqrequired                  => true,


I thibk :miqrequired is no longer used.

Correct. Please remove miqrequired.

gberginc · 2017-03-21T22:10:24Z

app/views/cloud_object_store_container/new.html.haml

@@ -0,0 +1,62 @@
+%form#form_div{:name => "angularForm", 'ng-controller' => "cloudObjectStoreContainerFormController"}


Just checking: did you notice any flicker when you open this page? First, browser default controls are displayed for a short period, then proper controls used by miq. If you did, check ng-cloak.

Wasn't aware on ng-cloak, thanks!

gberginc · 2017-03-21T22:16:08Z

app/views/cloud_object_store_container/new.html.haml

+                         "checkchange"                 => "",
+                         "selectpicker-for-select-tag" => "",
+                         "prefix"                      => "default",
+                         "reset-validation-status"     => "default_auth_status")


What is prefix and reset-validation-status? Are they intentional?

This is probably a copy-paste from the ems form.
reset-validation-status is specific to the Validation portion of the ems form and not applicable here. Please remove it.

Remove prefix as well.

Whooops, busted, this is a copy-paste from ems form. Removed prefix and reset-validation-status.

AparnaKarve · 2017-03-21T22:19:04Z

app/views/cloud_object_store_container/new.html.haml

+        = _('Storage Manager')
+      .col-md-8
+        = select_tag("storage_manager_id",
+          options_for_select([["<#{_('Choose')}>", nil]] + @storage_manager_choices.sort, disabled: ["<#{_('Choose')}>", nil]),


In an effort to make our angular forms that add new functionality (like this one) make use of REST API, could we populate this dropdown using a REST API call as well instead of using the instance variable @storage_manager_choices?

Can you try out this API and let me know if it works well for this dropdown?

/api/providers?expand=resources&filter[]=type='*StorageManager::S3*'

We need to list those storage managers that support :cloud_object_store_container_create feature. Is it possible to filter them by supported feature using API? Unfortunately supports? function is not a column in database nor virtual column so I'm not sure how to filter by this. Is it possible to filter based on utility functions at all?

If you need to filter them by the supported feature, then I guess we'd have to create a PR in manageiq core to address this.

@miha-plesko Can you try this API and LMK if it gets you the right storage managers :

/api/providers?expand=resources&filter[]=supports_block_storage?=true

I need to get a list of those that support cloud_object_store_container_create so I've inserted this in the link above. And viola, yes, the list is correct! 👍

/api/providers?expand=resources&filter[]=supports_cloud_object_store_container_create?=true result: { "name":"providers", "count":4, "subcount":1, "resources":[ { "href":"http://172.16.117.115:3000/api/providers/4", "id":4, "name":"AWS S3 Storage Manager", "created_on":"2017-03-20T14:30:59Z", "updated_on":"2017-03-30T14:16:49Z", "zone_id":1, "type":"ManageIQ::Providers::Amazon::StorageManager::S3", "provider_region":"us-west-2", "last_refresh_date":"2017-03-30T14:16:49Z", "tenant_id":1, "parent_ems_id":1, "actions":[ { "name":"edit", "method":"post", "href":"http://172.16.117.115:3000/api/providers/4" }, { "name":"refresh", "method":"post", "href":"http://172.16.117.115:3000/api/providers/4" }, { "name":"delete", "method":"post", "href":"http://172.16.117.115:3000/api/providers/4" }, { "name":"delete", "method":"delete", "href":"http://172.16.117.115:3000/api/providers/4" } ] } ], "actions":[ { "name":"query", "method":"post", "href":"http://172.16.117.115:3000/api/providers" }, { "name":"create", "method":"post", "href":"http://172.16.117.115:3000/api/providers" }, { "name":"edit", "method":"post", "href":"http://172.16.117.115:3000/api/providers" }, { "name":"refresh", "method":"post", "href":"http://172.16.117.115:3000/api/providers" }, { "name":"delete", "method":"post", "href":"http://172.16.117.115:3000/api/providers" } ] }

Thanks for trying the API @miha-plesko

@miha-plesko if you don't need all the attributes, I suggest you limit the list to those that are important, e.g. add &attributes=id,name to the request to get just the name and id:

{ "name": "providers", "count": 8, "subcount": 2, "resources": [ { "href": "http://localhost:3000/api/providers/17", "id": 17, "name": "OS XLAB Cinder Manager" }, { "href": "http://localhost:3000/api/providers/23", "id": 23, "name": "AWS XLAB EBS Storage Manager" } ], "actions": [ { "name": "query", "method": "post", "href": "http://localhost:3000/api/providers" }, { "name": "create", "method": "post", "href": "http://localhost:3000/api/providers" }, { "name": "edit", "method": "post", "href": "http://localhost:3000/api/providers" }, { "name": "refresh", "method": "post", "href": "http://localhost:3000/api/providers" }, { "name": "delete", "method": "post", "href": "http://localhost:3000/api/providers" } ] }

AparnaKarve · 2017-03-21T22:37:41Z

app/views/cloud_object_store_container/new.html.haml

+          = _('Region')
+        .col-md-8
+          - @provider_regions.each do |name, regions|
+            = select_tag('provider_region',


Same goes here... an API would be preferable.
Will get back to you on the exact API that you can use later.

Here I'd need API to be able to access class constant. Currently, regions are obtained from here: https://github.com/ManageIQ/manageiq-providers-amazon/blob/master/app/models/manageiq/providers/amazon/regions.rb#L8-L84

Is there a way to fetch them thru API?

Let's leave this as is.

Would have to dig deeper into this to find out what is involved to get the Amazon Regions via the API.

AparnaKarve · 2017-03-21T23:00:34Z

app/controllers/application_controller/ci_processing.rb

@@ -1868,6 +1868,7 @@ def process_cloud_object_storage_buttons(pressed)
    task = pressed.sub("#{klass.name.underscore.to_sym}_", "")

    return tag(klass) if task == "tag"
+    return javascript_redirect(:controller => klass.name.underscore.to_sym, :action => "new") if task == "new"



This does not seem right.
This logic needs to be in the cloud_object_store_container controller, not here.

Can you look up a few examples and fix this?

I did some research and now I'm confused: should "Add CloudObjectStoreContainer" option appear only next to the list of all containers and be hidden next to the list of containers per Manager?

In other words, should this be hidden:

?

I hope not; I think that creating a new bucket from the list of provider's buckets should be supported.

I think the comment is more about the location of the change. We discussed why in ci_processing some time ago, but I don't remember exactly.

Should the button be visible both from /cloud_object_store_container/show_list and /ems_storage/ID?display=cloud_object_store_containers, it would probably make more sense to have this logic in ci_processing (which is included by both controllers).

@AparnaKarve would you prefer me to copy line 1871 to above this line and to above this line?

@miha-plesko All button handling for cloud_object_store_container should be in that controller itself, not in ci_processing
Check this out for instance --
https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/cloud_network_controller.rb#L38-#L41

@miha-plesko I see that you have already added delete code in ci_processing and now you are about to add new.
The delete handling should have been added in cloud_object_store_container_controller.rb

The button handling for new, edit, delete should all live in the acting controller code.
Let me give you another code example that was recently added --
https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/ansible_credential_controller.rb#L27-L35
It's clear, concise and easy to follow. Could we implement something like that?

If it needs to be accessible from ems_storage as well, then you also add the button handling in ems_common.rb. Something like this --
https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/ems_common.rb#L455-L456

Sounds reasonable. Let me try to do it this way. What confuses me still is this line in ems_storage controller: https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/ems_common.rb#L285. It seems like delete for cloud_volume is implemented in ci_processing while other operations are elsewhere. Let me try to refactor this, we shall see if any problem arises.

AparnaKarve · 2017-03-21T23:15:21Z

Does that seem like a valid error?
If yes, why?

gberginc · 2017-03-22T03:36:58Z

This error is legit, because bucket names must be globaly unique (across all users and all regions). S3 console would warn with a simple message that the bucket already exists.

miha-plesko

@AparnaKarve @gberginc thanks for initial pass. I've answered to your comments, please take a look.

miha-plesko · 2017-03-24T07:36:46Z

app/controllers/cloud_object_store_container_controller.rb

@@ -32,8 +32,98 @@ def self.display_methods
    %w(cloud_object_store_objects)
  end

+  def new
+    assert_privileges("cloud_object_store_container_new")


Models app/models/cloud_object_store_container.rb and app/models/cloud_object_store_object existed in the miq core before and I never thought to change it. I understand what you're saying, but there is reasoning behing cloud prefix as well: the storage is in cloud, not on local machine. I must admin, however, that the name really is rather long :D

miha-plesko · 2017-03-24T07:40:59Z

app/controllers/cloud_object_store_container_controller.rb

+    # Depending on the storage manager type, collect required form params.
+    case params[:emstype]
+    when "ManageIQ::Providers::Amazon::StorageManager::S3"
+      options[:bucket] = params[:name] if params[:name]


Thanks for rising this question, let me argument my thinking. I had to decide where to perform mapping:

A. pass entire HTML form inputs to the backend (1 mapping)

frontend: no mapping backend: HTML form fields -> options

B. slightly modified HTML form inputs to the backend (2 mappings)

frontend: HTML form fields -> slightly modified options backend: slightly modified options-> options

C. pass fully-mapped options to backend (1 mapping)

frontend: HTML form fields -> options backend: no mapping

My thinking here was that A should be avoided since backend shouldn't care much about how the HTML form inputs are named. Then B is not very reasonable since we only want to map once. So C is the way to go and is also employed in cloud_volume_controller.rb where we give each provider what it actually needs.

To sum up: I would prefer to perform all the mapping on frontend. This being said, please let me know of your opinion.

miha-plesko · 2017-03-24T08:53:22Z

app/helpers/application_helper/button/cloud_object_store_container_new.rb

+
+  def disabled?
+    # TODO: disable button if no active providers support create action
+    false


miha-plesko · 2017-03-24T08:55:28Z

app/views/cloud_object_store_container/new.html.haml

@@ -0,0 +1,62 @@
+%form#form_div{:name => "angularForm", 'ng-controller' => "cloudObjectStoreContainerFormController"}


Wasn't aware on ng-cloak, thanks!

miha-plesko · 2017-03-24T09:57:53Z

app/views/cloud_object_store_container/new.html.haml

+        = _('Storage Manager')
+      .col-md-8
+        = select_tag("storage_manager_id",
+          options_for_select([["<#{_('Choose')}>", nil]] + @storage_manager_choices.sort, disabled: ["<#{_('Choose')}>", nil]),


We need to list those storage managers that support :cloud_object_store_container_create feature. Is it possible to filter them by supported feature using API? Unfortunately supports? function is not a column in database nor virtual column so I'm not sure how to filter by this. Is it possible to filter based on utility functions at all?

miha-plesko · 2017-03-24T10:08:41Z

app/views/cloud_object_store_container/new.html.haml

+          = _('Region')
+        .col-md-8
+          - @provider_regions.each do |name, regions|
+            = select_tag('provider_region',


Here I'd need API to be able to access class constant. Currently, regions are obtained from here: https://github.com/ManageIQ/manageiq-providers-amazon/blob/master/app/models/manageiq/providers/amazon/regions.rb#L8-L84

Is there a way to fetch them thru API?

miha-plesko · 2017-03-24T10:13:58Z

app/views/cloud_object_store_container/new.html.haml

+                         "checkchange"                 => "",
+                         "selectpicker-for-select-tag" => "",
+                         "prefix"                      => "default",
+                         "reset-validation-status"     => "default_auth_status")


Whooops, busted, this is a copy-paste from ems form. Removed prefix and reset-validation-status.

miha-plesko · 2017-03-24T10:44:49Z

app/views/cloud_object_store_container/new.html.haml

+                         "prefix"                      => "default",
+                         "reset-validation-status"     => "default_auth_status")
+
+  %table{:width => '100%'}


nice one 👍

miha-plesko · 2017-03-24T11:10:05Z

app/controllers/application_controller/ci_processing.rb

@@ -1868,6 +1868,7 @@ def process_cloud_object_storage_buttons(pressed)
    task = pressed.sub("#{klass.name.underscore.to_sym}_", "")

    return tag(klass) if task == "tag"
+    return javascript_redirect(:controller => klass.name.underscore.to_sym, :action => "new") if task == "new"



I did some research and now I'm confused: should "Add CloudObjectStoreContainer" option appear only next to the list of all containers and be hidden next to the list of containers per Manager?

In other words, should this be hidden:

?

miha-plesko · 2017-03-24T11:18:13Z

app/controllers/cloud_object_store_container_controller.rb

+    managers = ManageIQ::Providers::CloudManager.supported_subclasses.select(&:supports_regions?)
+    managers.each_with_object({}) do |manager, provider_regions|
+      regions = manager.parent::Regions.all.sort_by { |r| r[:description] }
+      provider_regions[manager.name] = regions.map do |region|


Done, thanks.

miha-plesko · 2017-04-11T13:43:59Z

@AparnaKarve I apologize that it took me so long to apply your comments. I've added two commits to this PR: one of them modernizes JavaScript code so that Angular now makes use of fancy this.vm paradigm. The other commit is an attempt to address issue regarding controller logic placement.

I was not able to fully satisfy your concerns regarding "moving all the logic out of ci_processing.rb" utility controller. Let me try to explain my problem here, but I'm also willing to perform a videocall to demonstrate you in person if needed.

So, there are two groups of operations that central button can trigger:

operations with own view
e.g. cloud_object_store_container_new with new.html.haml
operations without view
e.g. cloud_object_store_container_delete that has no view
e.g. cloud_object_store_container_clear that also has no view

For group (1) there is no doubt what to do when user triggers them: just redirect to the controller that manages corresponding view. But for group (2) this is not possible since there is nothing to redirect to! So the current controller (either ems_common.rb or cloud_object_store_container_controller.rb, depending on URL that we're at) must execute the operation on its own. And that is why I put the code for operations "delete container", "delete object", "clear container" in ci_processing.rb - code there is directly callable from either controller.

To put it simple: if ems_common.rb controller is to redirect to cloud_object_store_container_controller.rb, and cloud_object_store_container_controller.rb has no view for operation, what is supposed to be rendered to the user?

/cc @gberginc

AparnaKarve · 2017-04-12T23:02:24Z

@miha-plesko If I test this PR now in it's current state, would I be able to see the issue that you have described in scenario2 for operations: cloud_object_store_container_delete and cloud_object_store_container_clear without a view to render?

miha-plesko · 2017-04-13T06:33:40Z

@AparnaKarve no, the way it's implemented now everything works. The problem would arise only if I wanted to extract delete and clear out of ci_processing.rb and put it into cloud_object_store_container.rb. IMO this is not be part of this PR anyway ;)

miha-plesko

@AparnaKarve thank you for another iteration. I've pushed an additional commit where I apply your suggestions. I'd argue with you only regarding storage_manager_id topic since I think you've tested it on a wrong StorageManager, while the code is actually ok. See this comment.

I'm looking forward to any further suggestions that you might have.

miha-plesko · 2017-04-21T07:02:04Z

...pts/controllers/cloud_object_store_container/cloud_object_store_container_form_controller.js

+    };
+
+    // fetch StorageManager from querystring
+    if (! isNaN(parseInt(storageManagerId, 10))) {


Nice one, thank you.

miha-plesko · 2017-04-21T07:12:14Z

...pts/controllers/cloud_object_store_container/cloud_object_store_container_form_controller.js

+
+    // If storage manager ID was provided, we need to refresh the form and show
+    // corresponding form fields.
+    if (storageManagerId) {


miha-plesko · 2017-04-21T07:14:43Z

app/views/cloud_object_store_container/new.html.haml

+               "model-copy"    => "vm.modelCopy"}
+  = render :partial => "layouts/flash_msg"
+  %h3
+    = _('Basic Information')


miha-plesko · 2017-04-21T07:35:31Z

app/views/cloud_object_store_container/new.html.haml

+                            'ng-model'     => "vm.cloudContainerModel.name",
+                            'ng-maxlength' => 255,
+                            :miqrequired   => true,
+                            :checkchange   => true}


Done. I've also fixed the same issue for other inputs.

This commit adds a new page /cloud_object_store_container/new where user can create new CloudObjectStoreContainer. Page is fully functional and extendable. Tested on Amazon S3 provider. Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

Here we move the logic that redirects user to `cloud_object_store_container/new` from common place to each of the calling controllers. This way we are able to differentiate whether the button was cliked on a list of all Containers or on a list of Containers for specific StorageManager. Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

There were some _("Required") missing on the form. With this commit I add them where needed. This commit also moves logic for parsing manager id obtained thru querystring to the haml by using angular filter. Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

AparnaKarve · 2017-04-21T20:08:32Z

app/controllers/cloud_object_store_container_controller.rb

+    @breadcrumbs.pop if @breadcrumbs
+    session[:flash_msgs] = @flash_array.dup if @flash_array
+
+    javascript_redirect :action => "show_list"


Can you change this to :

javascript_redirect previous_breadcrumb_url

Useful while adding a New Cloud Object Store Container from the Storage Manager Summary page.
Once the Add is done, the page will redirected back to the Summary page.

Good point, thanks.

AparnaKarve · 2017-04-21T20:18:40Z

app/views/cloud_object_store_container/new.html.haml

+                         "ng-model"                    => "vm.cloudContainerModel.provider_region",
+                         "ng-switch-when"              => name,
+                         "required"                    => "",
+                         "checkchange"                 => "",


Adding a search capability in this dropdown would be a good idea since it could be a fairly long list.

All you need to do is add these 2 parameters to the Regions select

:multiple => false, "data-live-search" => true,

Awesome, done.

AparnaKarve · 2017-04-21T20:26:11Z

@miha-plesko Just those couple minor things above...but otherwise, this is looking great.

Question about edit: Would you be creating a PR for editing the Cloud Object Store containers after this is merged?

miha-plesko · 2017-04-21T20:44:34Z

@AparnaKarve just answering your question immediately (I shall apply other changes on Monday):
At this point I'm fully focusing on :new. The idea is, of course, to equip this with with :edit in the future, but I'm not sure if I'm gonna be the one implementing it.

AparnaKarve · 2017-04-21T23:56:20Z

At this point I'm fully focusing on :new. The idea is, of course, to equip this with with :edit in the future,

Agree. Let's get new merged first, edit should be fairly simple after that.

but I'm not sure if I'm gonna be the one implementing it.

Ok, not a problem.

miha-plesko

@AparnaKarve thank you for the two very useful comments, I've added one commit to address each.

Talking about :edit operation, I've realized that at this point we allow user to input only information that is immutable (name and location). In other words, it is not possible* to rename any S3 bucket - to achieve such behavior you must create new bucket with new name, copy objects there, and delete the old one. So supporting an :edit operation would result in all field greyed-out at the moment anyway 😄

*by not possible I mean it cannot be done due to limitations on S3 side, not MiQ side

miha-plesko · 2017-04-24T08:47:21Z

app/controllers/cloud_object_store_container_controller.rb

+    @breadcrumbs.pop if @breadcrumbs
+    session[:flash_msgs] = @flash_array.dup if @flash_array
+
+    javascript_redirect :action => "show_list"


Good point, thanks.

miha-plesko · 2017-04-24T08:55:32Z

app/views/cloud_object_store_container/new.html.haml

+                         "ng-model"                    => "vm.cloudContainerModel.provider_region",
+                         "ng-switch-when"              => name,
+                         "required"                    => "",
+                         "checkchange"                 => "",


Awesome, done.

Currently we were redirecting to the list of all Cloud Containers when user created a new bucket. But that's not OK when user came from manager's page. This commit redirects user one step back in breadcrumb instead. Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

There are 15 bucket regions to select from so it makes sense to offer user a search field as part of the drop-down. Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

miq-bot · 2017-04-24T10:28:43Z

Checked commits miha-plesko/manageiq-ui-classic@5a7a3ad~...731488e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
7 files checked, 1 offense detected

app/views/cloud_object_store_container/new.html.haml

⚠️ - Line 50 - Use hash rockets syntax.

bronaghs · 2017-04-24T14:05:44Z

@miq-bot remove_label blocker

bronaghs · 2017-04-24T14:21:23Z

Removed the blocker label because S3 will be disabled by default in the Fine release (see: ManageIQ/manageiq-providers-amazon#213)

AparnaKarve · 2017-04-24T17:23:19Z

In other words, it is not possible* to rename any S3 bucket - to achieve such behavior you must create new bucket with new name, copy objects there, and delete the old one. So supporting an :edit operation would result in all field greyed-out at the moment anyway 😄

Oh, that is good to know. Thanks for that vital piece of information.
So bottomline, AWS itself does not support editing buckets and hence edit does not apply in this case at all. Awesome! :)

AparnaKarve · 2017-04-24T17:42:19Z

@miha-plesko Thank you for addressing all the feedback!
Successfully tested in the UI. This is working really great!

@dclarizio @h-kataria This is good to merge.
All the CC errors are regarding "Similar code found in 1 other location", something that cannot be dealt with immediately (or at least in this PR)

Support operation `create` on CloudObjectStoreContainer (cherry picked from commit ac62737)

simaishi · 2017-04-24T22:40:35Z

Fine backport details:

$ git log -1
commit 88795e89e9368f9661470fdf7ba5b696f9b5ac80
Author: Harpreet Kataria <hkataria@redhat.com>
Date:   Mon Apr 24 13:52:53 2017 -0400

    Merge pull request #651 from miha-plesko/add_aws_bucket
    
    Support operation `create` on CloudObjectStoreContainer
    (cherry picked from commit ac62737c8dfbb282d65ccda4c0daa316b6ed436b)

miq-bot added the enhancement label Mar 10, 2017

miq-bot assigned h-kataria Mar 10, 2017

miq-bot added the unmergeable label Mar 20, 2017

miha-plesko force-pushed the add_aws_bucket branch from d26acbc to 02d2a65 Compare March 21, 2017 14:05

miq-bot removed the unmergeable label Mar 21, 2017

miha-plesko force-pushed the add_aws_bucket branch 2 times, most recently from 2dcc51b to 1dc6288 Compare March 21, 2017 14:54

miha-plesko force-pushed the add_aws_bucket branch from 1dc6288 to 41f0aee Compare March 21, 2017 15:10

gberginc reviewed Mar 21, 2017

View reviewed changes

AparnaKarve reviewed Mar 21, 2017

View reviewed changes

miha-plesko force-pushed the add_aws_bucket branch from 41f0aee to d1da355 Compare March 24, 2017 11:18

miha-plesko commented Mar 24, 2017

View reviewed changes

miha-plesko mentioned this pull request Mar 24, 2017

Remap :name parameter into :bucket parameter when creating new bucket ManageIQ/manageiq-providers-amazon#198

Merged

miha-plesko force-pushed the add_aws_bucket branch 2 times, most recently from 47f8311 to 027ae2d Compare March 24, 2017 14:24

gberginc mentioned this pull request Mar 30, 2017

Use API to get block storage manager for volumes #861

Closed

miha-plesko force-pushed the add_aws_bucket branch from 027ae2d to 66dccb1 Compare April 11, 2017 13:06

miha-plesko closed this Apr 11, 2017

miha-plesko reopened this Apr 11, 2017

miha-plesko force-pushed the add_aws_bucket branch from 66dccb1 to 71efe12 Compare April 11, 2017 14:00

miha-plesko commented Apr 21, 2017

View reviewed changes

miha-plesko closed this Apr 21, 2017

miha-plesko reopened this Apr 21, 2017

miha-plesko added 4 commits April 21, 2017 10:43

Modernize javascript code: use API and this.vm

c30bd2f

Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

Fine-tune the html form

3be5955

There were some _("Required") missing on the form. With this commit I add them where needed. This commit also moves logic for parsing manager id obtained thru querystring to the haml by using angular filter. Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

miha-plesko force-pushed the add_aws_bucket branch from 6d6f85c to 3be5955 Compare April 21, 2017 08:44

AparnaKarve reviewed Apr 21, 2017

View reviewed changes

miha-plesko commented Apr 24, 2017

View reviewed changes

miha-plesko added 2 commits April 24, 2017 12:24

Make bucket region drop-down searchable

731488e

There are 15 bucket regions to select from so it makes sense to offer user a search field as part of the drop-down. Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

miha-plesko force-pushed the add_aws_bucket branch from 59309d2 to 731488e Compare April 24, 2017 10:26

miq-bot removed the blocker label Apr 24, 2017

h-kataria added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 24, 2017

h-kataria merged commit ac62737 into ManageIQ:master Apr 24, 2017

simaishi pushed a commit that referenced this pull request Apr 24, 2017

Merge pull request #651 from miha-plesko/add_aws_bucket

88795e8

Support operation `create` on CloudObjectStoreContainer (cherry picked from commit ac62737)

simaishi added fine/backported and removed fine/yes labels Apr 24, 2017

miha-plesko deleted the add_aws_bucket branch January 7, 2019 08:24

		@@ -0,0 +1,62 @@
		%form#form_div{:name => "angularForm", 'ng-controller' => "cloudObjectStoreContainerFormController"}

Support operation create on CloudObjectStoreContainer #651

Support operation create on CloudObjectStoreContainer #651

Conversation

miha-plesko commented Mar 10, 2017 • edited Loading

miq-bot commented Mar 20, 2017

miha-plesko commented Mar 21, 2017

gberginc commented Mar 21, 2017

miha-plesko commented Mar 21, 2017

AparnaKarve commented Mar 21, 2017

gberginc left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve Mar 29, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve commented Mar 21, 2017

gberginc commented Mar 22, 2017

miha-plesko left a 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

Choose a reason for hiding this comment

miha-plesko commented Apr 11, 2017

AparnaKarve commented Apr 12, 2017

miha-plesko commented Apr 13, 2017

miha-plesko left a 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

Support operation `create` on CloudObjectStoreContainer #651

Support operation `create` on CloudObjectStoreContainer #651

miha-plesko commented Mar 10, 2017 •

edited

Loading

AparnaKarve Mar 21, 2017 •

edited

Loading

AparnaKarve Mar 29, 2017 •

edited

Loading

AparnaKarve Mar 29, 2017 •

edited

Loading