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

Default to resource name for conversion hosts #18516

Merged
merged 4 commits into from
Mar 7, 2019
Merged

Default to resource name for conversion hosts #18516

merged 4 commits into from
Mar 7, 2019

Conversation

djberg96
Copy link
Contributor

@djberg96 djberg96 commented Mar 4, 2019 •
edited by simaishi
Loading

Based on feedback from the V2V team, a conversion host should default to the associated resource name if no name is explicitly specified. This PR modifies the ConversionHost model so that it does just that on creation.

https://bugzilla.redhat.com/show_bug.cgi?id=1689894
https://bugzilla.redhat.com/show_bug.cgi?id=1695797

@djberg96
Copy link
Contributor Author

djberg96 commented Mar 4, 2019

@fdupont-redhat @mturley Please take a look.

@miq-bot
Copy link
Member

miq-bot commented Mar 4, 2019

Checked commits https://github.com/djberg96/manageiq/compare/cf1aa8c6df1f9de30956e391a52f065fa01d4355~...ec9b8d5ed388d49839e855cf8436e2d7c3b23190 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. 🍪

ghost
ghost reviewed Mar 4, 2019
View reviewed changes
spec/models/conversion_host_spec.rb
let(:ems) { FactoryBot.create(:ems_redhat, :zone => FactoryBot.create(:zone), :api_version => '4.2.4') }
let(:redhat_host) { FactoryBot.create(:host_redhat, :name => 'foo', :ext_management_system => ems) }

it "defaults to the associated resource name if no name is explicitly provided" do
Copy link

Choose a reason for hiding this comment

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

Don't you also test with an OpenStack VM as resource ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For purposes of a validation spec it's not really necessary. We mostly just want to make sure the name is set, regardless of the associated resource type.

Copy link

Choose a reason for hiding this comment

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

Ok. Then LGTM.

mturley
mturley approved these changes Mar 4, 2019
View reviewed changes
Copy link
Contributor

@mturley mturley left a comment

Choose a reason for hiding this comment

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

Works on my end 👍

@djberg96
Copy link
Contributor Author

djberg96 commented Mar 4, 2019

@miq-bot add_reviewer @agrare

@miq-bot miq-bot requested a review from agrare March 4, 2019 20:28
@djberg96
Copy link
Contributor Author

djberg96 commented Mar 5, 2019

@miq-bot add_label hammer/yes

@djberg96
Copy link
Contributor Author

djberg96 commented Mar 5, 2019

@miq-bot add_label transformation, enhancement

@agrare
Copy link
Member

agrare commented Mar 5, 2019

@djberg96 this looks like it'd be far simpler to handle on the ConversionHost.create!() side, just :name => name || resource.name instead of any before_validation hooks

@djberg96
Copy link
Contributor Author

djberg96 commented Mar 5, 2019

@agrare Sorry, not sure what you mean by "on the ConversionHost.create!()" side. You want me to redefine the create! method? Is that really simpler? I worry that I don't know what other repercussions that might have.

@agrare
Copy link
Member

agrare commented Mar 5, 2019

You want me to redefine the create! method

Definitely not

I mean name is just another parameter to create!, so whoever calls ConversionHost.create! can just pass in name as well.

@agrare agrare self-assigned this Mar 5, 2019
@djberg96
Copy link
Contributor Author

djberg96 commented Mar 5, 2019

I feel that approach makes the model less robust, and I would have to set it in the rest api, and both the enable and enable_queue methods, and so on. This way I don't have to worry about who is calling it or how.

@agrare
Copy link
Member

agrare commented Mar 5, 2019

and I would have to set it in the rest api, and both the enable and enable_queue methods, and so on

Why in all of these places? enable/enable_queue just take params so you can just do params[:name] ||= resource.name in the API when you call enable_queue

@djberg96
Copy link
Contributor Author

djberg96 commented Mar 5, 2019

Ok, but I still prefer this approach since it works no matter how a conversion host is created instead of tinkering with a single method.

@agrare agrare merged commit 297f4c2 into ManageIQ:master Mar 7, 2019
@ghost
Copy link

ghost commented Mar 18, 2019

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

@djberg96 djberg96 mentioned this pull request Mar 20, 2019
Closed
@djberg96 djberg96 mentioned this pull request Mar 29, 2019
Merged
@agrare agrare added this to the Sprint 107 Ending Mar 18, 2019 milestone Apr 4, 2019
@mturley mturley mentioned this pull request Apr 5, 2019
Merged
simaishi pushed a commit that referenced this pull request Apr 22, 2019
@agrare @simaishi
Merge pull request #18516 from djberg96/conversion_host_validations
6e595db 
Default to resource name for conversion hosts

(cherry picked from commit 297f4c2)

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

Hammer backport details:

$ git log -1
commit 6e595db3ff47f30cd8698920ae23dbeda746435f
Author: Adam Grare <agrare@redhat.com>
Date:   Thu Mar 7 16:30:19 2019 -0500

    Merge pull request #18516 from djberg96/conversion_host_validations
    
    Default to resource name for conversion hosts
    
    (cherry picked from commit 297f4c22abbc1dcf98b3b4e9bc2337bcf3acb9be)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1696437
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1693715

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agrare agrare agrare approved these changes

@mturley mturley mturley approved these changes

Assignees

@agrare agrare

Labels
enhancement hammer/backported v2v
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@djberg96 @miq-bot @agrare @simaishi @mturley