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

Providers discovery without unspecified discovery types (FIX) #17229

Conversation

slemrmartin
Copy link
Contributor

Host discovery raises exception without discovery types options

Cc @agrare

Links

Steps for Testing/QA [Optional]

In Rails console, put
Host.discoverHost(Marshal.dump({:ipaddr => '10.8.196.29'}))

@@ -30,7 +30,7 @@ def self.scan_host(ost)

if ping
# Trigger probes
ost.discover_types.each do |type|
ost.discover_types&.each do |type|
Copy link
Member

Choose a reason for hiding this comment

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

If you don't pass discover_types then this method just pings the target, but doesn't do anything and doesn't raise an Exception, and so looks like it passed. I think maybe we should raise ArgumentError, "must pass discovery_types" or something instead to ensure it's passed. Unless, just pinging is the thing we want it to do? @agrare ?

Copy link
Member

Choose a reason for hiding this comment

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

Unless, just pinging is the thing we want it to do?

That's not the intention, the UI won't let you submit the form without specifying at least one hypervisor discovery type.

👍 for raising an exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ook, replaced by exception

@slemrmartin slemrmartin force-pushed the providers-discovery-without-specified-types branch 2 times, most recently from 748c037 to 2d72b9b Compare April 3, 2018 07:44
@@ -29,6 +29,7 @@ def self.scan_host(ost)
end

if ping
raise ArgumentError, "must pass discover_types" if ost.discover_types.nil?
Copy link
Member

Choose a reason for hiding this comment

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

Probably want to raise on discover_types.empty? instead of just nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ou, right, it doesn't raise Exception if empty, but doesn't improve code as intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer blank?, empty? will fail on nil

Copy link
Member

Choose a reason for hiding this comment

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

👍

Host discovery doesn't raise exception without discovery types options
@slemrmartin slemrmartin force-pushed the providers-discovery-without-specified-types branch from 2d72b9b to d1f8361 Compare April 3, 2018 14:20
@agrare agrare self-assigned this Apr 3, 2018
@miq-bot
Copy link
Member

miq-bot commented Apr 3, 2018

Checked commit slemrmartin@d1f8361 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@agrare agrare merged commit f4b2f93 into ManageIQ:master Apr 3, 2018
@agrare agrare added this to the Sprint 83 Ending Apr 9, 2018 milestone Apr 3, 2018
@slemrmartin slemrmartin deleted the providers-discovery-without-specified-types branch April 3, 2018 14:50
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.

4 participants