(FACT-2620) Add EC2 facts for linux #544
Conversation
lib/resolvers/ec2.rb
Outdated
| http = Net::HTTP.new(parsed_url.host) | ||
|
|
||
| session_timeout = ENV['EC2_SESSION_TIMEOUT'] | ||
| http.read_timeout = session_timeout ? session_timeout.to_i : EC2_SESSION_TIMEOUT | ||
| http.open_timeout = EC2_CONNECTION_TIMEOUT |
There was a problem hiding this comment.
this can be moved in a method called config_http
lib/resolvers/ec2.rb
Outdated
| if !name.end_with?('/') | ||
| container[name] = get_data_from("#{url}#{name}").strip | ||
| else | ||
| child = {} | ||
| child[name] = query_for_metadata("#{url}#{name}", child) | ||
| child.reject! { |key, _info| key == name } | ||
| name = name.gsub(%r{/$}, '') | ||
| container[name] = child |
There was a problem hiding this comment.
this can be moved in a method called name_parsing or something better. Even name should be changed into something more suggestive, something like metadata_name
There was a problem hiding this comment.
I would not move this code into another method as this method is a recursive one and i think is more easy to understand if the recursive call is made from inside it.
lib/resolvers/ec2.rb
Outdated
| http = Net::HTTP.new(parsed_url.host) | ||
|
|
||
| session_timeout = ENV['EC2_SESSION_TIMEOUT'] | ||
| http.read_timeout = session_timeout ? session_timeout.to_i : EC2_SESSION_TIMEOUT |
There was a problem hiding this comment.
This condition can be moved where the constant is declared.EC2_SESSION_TIMEOUT = ENV['k'] || 5
Or, create a method that returns the timeout and will also handle the to_i part.
There was a problem hiding this comment.
I would go even further and extract all the logic for timeout in a different method e.g.
EC2_SESSION_TIMEOUT = ENV['EC2_SESSION_TIMEOUT'].to_i || 5
...
def configure_timout(http)
http.read_timeout = EC2_SESSION_TIMEOUT
http.open_timeout = EC2_CONNECTION_TIMEOUT
end
oanatmaria
force-pushed
the
FACT-2620
branch
3 times, most recently
from
27ea257 to
b9a90c7
Compare
oanatmaria
force-pushed
the
FACT-2620
branch
2 times, most recently
from
97f6eef to
ec374b4
Compare
|
I suggest using https://github.com/bblimke/webmock to stub external APIs and disable external calls instead of mocking NET::HTTP. |
oanatmaria
force-pushed
the
FACT-2620
branch
2 times, most recently
from
c8f231a to
cf03a9b
Compare
lib/facts/linux/ec2_userdata.rb
Outdated
| Facter::Resolvers::Xen.resolve(:vm) | ||
| end | ||
|
|
||
| def check_other_facts |
There was a problem hiding this comment.
Could this method be split into two? I don't see any dependency between the two resolver calls.
lib/facts/linux/ec2_userdata.rb
Outdated
| virtual == 'kvm' || virtual =~ /xen/ | ||
| end | ||
|
|
||
| def retrieve_from_virt_what |
There was a problem hiding this comment.
Can you rename this to check_... to be consistent with the other methods?
lib/facts/linux/ec2_userdata.rb
Outdated
| module Linux | ||
| class Ec2Userdata | ||
| FACT_NAME = 'ec2_userdata' | ||
| HYPERVISORS_HASH = { 'VMware' => 'vmware', 'VirtualBox' => 'virtualbox', 'Parallels' => 'parallels', |
There was a problem hiding this comment.
Can this hash be moved to a different, common location? I saw that this is already defined in two other resolvers.
No description provided.