Fix discovery of version 4 of oVirt #69
Conversation
| # Do nothing, just try the next candidate. | ||
| end | ||
| end | ||
| nil |
There was a problem hiding this comment.
It might be cleaner to use .detect
def engine_ssh_public_key
CANDIDATE_SSH_PUBLIC_KEY_PATHS.detect do |path|
begin
key = RestClient::Resource.new("#{base_uri}#{path}", resource_options).get
key.present?
rescue RestClient::ResourceNotFound, NoMethodError
false # Do nothing, just try the next candidate.
end
end
endThere was a problem hiding this comment.
Not 100% sure how detect works, but my understanding is that in this case it would return the first path that passes the test, and what we want to do here is to return the key retrieved from that path, not the path itself.
There was a problem hiding this comment.
Ah fair enough...I misread and mixed up key and path. Carry on :)
There was a problem hiding this comment.
Do you need the key? or is just the presence of it important?
There was a problem hiding this comment.
Sorry for the message spam, but what I mean is, if you don't actually need the key itself, then you can keep the detect above and maybe rename the method to engine_ssh_public_key?, then the ovirt? method can be
def self.ovirt?(options)
options[:username] = options[:password] = "_unused"
new(options).engine_ssh_public_key?
end
|
The key isn't needed here, except for the tests: they check that it starts with |
|
I don't believe this gem is used outside of the provider, but I don't think there's any real way to know. So, yeah, for SemVer reasons, you'll have to leave it as is, unless you cut a major version. |
| expect_any_instance_of(described_class).to receive(:engine_ssh_public_key).and_raise(RestClient::ResourceNotFound) | ||
| expect(described_class.ovirt?(:server => "127.0.0.1")).to be false | ||
| context "#ssh_public_key" do | ||
| BASE_URI = "https://nobody.com" |
There was a problem hiding this comment.
You could use let to define these strings then refer to it like a method call in your tests.
let(:base_uri) { "https://nobody.com" }
|
@jhernand I'm okay with the unhandled exception rubocop issue since it is well documented. |
Currently this gem tests for the presence of the oVirt engine sending a request to the following URL: http://.../engine.ssh.key.txt This doesn't work with version 4 of the engine, as that path was removed. In order to support older and newer versions of the engine this patch modifies the gem so that it checks first the path that works since version 3.5 of the engine, and then, if that doesn't work, the old path. Bug-Url: https://bugzilla.redhat.com/1382732 Signed-off-by: Juan Hernandez <[email protected]>
This patch changes ghe 'service_spec.rb' file so that it uses 'let' instead of module constants, as otherwise Rubocop doesn't like that the constants aren't frozen. Signed-off-by: Juan Hernandez <[email protected]>
|
@bdunne I addressed your comments in the parts that are relevant for the patch, and introduced another patch in the pull request to address them in the parts that aren't relevant. Hopefully Rubocop will be satisfied now. |
|
<github_pr_commenter_batch />Some comments on commits https://github.com/jhernand/ovirt/compare/3f438dea65f71dfa2ae389b73938947eb62e1a3d~...a170624ad8c2b631f29f1899fab426ee84455e67 spec/service_spec.rb
|
|
Checked commits https://github.com/jhernand/ovirt/compare/3f438dea65f71dfa2ae389b73938947eb62e1a3d~...a170624ad8c2b631f29f1899fab426ee84455e67 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 lib/ovirt/service.rb
|
Currently this gem tests for the presence of the oVirt engine sending a
request to the following URL:
http://.../engine.ssh.key.txt
This doesn't work with version 4 of the engine, as that path was
removed. In order to support older and newer versions of the engine this
patch modifies the gem so that it checks first the path that works since
version 3.5 of the engine, and then, if that doesn't work, the old path.
https://bugzilla.redhat.com/1382732