webhook swallowing r10k error messages

[Rudikza]

Affected Puppet, Ruby, OS and module versions/distributions

• Puppet: 5.5.6
• Ruby: 2.4.4p296
• Distribution: RHEL 7.5
• Module version: 6.7.1-rc0

What are you seeing

I had an issue where the webhook was not fetching/building the puppet forge modules. There was nothing in the webhook access.log and when I ran r10k from the command line it worked perfectly. After much digging (and using pry-remote) I found that r10k was being blocked by our proxy server.

The proxy is a quick fix but I was more concerned that the webhook app seemed to swallow the errors from r10k.

I found that this block of code seemed to the cause of the problem:

    def run_command(command)
      message = ''
      File.open(LOCKFILE, 'w+') do |file|
        # r10k has a small race condition which can cause failed deploys if two happen
        # more or less simultaneously. To mitigate, we just lock on a file and wait for
        # the other one to complete.
        file.flock(File::LOCK_EX)

        message = "triggered: #{command}"
        exec "#{command} &"
        exit_status = 0
        raise "#{stdout}\n#{stderr}" if exit_status != 0
      end
      message
    end

I played around a bit with Open3.capture2e and came up with the following code which correctly picks up the failure, logs the error messages to the log file and then raises a failure.

    def run_command(command)
      message = ''
      File.open(LOCKFILE, 'w+') do |file|
        # r10k has a small race condition which can cause failed deploys if two happen
        # more or less simultaneously. To mitigate, we just lock on a file and wait for
        # the other one to complete.
        file.flock(File::LOCK_EX)
        message = "triggered: #{command}"
        stderr, status = Open3.capture2e(command)
        unless status.success?
          ar = stderr.split("\n").grep /ERROR/
          ar.each { |e| $logger.error(e)}
          "#{command} failed to execute"
          raise "Failed to exec  #{command}"
        end
      end
      message
    end

The logfile formatting needs a bit of work (https://gist.github.com/Rudikza/5d519b206f9879f25796f5b3e71dce6e) and we should perhaps look at adding a error log file? I'm also not sure if the raise is the correct way to handle this type of error.

Would this be worth adding to the module and spending more time on?