infrablocks / ruby_terraform

A simple Ruby wrapper for invoking terraform commands.
MIT License
109 stars 33 forks source link

Flush logger #37

Closed jesusbv closed 4 years ago

jesusbv commented 4 years ago

When configuring RubyTerraform with a logger file, it needs to flush or some commands could go missing.

Steps to reproduce:

  1. Set RubyTerraform.configuration with a file as logger e.g.
    log_file = File.open('/foo/bar.log', 'a')
    RubyTerraform.configure do |config|
    config.binary = '/binary/path/terraform'
    config.logger = Logger.new(
    RubyTerraform::MultiIO.new(STDOUT, log_file),
    level: :debug
    )
    config.stdout = config.logger
    config.stderr = config.logger
    end
  2. Run RubyTerraform init in a dir without terraform vars

The output of that is

D, [timestamp] DEBUG -- : Running '/binary/path/terraform init'.
Terraform initialized in an empty directory!

The directory has no Terraform configuration files. You may begin working
with Terraform immediately by creating Terraform configuration files.

it is expected in STDOUT and in /foo/bar.log, however, the file is empty.

tobyclemson commented 4 years ago

@jesusbv This is definitely a problem although I'm not sure MultiIO is the right place to fix it since as far as I can tell it won't fix it for the case where only a file is used and not a file and standard out.

Any ideas how else we could achieve the same?

jesusbv commented 4 years ago

@tobyclemson The problem comes because of the buffering. One solution that works is disable that option,

Following the same example

log_file = File.open('/foo/bar.log', 'a')
log_file.sync = true # implicit flushing 
RubyTerraform.configure do |config|
  config.binary = '/binary/path/terraform'
  config.logger = Logger.new(
    RubyTerraform::MultiIO.new(STDOUT, log_file),
    level: :debug
  )
  config.stdout = config.logger
  config.stderr = config.logger
end

However, the responsibility to do that falls on the user side, I rather that was not the case and users did not have to know/worry about that.

If this is the chosen option, we may want to document it in the README

jesusbv commented 4 years ago

@tobyclemson thoughts ?

tobyclemson commented 4 years ago

@jesusbv I'm happy to merge this for now but I think there must be a better solution. Force flushing on every write is going to result in some disk thrashing unnecessarily. Really we just need to ensure we flush before exit. It would also be good for terraform to manage this for other loggers than those using MultiIO as I mentioned.

I'll merge this in and have a think about it independently.

tobyclemson commented 4 years ago

After a little more research, it seems the logger stdlib solves this problem for us. If instead of opening a File directly, you use an instance of Logger::LogDevice file synchronisation is managed automatically:

require 'logger'

log_file = Logger::LogDevice.new('/foo/bar.log') # results in a file with sync true in the background
logger = Logger.new(RubyTerraform::MultiIO.new(STDOUT, log_file), level: :debug)

RubyTerraform.configure do |config|
  config.binary = '/binary/path/terraform'
  config.logger = logger
  config.stdout = logger
  config.stderr = logger
end

As a result, I think we could just update the docs and not merge this PR.

jesusbv commented 4 years ago

I will take a look, then update the docs and close this issue.

jesusbv commented 4 years ago

Tested a while ago, and it has been updated and in use since. Close this and will update the docs as agreed.

tobyclemson commented 4 years ago

Thanks @jesusbv