Extract ExternalCommand class

README.md

@@ -53,3 +53,22 @@ Use the `CleanedOutput` class to obtain a cleaned version of an external command
 cleaned_output = CleanedOutput.new(output: 'key=secret pw=password', redactions: ['secret', 'password'])
 puts cleaned_output.to_s # => "key=REDACTED pw=REDACTED"
 ```
+
+### Running external commands
+
+The main task this app performs is running the EveryPolitician build process as an external command and then creating a pull request with the output from the command. As such, one of the main classes in the system is the `ExternalCommand` class. This encapsulates running an external command, checking for errors and returning the output.
+
+To use this class create an instance of `ExternalCommand` and pass `command` and `env` keyword arguments to the constructor. The `command` should be a string representing the external command to execute, and `env` is an optional hash containing environment variables that should be set, or if they are `nil`, unset.
+
+Note that a given instance will only run an external command once. Create a new instance if you need to run the command again.
+
+```ruby
+command = ExternalCommand.new(
+  command: 'echo "Hello, $name."',
+  env: { 'name' => 'Bob' }
+).run
+
+# Now you can access the output and status of the command.
+command.output # => "Hello, Bob.\n"
+command.success? # => true
+```

app.rb

@@ -4,9 +4,9 @@
 Dotenv.load
 
 require 'active_support/core_ext'
-require 'English'
 
 require_relative './lib/cleaned_output'
+require_relative './lib/external_command'
 
 configure :production do
   require 'rollbar/middleware/sinatra'
@@ -51,19 +51,21 @@ def perform(country_slug, legislature_slug, source = nil)
 
     branch = [country_slug, legislature_slug, Time.now.to_i].join('-').parameterize
 
-    output, child_status = run(
-      "#{File.join(__dir__, 'bin/everypolitician-data-builder')} 2>&1",
-      'BRANCH_NAME'           => branch,
-      'GIT_CLONE_URL'         => clone_url.to_s,
-      'LEGISLATURE_DIRECTORY' => File.dirname(legislature.popolo),
-      'SOURCE_NAME'           => source,
-      'COUNTRY_NAME'          => country.name,
-      'COUNTRY_SLUG'          => country.slug
-    )
-
-    cleaned_output = CleanedOutput.new(output: output, redactions: [ENV['MORPH_API_KEY']])
-
-    unless child_status && child_status.success?
+    build_command = ExternalCommand.new(
+      command: "#{File.join(__dir__, 'bin/everypolitician-data-builder')} 2>&1",
+      env:     {
+        'BRANCH_NAME'           => branch,
+        'GIT_CLONE_URL'         => clone_url.to_s,
+        'LEGISLATURE_DIRECTORY' => File.dirname(legislature.popolo),
+        'SOURCE_NAME'           => source,
+        'COUNTRY_NAME'          => country.name,
+        'COUNTRY_SLUG'          => country.slug,
+      }
+    ).run
+
+    cleaned_output = CleanedOutput.new(output: build_command.output, redactions: [ENV['MORPH_API_KEY']])
+
+    unless build_command.success?
       Rollbar.error("Failed to build #{country.name} - #{legislature.name}\n\n#{cleaned_output}")
       return
     end
@@ -95,30 +97,6 @@ def clone_url
   def repo
     @repo ||= github.repository(EVERYPOLITICIAN_DATA_REPO)
   end
-
-  # Unset bundler environment variables so it uses the correct Gemfile etc.
-  def env
-    @env ||= {
-      'BUNDLE_GEMFILE'                => nil,
-      'BUNDLE_BIN_PATH'               => nil,
-      'RUBYOPT'                       => nil,
-      'RUBYLIB'                       => nil,
-      'NOKOGIRI_USE_SYSTEM_LIBRARIES' => '1',
-    }
-  end
-
-  def run(command, extra_env = {})
-    with_tmp_dir do
-      output = IO.popen(env.merge(extra_env), command, &:read)
-      [output, $CHILD_STATUS]
-    end
-  end
-
-  def with_tmp_dir(&block)
-    Dir.mktmpdir do |tmp_dir|
-      Dir.chdir(tmp_dir, &block)
-    end
-  end
 end
 
 class CreatePullRequestJob

lib/external_command.rb

@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+require 'English'
+require_relative 'external_command/result'
+
+class ExternalCommand
+  def initialize(command:, env: {})
+    @command = command
+    @env = env
+  end
+
+  def run
+    with_tmp_dir do
+      Result.new(
+        output:       IO.popen(env, command, &:read),
+        child_status: $CHILD_STATUS
+      )
+    end
+  end
+
+  private
+
+  attr_reader :env, :command
+
+  def with_tmp_dir(&block)
+    Dir.mktmpdir do |tmp_dir|
+      Dir.chdir(tmp_dir, &block)
+    end
+  end
+end

lib/external_command/result.rb

@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+class ExternalCommand
+  class Result
+    attr_reader :output
+
+    def initialize(output:, child_status:)
+      @output = output
+      @child_status = child_status
+    end
+
+    def success?
+      child_status.success?
+    end
+
+    private
+
+    attr_reader :child_status
+  end
+end

test/external_command_test.rb

@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+require 'test_helper'
+require_relative '../lib/external_command'
+
+describe 'ExternalCommand' do
+  describe 'running a simple command' do
+    subject { ExternalCommand.new(command: 'echo Hello, world.').run }
+
+    it 'returns the expected output' do
+      subject.output.must_equal "Hello, world.\n"
+    end
+
+    it 'is considered to be successful' do
+      subject.success?.must_equal true
+    end
+  end
+
+  describe 'running a command with environment variables set' do
+    subject { ExternalCommand.new(command: 'echo Hello, $name.', env: { 'name' => 'Bob' }).run }
+
+    it 'returns the expected output' do
+      subject.output.must_equal "Hello, Bob.\n"
+    end
+
+    it 'is considered to be successful' do
+      subject.success?.must_equal true
+    end
+  end
+
+  describe 'running an external command that fails' do
+    subject { ExternalCommand.new(command: 'echo fail; exit 1').run }
+
+    it 'returns the expected output' do
+      subject.output.must_equal "fail\n"
+    end
+
+    it 'is considered to be a failure' do
+      subject.success?.must_equal false
+    end
+  end
+
+  describe 'running commands in a tmpdir' do
+    let(:other_command) { ExternalCommand.new(command: 'pwd').run }
+    subject { ExternalCommand.new(command: 'pwd').run }
+
+    it 'runs in a different directory each time' do
+      subject.output.wont_equal other_command.output
+    end
+  end
+end