-
Notifications
You must be signed in to change notification settings - Fork 107
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.
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
Support deletion of CloudObjectStoreObject #152
Conversation
@miq-bot add_label enhancement, wip |
@@ -0,0 +1,8 @@ | |||
module ManageIQ::Providers::Amazon::CloudManager::CloudObjectStoreContainer::Operations | |||
extend ActiveSupport::Concern | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miha-plesko
I think this should be:
supports :delete do
unsupported_reason_add(:delete, "CloudObjectStoreContainer has no #{ui_lookup(:table => "ext_management_systems")}, unable to delete it" unless ext_management_system
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like what you suggest, thanks!
extend ActiveSupport::Concern | ||
|
||
def raw_delete | ||
raise "CloudObjectStoreContainer has no #{ui_lookup(:table => "ext_management_systems")}, unable to delete it" unless ext_management_system |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miha-plesko - then this raise
can go away.
def raw_delete | ||
raise "CloudObjectStoreContainer has no #{ui_lookup(:table => "ext_management_systems")}, unable to delete it" unless ext_management_system | ||
with_provider_object(&:delete!) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miha-plesko - is there a status property that needs to be updated to reflect a deletion is in progress until the next refresh happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Unfortunately ATM there is no status property available. We should definitely support this in the future.
def raw_delete | ||
raise "CloudObjectStoreObject has no #{ui_lookup(:table => "ext_management_systems")}, unable to delete it" unless ext_management_system | ||
raise "CloudObjectStoreObject has no #{ui_lookup(:table => "cloud_object_store_container")}, unable to delete it" unless cloud_object_store_container | ||
with_provider_object(&:delete) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miha-plesko - same comment applies to these raise
exceptions
ff025dc
to
99f5cdf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bronaghs Thank you very much for the review. Your comments helped me a lot. I've updated the code and repushed all the PRs that I've pinged you for and I think that code is notably better now 😄
I'd kindly ask you to perform second pass.
@@ -0,0 +1,8 @@ | |||
module ManageIQ::Providers::Amazon::CloudManager::CloudObjectStoreContainer::Operations | |||
extend ActiveSupport::Concern | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like what you suggest, thanks!
def raw_delete | ||
raise "CloudObjectStoreContainer has no #{ui_lookup(:table => "ext_management_systems")}, unable to delete it" unless ext_management_system | ||
with_provider_object(&:delete!) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Unfortunately ATM there is no status property available. We should definitely support this in the future.
This pull request is not mergeable. Please rebase and repush. |
Support deletion of CloudObjectStoreObject for Amazon provider. Signed-off-by: Miha Pleško <[email protected]>
99f5cdf
to
a7676a4
Compare
Checked commit miha-plesko@a7676a4 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🏆
@blomquisg can you merge this and the related core PR?
Support deletion of CloudObjectStoreObject for Amazon provider.
Depends on (from other repos):
Will enable merge of:
delete
on CloudObjectStoreObject manageiq-ui-classic#497