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 more ConversionHost validations #18264

Closed
wants to merge 7 commits into from
Closed

Add more ConversionHost validations #18264

wants to merge 7 commits into from

Conversation

djberg96
Copy link
Contributor

@djberg96 djberg96 commented Dec 4, 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"
  • resource_id - must be present
  • address - must be present, unique, and be a valid IP address

I'm not sure about the uniqueness constraint on the address field and/or if there are any other validations that we would like to see added, so a WIP for now.

@miq-bot miq-bot added the wip label Dec 4, 2018
@djberg96 djberg96 changed the title [WIP] Add more ConversionHost validations Add more ConversionHost validations Dec 7, 2018
@@ -38,7 +47,7 @@ def source_transport_method
def ipaddress(family = 'ipv4')
return address if address && IPAddr.new(address).send("#{family}?")
Copy link
Contributor

Choose a reason for hiding this comment

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

Now address is enforced to be present and in the format of IP and coming from the resource.ipaddresses, do we still need this ipaddress method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not, but let's refactor in a separate PR. :)

@miq-bot miq-bot removed the wip label Dec 7, 2018
@miq-bot
Copy link
Member

miq-bot commented Dec 7, 2018

Checked commits https://github.com/djberg96/manageiq/compare/58bc0e469ba91e6448e3db42d6e95938097cc9e4~...a519f75360034e13d064ac0ab5d6ff78b9ee143a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 7 offenses detected

app/models/conversion_host.rb

spec/models/service_template_transformation_plan_request_spec.rb

spec/models/service_template_transformation_plan_task_spec.rb

@djberg96
Copy link
Contributor Author

djberg96 commented Dec 7, 2018

@jameswnl @agrare I'm going to have to rework this, because it's having a ripple effect through the specs anywhere there's a ConversionHost factory and/or anything relying on the current ipaddresses method. It's going to have to be less strict.

@djberg96
Copy link
Contributor Author

@jameswnl @fdupont-redhat Closed in favor of #18277

@djberg96 djberg96 closed this Dec 10, 2018
@djberg96 djberg96 deleted the conversion_host_validations branch March 4, 2019 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants