Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support operation delete on CloudObjectStoreContainer #420

Merged
merged 1 commit into from
Mar 7, 2017
Merged

Support operation delete on CloudObjectStoreContainer #420

merged 1 commit into from
Mar 7, 2017

Conversation

miha-plesko
Copy link
Contributor

@miha-plesko miha-plesko commented Feb 17, 2017 •
edited
Loading

With this commit we add "Configuration" menu option to the list of CloudObjectStore containers where we support operation delete.

This commit brings a new mid-page menu item "Configuration->Remove Object Storage Container" to the following pages:

  • cloud object container details page
    capture2

  • cloud object container full list
    capture

  • cloud object container list per StorageManager
    capture_1

Video:
delete_bucket.zip

Links:

This was referenced Feb 17, 2017
Merged
Merged
@martinpovolny
Copy link
Member

I have not read the code yet, but I can see that there's a bunch of Rubocop issues and there are no tests for the newly added functions. So marking this as WIP for now.

@martinpovolny martinpovolny changed the title Support operations on CloudObjectStoreContainer [WIP] Support operations on CloudObjectStoreContainer Feb 18, 2017
@miha-plesko miha-plesko force-pushed the delete_aws_bucket branch 3 times, most recently from e9fb792 to 5661331 Compare February 21, 2017 11:01
@miha-plesko
Copy link
Contributor Author

Hello @martinpovolny I've added a dozen of unit tests and fixed rubocop issues. I'm not sure, however, whether it's better now that there is a mixed code styling with less rubocop issues or was it more readable before when code styling was consistent across the file (e.g. https://github.com/ManageIQ/manageiq-ui-classic/pull/420/files#diff-4b513b358c3d87df7b0be7e0eef4d95e).

Could you please take a look if there is anything else that can be improved? :D

Ladas
Ladas reviewed Feb 21, 2017
View reviewed changes
app/helpers/application_helper/toolbar/cloud_object_store_container_center.rb Outdated
N_('Remove Object Storage Container'),
:url_parms => "main_div",
:confirm => N_("Warning: The selected Object Storage Container and ALL related Objects will be "\
"permanently removed!"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are missing supports check https://github.com/Ladas/manageiq-ui-classic/blob/29e35d051703fc0b1e85ec88d14ca62d972dcad5/app/helpers/application_helper/toolbar/cloud_subnet_center.rb#L25-L25

similar supports check should be also in the controller @romanblanco ?

Copy link
Member

@romanblanco romanblanco Feb 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ladas The support check is in the Toolbar Button code. In Toolbar is only specified feature that you want to test.

@miha-plesko miha-plesko force-pushed the delete_aws_bucket branch 4 times, most recently from 9581e93 to 8f09667 Compare February 23, 2017 14:57
@miha-plesko
Copy link
Contributor Author

I've noticed an unused function in my commit so I've removed it and repushed.

@miha-plesko miha-plesko changed the title [WIP] Support operations on CloudObjectStoreContainer Support operations on CloudObjectStoreContainer Feb 24, 2017
@miq-bot miq-bot removed the wip label Feb 24, 2017
This was referenced Feb 27, 2017
Merged
Merged
@miha-plesko miha-plesko force-pushed the delete_aws_bucket branch 2 times, most recently from 2b5dc65 to 8abd781 Compare March 1, 2017 13:53
@martinpovolny
Copy link
Member

@miha-plesko : the failed tests seem to be relevant and related to these changes. Please, take a look,

@miha-plesko
Copy link
Contributor Author

@martinpovolny I think all failed tests are a result of related PR in manageiq repo not being merged yet. Knowing this, I would still be happy to be hearing your opinion on the approach in this PR. This way we will be able to merge this one just after the related one is merged and tests turn green 😄

@gberginc
Copy link
Contributor

gberginc commented Mar 2, 2017

@miha-plesko close & reopen in case the merged PR (ManageIQ/manageiq#13965) affects this.

@miha-plesko miha-plesko closed this Mar 3, 2017
@miha-plesko miha-plesko reopened this Mar 3, 2017
@miha-plesko
Copy link
Contributor Author

@martinpovolny any suggestion why Travis fails here
https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/207300784#L608
while it's all green locally (tested on my laptop and friend's laptop)?

The easiest workaround would be to delete the two tests that are failing, but I would feel better if we somehow make them green instead 😄

@miha-plesko miha-plesko changed the title Support operations on CloudObjectStoreContainer Support operation delete on CloudObjectStoreContainer Mar 3, 2017
@chargio
Copy link

chargio commented Mar 6, 2017

Hi @martinpovolny The code is ready to merge, could you please have a look at it?

Thanks,
Sergio

martinpovolny
martinpovolny reviewed Mar 7, 2017
View reviewed changes
app/controllers/application_controller/ci_processing.rb Outdated
@@ -1840,6 +1840,49 @@ def vm_button_operation(method, display_name, partial_after_single_selection = n
vms.count
end

def process_cloud_object_storage_buttons
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you, please, pass in params[:pressed] as an argument here?
(better for testing, code readability etc.)

@martinpovolny
Copy link
Member

This looks good! Please, just add the argument mentioned above.

The Rubocops about complexity we don't really enforce at this point, but I don't see the reason for the compaints in spec/controllers/cloud_object_store_container_spec.rb:
:exclamation: - Line 96, Col 7 - Rails/HttpPositionalArguments - Use keyword arguments instead of positional arguments for http call: post.

I see you have hash arguments there, which looks good to me. So we'll ignore that too.

@miha-plesko
Support operations on CloudObjectStoreContainer
b3b5cc2 
CloudObjectStoreContainer model had no support for operations
(e.g. delete) since they were not supported by backend. Well,
now the backend supports deleting bucket hence we add "Configuration"
menu option for this.

This commit brings a new mid-page menu item "Configuration->Remove
Object Storage Container" to the following pages:

* cloud object container details page
* cloud object container full list
* cloud object container list per StorageManager

Signed-off-by: Miha Pleško <[email protected]>
@miq-bot
Copy link
Member

miq-bot commented Mar 7, 2017

Some comments on commit miha-plesko@b3b5cc2

spec/controllers/application_controller/ci_processing_spec.rb

  • ⚠️ - 168 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 24 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 99 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

spec/controllers/cloud_object_store_container_spec.rb

  • ⚠️ - 5 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Mar 7, 2017

Checked commit miha-plesko@b3b5cc2 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
8 files checked, 8 offenses detected

app/controllers/application_controller/ci_processing.rb

spec/controllers/application_controller/ci_processing_spec.rb

spec/controllers/cloud_object_store_container_spec.rb

@miha-plesko miha-plesko closed this Mar 7, 2017
@miha-plesko miha-plesko reopened this Mar 7, 2017
@miha-plesko
Copy link
Contributor Author

Thank you for the review @martinpovolny, I totally agree with your suggestions. I've applied them and repushed, please take a look.

@martinpovolny
Copy link
Member

Thx!

@martinpovolny martinpovolny merged commit 199e81c into ManageIQ:master Mar 7, 2017
@martinpovolny martinpovolny added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 7, 2017
@miha-plesko miha-plesko deleted the delete_aws_bucket branch January 7, 2019 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ladas Ladas Ladas left review comments

@romanblanco romanblanco romanblanco left review comments

@martinpovolny martinpovolny martinpovolny left review comments

Assignees

@martinpovolny martinpovolny

Labels
compute/cloud enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@miha-plesko @martinpovolny @gberginc @chargio @miq-bot @romanblanco @Ladas @dclarizio