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

Only data storage domains in VM transform dialog #229

Merged
merged 1 commit into from
Jan 10, 2018
Merged

Only data storage domains in VM transform dialog #229

merged 1 commit into from
Jan 10, 2018

Conversation

smelamud
Copy link
Contributor

@smelamud smelamud commented Dec 14, 2017 •
edited

In VM transform dialog allow to select only data storage domains as the
destination of VM transformation.

Depends on ManageIQ/manageiq#16719

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1516706

@smelamud
Copy link
Contributor Author

@miq-bot assign tinaafitz

@smelamud
Copy link
Contributor Author

@miq-bot add_labels gaprindashvili/yes

@smelamud
Copy link
Contributor Author

@tinaafitz @gmcculloug Can you please review this PR?

@smelamud
Copy link
Contributor Author

@miq-bot assign gmcculloug

@miq-bot miq-bot assigned gmcculloug and unassigned tinaafitz Dec 19, 2017
agrare
agrare reviewed Dec 19, 2017
View reviewed changes
content/automate/ManageIQ/Infrastructure/VM/Transform/Import.class/__methods__/list_storages.rb Outdated
@@ -21,6 +21,7 @@ def main
values_hash[nil] = 'None'
else
provider.storages.each do |storage|
Copy link
Member

Choose a reason for hiding this comment

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

We have a #writable_storages which checks the read_only bit on the host/storage mount. Do non-data storage domains set read-only? Could be a nicer way to skip thse.

Copy link
Member

Choose a reason for hiding this comment

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

Note that #writable_storages is only available from the Host model today. That method is already exposed to automate.

Copy link
Member

Choose a reason for hiding this comment

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

Ah you're right for some reason I thought we had that exposed at the provider level, good catch

tinaafitz
tinaafitz suggested changes Dec 19, 2017
View reviewed changes
Copy link
Member

@tinaafitz tinaafitz left a comment

Choose a reason for hiding this comment

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

@smelamud Looks good.

Could you update the spec test:
https://github.com/ManageIQ/manageiq-content/blob/master/spec/content/automate/ManageIQ/Infrastructure/VM/Transform/Import.class/__methods__/list_storages_spec.rb

@smelamud
Copy link
Contributor Author

@tinaafitz Done.

@miq-bot
Copy link
Member

miq-bot commented Dec 20, 2017

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

@smelamud
Copy link
Contributor Author

Rebased.

gmcculloug
gmcculloug reviewed Dec 20, 2017
View reviewed changes
content/automate/ManageIQ/Infrastructure/VM/Transform/Import.class/__methods__/list_storages.rb Outdated
@@ -23,6 +23,7 @@ def main
values_hash[nil] = 'None'
else
provider.storages.each do |storage|
next unless storage.storage_domain_type == "data"
values_hash[storage.id] = storage.name
Copy link
Member

Choose a reason for hiding this comment

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

Instead of next unless this could be a simple if-statement.

values_hash[storage.id] = storage.name if storage.storage_domain_type == "data"

@smelamud
Copy link
Contributor Author

@gmcculloug Done.

@smelamud
Copy link
Contributor Author

@gmcculloug Is it good, may be merged?

@tinaafitz
Copy link
Member

@smelamud I ran the existing list_storages spec test and noticed there weren't any storages included:
the result: storages: {nil=>"-- select storage from list --"}
Can you modify the spec test so storages has values and add a test to show that the storage counts differ when checking for storage.storage_domain_type == "data"?

@smelamud
Copy link
Contributor Author

@tinaafitz I see, but still cannot understand how to fix this. Do you have an idea?

The provider is created with FactoryGirl.create(:ems_redhat, :with_storages), this call creates also storages, but provider.storages is still an empty list. And I cannot add values to it, because this association is declared as:

has_many :storages, -> { distinct }, :through => :hosts

When I try to add elements to it, error is thrown.

@smelamud smelamud force-pushed the list-storages-data-only branch 2 times, most recently from 4110ae2 to 8af6559 Compare December 24, 2017 17:40
@smelamud
Copy link
Contributor Author

@tinaafitz Done.

@smelamud
Copy link
Contributor Author

smelamud commented Jan 4, 2018

@tinaafitz ManageIQ/manageiq#16719 which is a prerequisite of this patch has been merged. Please check if it is possible to merge this PR also.

@bdunne bdunne closed this Jan 4, 2018
@bdunne bdunne reopened this Jan 4, 2018
@smelamud smelamud closed this Jan 7, 2018
@smelamud smelamud reopened this Jan 7, 2018
@gmcculloug gmcculloug closed this Jan 9, 2018
@gmcculloug gmcculloug reopened this Jan 9, 2018
@bdunne bdunne closed this Jan 9, 2018
@bdunne bdunne reopened this Jan 9, 2018
@gmcculloug
Copy link
Member

@smelamud Tests are running now. Please rebase so you pick up PR #231 which should resolve the test failures. Thanks.

@smelamud
Copy link
Contributor Author

@gmcculloug Done.

@smelamud
Only data storage domains in VM transform dialog
e03ea85 
In VM transform dialog allow to select only data storage domains as the
destination of VM transformation.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1516706
@smelamud
Copy link
Contributor Author

Fixed failing test.

@miq-bot
Copy link
Member

miq-bot commented Jan 10, 2018

Checked commit smelamud@e03ea85 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@gmcculloug
Copy link
Member

@tinaafitz Can you review/approve this PR?

tinaafitz
tinaafitz approved these changes Jan 10, 2018
View reviewed changes
Copy link
Member

@tinaafitz tinaafitz left a comment

Choose a reason for hiding this comment

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

@gmcculloug Looks good.

@gmcculloug gmcculloug merged commit 2ece9d3 into ManageIQ:master Jan 10, 2018
@gmcculloug gmcculloug added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 10, 2018
@simaishi simaishi mentioned this pull request Jan 10, 2018
Merged
simaishi pushed a commit that referenced this pull request Jan 11, 2018
@gmcculloug @simaishi
Merge pull request #229 from smelamud/list-storages-data-only
d7abe46 
Only data storage domains in VM transform dialog
(cherry picked from commit 2ece9d3)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1533550
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit d7abe46898eb0992613ccc6a75bb542d75c16ae8
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Wed Jan 10 14:00:55 2018 -0500

    Merge pull request #229 from smelamud/list-storages-data-only
    
    Only data storage domains in VM transform dialog
    (cherry picked from commit 2ece9d36033f3770ac9c112690fc18a185b22693)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1533550

@gmcculloug gmcculloug added the bug label Jan 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmcculloug gmcculloug gmcculloug left review comments

@agrare agrare agrare left review comments

@tinaafitz tinaafitz tinaafitz approved these changes

Assignees

@gmcculloug gmcculloug

Labels
bug gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@smelamud @miq-bot @tinaafitz @gmcculloug @simaishi @agrare @bdunne