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.

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

Storage removal #993

Merged
merged 1 commit into from
Apr 24, 2017
Merged

Storage removal #993

merged 1 commit into from
Apr 24, 2017

Conversation

pkliczewski
Copy link
Contributor

Datastores can't be removed if there are relations to hosts or vms.
We need provide a way to check whether datastore removal is supported
(no vm or host relations). When there condition is not met we need
to notify a user that relations needs to be removed first.

This PR fixes:
https://bugzilla.redhat.com/1439380

@pkliczewski
Copy link
Contributor Author

This PR depends on ManageIQ/manageiq#14724

@pkliczewski
Copy link
Contributor Author

Travis needs ManageIQ/manageiq#14724 first and the test will pass.

@pkliczewski
Copy link
Contributor Author

@miq-bot assign @martinpovolny

@pkliczewski
Copy link
Contributor Author

@borod108 @masayag please take a look

Datastores can't be removed if there are relations to hosts or vms.
We need provide a way to check whether datastore removal is supported
(no vm or host relations). When there condition is not met we need
to notify a user that relations needs to be removed first.

This PR fixes:
https://bugzilla.redhat.com/1439380
@miq-bot
Copy link
Member

miq-bot commented Apr 12, 2017

Checked commit pkliczewski@2cfe2cf with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍰

@martinpovolny
Copy link
Member

martinpovolny commented Apr 19, 2017

Can this to be coded using the supports mechanism on the backend? (See e.g. app/models/vm_or_template/operations/snapshot.rb in the core repo).

And then I would expect the support to be checked in the code that displayes the button and the error message used from the support feature.

Then here we could check again (and again reuse the error message).

Does it make sense?

@pkliczewski
Copy link
Contributor Author

It is dependent on backend and it is using supports. Please see ManageIQ/manageiq#14724

@martinpovolny
Copy link
Member

martinpovolny commented Apr 20, 2017

Right, so then we should reuse the "unsuported reason" to form the flash message and it's done.

Something like:

add_flash(_(SecurityGroup.unsupported_reason(:create)), :error)

but I don't know how that's done with batch_operation_supported?

@pkliczewski
Copy link
Contributor Author

OK, will fix

@durandom
Copy link
Member

why is this fine/yes?
https://bugzilla.redhat.com/1439380 is not?!

@martinpovolny
Copy link
Member

why is this fine/yes?
https://bugzilla.redhat.com/1439380 is not?!

@dclarizio : shall this go to fine?

@blomquisg
Copy link
Member

@martinpovolny @durandom I've gotten clarification from PM that this can go to Fine. However, there's some discussion on the related Backend PR as to the general solution. Need to make sure we're all on the same page before moving forward.

@martinpovolny martinpovolny merged commit befd4da into ManageIQ:master Apr 24, 2017
@martinpovolny martinpovolny added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 24, 2017
@martinpovolny
Copy link
Member

@pkliczewski : I merged this as the backend is merged. Please, deal with the error message in a follow up PR (and ping me). Thx!

@pkliczewski
Copy link
Contributor Author

@martinpovolny Sure, I will.

@durandom
Copy link
Member

@miq-bot add_label fine/yes
because BZ got updated

@durandom
Copy link
Member

@miq-bot remove_label fine/no

@miq-bot miq-bot added fine/yes and removed fine/no labels Apr 24, 2017
simaishi pushed a commit that referenced this pull request Apr 24, 2017
@simaishi
Copy link
Contributor

Fine backport details:

 $ git log -1
commit 11807c93192667e8745655155aa46450434bbdaa
Author: Martin Povolny <[email protected]>
Date:   Mon Apr 24 07:15:33 2017 +0200

    Merge pull request #993 from pkliczewski/master
    
    Storage removal
    (cherry picked from commit befd4da221979024c715812dd3ad5c5a75ff4425)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1444889

@pkliczewski
Copy link
Contributor Author

@martinpovolny I checked and it is not that easy to get unsupported_reason for a group of storage domains. We can have 1 or more domains. We would need to decide which one to display. It seems like this approach (having generic message) fits good in such use case.

@martinpovolny
Copy link
Member

@pkliczewski : ok, thx

@durandom
Copy link
Member

@pkliczewski could you add this shortcoming to ManageIQ/manageiq#14740 ? I'll collect there what is needed as improvemnts in the supports feature mixin.

@pkliczewski
Copy link
Contributor Author

@durandom Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants