-
Notifications
You must be signed in to change notification settings - Fork 909
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
Dont resolve name if IP given #78
Comments
And you're specifically providing in your source (sql or 'router.db') 'ip' field, instead of 'name' field, or you're providing both? And what you want is node.rb to also accept that source can provide 'ip'? If I understood correctly, I think it's useful. And I'm wondering why #length > 0? Wouldn't just 'if opt[:ip]' work? Or is there case when it's defined but it's empty? @ip = opt[:ip] So if opt[:ip] is defined, we use it, if it's not defined, we try to resolve? Should we try to validate it being actually IP adddress? Something like this maybe? [ytti@ytti.fi ~]% pry -r ipaddr |
I should have included my config :) I am providing both, like this: source: In the database the "name" column is our friendly name of the device. I think checking the length is good as if somebody inserts into the database a hostname with an empty "ipv4" column then it will attempt to resolve the host. Of course, it should be null in the database, but that depends on the db schema. Ruby is something I'm new to, so please be gentle :) |
I think it makes sense and is a good idea. But maybe we should be more strict about getting real IP address.
If possible with bogus data in SQL too and monitor syslog. |
That looks to work good to me. Tests I have done:
A elegant solution compared to mine :) |
We have alot of switches backed up using oxidized and they all have internal names, so no DNS for these.
I've modified "lib/oxidized/node.rb" with the following, and it seems to work well.
if opt[:ip].length > 0
@ip = opt[:ip]
else
@ip = Resolv.getaddress @name
end
Would it be possible to get this changed officially if people think this is a good idea?
The text was updated successfully, but these errors were encountered: