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

Add ConversionHost validations #18277

Merged
merged 7 commits into from
Dec 11, 2018
Merged

Add ConversionHost validations #18277

merged 7 commits into from
Dec 11, 2018

Conversation

djberg96
Copy link
Contributor

@djberg96 djberg96 commented Dec 10, 2018

As I was doing some general testing with various POST operations with the REST API for conversion hosts, I realized there were almost no validations on the model. This PR adds some validations to a few columns, specifically the following:

name - must be present
resource_type - must be present and "Vm" or "Host" or "VmOrTemplate"
resource_id - must be present
address - must be unique, and be a valid IP address, if present

Replaces #18264

The "VmOrTemplate" was required for the service template request/plan models and specs, as was the check for the presence of an IP address before validating.

Ignore that first part, we only need VmOrTemplate and not Vm, since it's a subclass.

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

@djberg96
Copy link
Contributor Author

@jameswnl @fdupont-redhat Please take a look. This is as strict as I could make it without breaking service templates.

@agrare
Copy link
Member

agrare commented Dec 10, 2018

@djberg96 didn't we add :supports to the models that allowed conversion hosts already? Couldn't we use those in the API so it was consistent with the UI? This doesn't cover the "host must be rhev and VM must be openstack" case which the supports feature ones do.

@agrare
Copy link
Member

agrare commented Dec 10, 2018

@djberg96 I think we're just stubbing way too many things, the vm/host ipaddresses should be hardware.networks and the active tasks should just use the association. I think some of these used to be method calls but now that they're real associations we should just set this up for real instead of stubbing.

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

(Just a drive by)

Saw the p and thought I'd comment

app/models/conversion_host.rb Show resolved Hide resolved
@djberg96 djberg96 changed the title Conversion host validations2 Add ConvesionHost validations Dec 10, 2018
@djberg96
Copy link
Contributor Author

@agrare I've only added one stub, and then only because of some odd behavior that @gmcculloug had to narrow down, so I feel like adding "real" network data would just be testing that Rails associations work as advertised. I actually started doing that, and it was just more work than it was worth IMHO.

@djberg96
Copy link
Contributor Author

@agrare True, we could put some of the logic in the supports block instead of on the model itself. Part of the reason for these validations was actually inspired by the eligible? method, too. So the idea was to do something like:

supports do
  unless valid? && eligible?
    unsupported_reason_add(:conversion_host, "Conversion host is not valid")
  end
end

That, or update the eligible? method to check for valid? as well.

@djberg96 djberg96 changed the title Add ConvesionHost validations Add ConversionHost validations Dec 10, 2018
@agrare agrare self-assigned this Dec 11, 2018
@miq-bot
Copy link
Member

miq-bot commented Dec 11, 2018

Checked commits https://github.com/djberg96/manageiq/compare/bd2ade60f867d5e2e2e73c7d5ee6d5c16521fd9c~...99d065703aa6daf95f35af7850947fe30f3938af with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

app/models/conversion_host.rb

@agrare agrare merged commit c617468 into ManageIQ:master Dec 11, 2018
@agrare agrare added this to the Sprint 101 Ending Dec 17, 2018 milestone Dec 11, 2018
NickLaMuro added a commit to NickLaMuro/manageiq-api that referenced this pull request Dec 12, 2018
A recent change in ManageIQ/manageiq#18277
caused specs to fail in the API since the new validations no longer
allow the specs to create `:conversion_host` factories.

This change simply updates the specs to support these new restrictions,
but it might not be the ideal API interface.
@djberg96
Copy link
Contributor Author

@miq-bot add_label hammer/yes

simaishi pushed a commit that referenced this pull request Mar 29, 2019
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit 373368e38ce4f7c40b6e88e676c1473b18515838
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Dec 11 14:15:21 2018 -0500

    Merge pull request #18277 from djberg96/conversion_host_validations2
    
    Add ConversionHost validations
    
    (cherry picked from commit c6174684c548dc78120ab21ff2725d8f00a70bec)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1694229

@simaishi
Copy link
Contributor

simaishi commented Apr 1, 2019

Reverted the backport:

commit c43f47a0e701a048879deeb48e72a84124ab299e
Author: Satoe Imaishi <simaishi@redhat.com>
Date:   Mon Apr 1 11:30:56 2019 -0400

    Revert "Merge pull request #18277 from djberg96/conversion_host_validations2"
    
    This reverts commit 373368e38ce4f7c40b6e88e676c1473b18515838.

@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit 47d0ef9ab752a565f4fb7b67741a0dce909aa834
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Dec 11 14:15:21 2018 -0500

    Merge pull request #18277 from djberg96/conversion_host_validations2
    
    Add ConversionHost validations
    
    (cherry picked from commit c6174684c548dc78120ab21ff2725d8f00a70bec)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1696437

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.

5 participants