Minor code quality improvements to host key code

app/models/ssh_host_key.rb

@@ -20,15 +20,16 @@ class SshHostKey
 
   self.reactive_cache_key = ->(key) { [key.class.to_s, key.id] }
 
-  # Do not refresh the data in the background - it is not expected to change
+  # Do not refresh the data in the background - it is not expected to change.
+  # This is achieved by making the lifetime shorter than the refresh interval.
   self.reactive_cache_refresh_interval = 15.minutes
   self.reactive_cache_lifetime = 10.minutes
 
   def self.find_by(opts = {})
-    id = opts.fetch(:id, "")
-    project_id, url = id.split(':', 2)
+    return nil unless opts.key?(:id)
 
-    project = Project.where(id: project_id).includes(:import_data).first
+    project_id, url = opts[:id].split(':', 2)
+    project = Project.find_by(id: project_id)
 
     project.presence && new(project: project, url: url)
   end
@@ -96,13 +97,13 @@ class SshHostKey
 
     # ssh-keyscan returns an exit code 0 in several error conditions, such as an
     # unknown hostname, so check both STDERR and the exit code
-    if !status.success? || errors.present?
+    if status.success? && !errors.present?
+      { known_hosts: known_hosts }
+    else
       Rails.logger.debug("Failed to detect SSH host keys for #{id}: #{errors}")
 
-      return { error: 'Failed to detect SSH host keys' }
+      { error: 'Failed to detect SSH host keys' }
     end
-
-    { known_hosts: known_hosts }
   end
 
   private
@@ -112,8 +113,7 @@ class SshHostKey
     data
       .to_s
       .each_line
-      .map { |line| line unless line.start_with?('#') || line.chomp.empty? }
-      .compact
+      .reject { |line| line.start_with?('#') || line.chomp.empty? }
       .uniq
       .sort
       .join

spec/controllers/projects/mirrors_controller_spec.rb

@@ -72,10 +72,12 @@ describe Projects::MirrorsController do
     end
 
     context 'invalid URLs' do
-      where(url: %w[INVALID git@example.com:foo/bar.git ssh://git@example.com:foo/bar.git])
-
-      with_them do
-        it 'returns an error with a 400 response' do
+      %w[
+        INVALID
+        git@example.com:foo/bar.git
+        ssh://git@example.com:foo/bar.git
+      ].each do |url|
+        it "returns an error with a 400 response for URL #{url.inspect}" do
           do_get(project, url)
 
           expect(response).to have_gitlab_http_status(400)

spec/models/ssh_host_key_spec.rb

@@ -85,7 +85,7 @@ describe SshHostKey do
   end
 
   describe '#host_keys_changed?' do
-    where(:a, :b, :result) do
+    where(:known_hosts_a, :known_hosts_b, :result) do
       known_hosts | extra       | true
       known_hosts | "foo\n"     | true
       known_hosts | ''          | true
@@ -99,15 +99,15 @@ describe SshHostKey do
     end
 
     with_them do
-      let(:compare_host_keys) { b }
+      let(:compare_host_keys) { known_hosts_b }
 
       subject { ssh_host_key.host_keys_changed? }
 
       context '(normal)' do
-        let(:compare_host_keys) { b }
+        let(:compare_host_keys) { known_hosts_b }
 
         before do
-          expect(ssh_host_key).to receive(:known_hosts).and_return(a)
+          expect(ssh_host_key).to receive(:known_hosts).and_return(known_hosts_a)
         end
 
         it { is_expected.to eq(result) }
@@ -115,10 +115,10 @@ describe SshHostKey do
 
       # Comparisons should be symmetrical, so test the reverse too
       context '(reversed)' do
-        let(:compare_host_keys) { a }
+        let(:compare_host_keys) { known_hosts_a }
 
         before do
-          expect(ssh_host_key).to receive(:known_hosts).and_return(b)
+          expect(ssh_host_key).to receive(:known_hosts).and_return(known_hosts_b)
         end
 
         it { is_expected.to eq(result) }