GitHub PullRequestReviewCommentEvent at 2014-01-08 07:43:21
Repository: katello
Actor: pitr-ch
Payload: {"comment":{"url":"https://api.github.com/repos/Katello/katello/pulls/comments/8717286","id":8717286,"diff_hunk":"@@ -88,19 +90,23 @@ def self.configuration_loader\n           root[:path] = \"#{Rails.root}/log\" unless root.key?(:path) if environment\n           root[:type] ||= 'file'\n \n-          unless config.key? :katello_version\n+          if !config.key?(:katello_version) || config.katello_version.nil?\n             config[:katello_version] = begin\n               rpm_command_present = system('which rpm &>/dev/null')\n-              if rpm_command_present\n+              if system(\"cd #{File.dirname(__FILE__)} && git rev-parse --git-dir > /dev/null 2>&1\")\n+                hash = `git rev-parse --short HEAD 2>/dev/null`\n+                $?.exitstatus.zero? ? \"git hash (#{hash.chop})\" : fail(UnknownKatelloVersionException)","path":"lib/katello/load_configuration.rb","position":20,"original_position":20,"commit_id":"f101cfda630dc679c35b117b7c947ff9475567a3","original_commit_id":"f101cfda630dc679c35b117b7c947ff9475567a3","user":{"login":"pitr-ch","id":204460,"avatar_url":"https://gravatar.com/avatar/1390a7fcbc3337885de95bb9b1bcecfd?d=https%3A%2F%2Fidenticons.github.com%2F91eb09e1c2e582bb29aacc38d3915c3e.png&r=x","gravatar_id":"1390a7fcbc3337885de95bb9b1bcecfd","url":"https://api.github.com/users/pitr-ch","html_url":"https://github.com/pitr-ch","followers_url":"https://api.github.com/users/pitr-ch/followers","following_url":"https://api.github.com/users/pitr-ch/following{/other_user}","gists_url":"https://api.github.com/users/pitr-ch/gists{/gist_id}","starred_url":"https://api.github.com/users/pitr-ch/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pitr-ch/subscriptions","organizations_url":"https://api.github.com/users/pitr-ch/orgs","repos_url":"https://api.github.com/users/pitr-ch/repos","events_url":"https://api.github.com/users/pitr-ch/events{/privacy}","received_events_url":"https://api.github.com/users/pitr-ch/received_events","type":"User","site_admin":false},"body":"I know this code is called only once so it may seem like nitpicking but I think this is controlling flow by exception. I think `fail(UnknownKatelloVersionException)` can by just replaced by `\"Unknown\"` (or better by a variable holding the string).\r\n\r\nOr if you want to control the flow `catch` `throw` should be used in this case instead.","created_at":"2014-01-08T07:43:21Z","updated_at":"2014-01-08T07:43:21Z","html_url":"https://github.com/Katello/katello/pull/3546#discussion_r8717286","pull_request_url":"https://api.github.com/repos/Katello/katello/pulls/3546","_links":{"self":{"href":"https://api.github.com/repos/Katello/katello/pulls/comments/8717286"},"html":{"href":"https://github.com/Katello/katello/pull/3546#discussion_r8717286"},"pull_request":{"href":"https://api.github.com/repos/Katello/katello/pulls/3546"}}}}