ronantakizawa/codereview-bench huggingface dataset:
Repository: mrsked/mrsk
File: lib/kamal/commands/app/logging.rb (Ruby)

Code before:
module Kamal::Commands::App::Logging
  def logs(version: nil, timestamps: true, since: nil, lines: nil, grep: nil, grep_options: nil)
    pipe \
      version ? container_id_for_version(version) : current_running_container_id,
      "xargs docker logs#{" --timestamps" if timestamps}#{" --since #{since}" if since}#{" --tail #{lines}" if lines} 2>&1",
      ("grep '#{grep}'#{" #{grep_options}" if grep_options}" if grep)
  end

  def follow_logs(host:, timestamps: true, lines: nil, grep: nil, grep_options: nil)
    run_over_ssh \
      pipe(
        current_running_container_id,
        "xargs docker logs#{" --timestamps" if timestamps}#{" --tail #{lines}" if lines} --follow 2>&1",
        (%(grep "#{grep}"#{" #{grep_options}" if grep_options}) if grep)
      ),
      host: host
  end
end


Review comment (suggestion):
TBH, I'm not happy about this approach, but I couldn't come up with a better approach that would allow passing literal container ID (via kamal command) and a command that outputs container ID (see: [lib/kamal/cli/app/boot.rb](https://github.com/basecamp/kamal/pull/906/files#diff-16c00ee2b4b547d9ca9cd78761f722f4f4cc56b0f0c709c61c9beab9960f08e1R94))

Code after:
module Kamal::Commands::App::Logging
  def logs(container_id: nil, timestamps: true, since: nil, lines: nil, grep: nil, grep_options: nil)
    pipe \
      container_id_command(container_id),
      "xargs docker logs#{" --timestamps" if timestamps}#{" --since #{since}" if since}#{" --tail #{lines}" if lines} 2>&1",
      ("grep '#{grep}'#{" #{grep_options}" if grep_options}" if grep)
  end

  def follow_logs(host:, container_id: nil, timestamps: true, lines: nil, grep: nil, grep_options: nil)
    run_over_ssh \
      pipe(
        container_id_command(container_id),
        "xargs docker logs#{" --timestamps" if timestamps}#{" --tail #{lines}" if lines} --follow 2>&1",
        (%(grep "#{grep}"#{" #{grep_options}" if grep_options}) if grep)
      ),
      host: host
  end

  private

  def container_id_command(container_id)
    case container_id
    when Array then container_id
    when String, Symbol then "echo #{container_id}"
    else current_running_container_id
    end
  end
end
Repository: mrsked/mrsk
File: lib/kamal/commands/builder/base.rb (Ruby)

Code before:

  def clean
    docker :image, :rm, "--force", config.absolute_image
  end

  def push
    docker :buildx, :build,
      "--push",
      *platform_options(arches),
      *([ "--builder", builder_name ] unless docker_driver?),
      *build_options,
      build_context
  end

  def pull
    docker :pull, config.absolute_image
  end

  def info
    combine \
      docker(:context, :ls),
      docker(:buildx, :ls)
  end

  def inspect_builder
    docker :buildx, :inspect, builder_name unless docker_driver? || pack?
  end

  def build_options
    [ *build_tags, *build_cache, *build_labels, *build_args, *build_secrets, *build_dockerfile, *build_target, *build_ssh ]
  end

  def build_context
    config.builder.context
  end

  def validate_image
    pipe \
      docker(:inspect, "-f", "'{{ .Config.Labels.service }}'", config.absolute_image),
      any(
        [ :grep, "-x", config.service ],
        "(echo \"Image #{config.absolute_image} is missing the 'service' label\" && exit 1)"
      )
  end

  def first_mirror
    docker(:info, "--format '{{index .RegistryConfig.Mirrors 0}}'")
  end

  private
    def build_tags

Review comment (refactor):
Maybe we could extract a `buildx?` method here?

```
def buildx?
  !docker_driver? && !pack?
end
```

Code after:

  def clean
    docker :image, :rm, "--force", config.absolute_image
  end

  def push(export_action = "registry", tag_as_dirty: false)
    docker :buildx, :build,
      "--output=type=#{export_action}",
      *platform_options(arches),
      *([ "--builder", builder_name ] unless docker_driver?),
      *build_tag_options(tag_as_dirty: tag_as_dirty),
      *build_options,
      build_context,
      "2>&1"
  end

  def pull
    docker :pull, config.absolute_image
  end

  def info
    combine \
      docker(:context, :ls),
      docker(:buildx, :ls)
  end

  def inspect_builder
    docker :buildx, :inspect, builder_name unless docker_driver?
  end

  def build_options
    [ *build_cache, *build_labels, *build_args, *build_secrets, *build_dockerfile, *build_target, *build_ssh, *builder_provenance, *builder_sbom ]
  end

  def build_context
    config.builder.context
  end

  def validate_image
    pipe \
      docker(:inspect, "-f", "'{{ .Config.Labels.service }}'", config.absolute_image),
      any(
        [ :grep, "-x", config.service ],
        "(echo \"Image #{config.absolute_image} is missing the 'service' label\" && exit 1)"
      )
  end

  def first_mirror
    docker(:info, "--format '{{index .RegistryConfig.Mirrors 0}}'")
  end
Repository: mrsked/mrsk
File: lib/kamal/commands/builder/pack.rb (Ruby)

Code before:
class Kamal::Commands::Builder::Pack < Kamal::Commands::Builder::Base
  def push
    combine \
      pack(:build,
        config.repository,
        "--platform", platform,
        "--builder", pack_builder,
        buildpacks,
        "-t", config.absolute_image,
        "-t", config.latest_image,
        "--env", "BP_IMAGE_LABELS=service=#{config.service}",
        *argumentize("--env", secrets, sensitive: true),
        "--path", build_context),
      docker(:push, config.absolute_image),
      docker(:push, config.latest_image)
  end

  private
    def platform
      "linux/#{local_arches.first}"
    end

    def buildpacks
      (pack_buildpacks << "paketo-buildpacks/image-labels").map { |buildpack| [ "--buildpack", buildpack ] }
    end
end


Review comment (security):
I've filed https://github.com/buildpacks/pack/issues/2268 for adding support for `--label` to `pack build`, which if/when added would mean the buildpack injection here could be dropped :-)

Code after:
        "--platform", platform,
        "--creation-time", "now",
        "--builder", pack_builder,
        buildpacks,
        "-t", config.absolute_image,
        "-t", config.latest_image,
        "--env", "BP_IMAGE_LABELS=service=#{config.service}",
        *argumentize("--env", args),
        *argumentize("--env", secrets, sensitive: true),
        "--path", build_context)
    end

    def export(export_action)
      return unless export_action == "registry"

      combine \
        docker(:push, config.absolute_image),
        docker(:push, config.latest_image)
    end

    def platform
      "linux/#{local_arches.first}"
    end

    def buildpacks
      (pack_buildpacks << "paketo-buildpacks/image-labels").map { |buildpack| [ "--buildpack", buildpack ] }
    end
end
Repository: mrsked/mrsk
File: lib/kamal/commands/builder/pack.rb (Ruby)

Code before:
class Kamal::Commands::Builder::Pack < Kamal::Commands::Builder::Base
  def push
    combine \
      pack(:build,
        config.repository,
        "--platform", platform,
        "--builder", pack_builder,
        buildpacks,
        "-t", config.absolute_image,
        "-t", config.latest_image,
        "--env", "BP_IMAGE_LABELS=service=#{config.service}",
        *argumentize("--env", args),
        *argumentize("--env", secrets, sensitive: true),
        "--path", build_context),
      docker(:push, config.absolute_image),
      docker(:push, config.latest_image)
  end

  private
    def platform
      "linux/#{local_arches.first}"
    end

    def buildpacks
      (pack_buildpacks << "paketo-buildpacks/image-labels").map { |buildpack| [ "--buildpack", buildpack ] }
    end
end


Review comment (suggestion):
With docker, build args are passed as `--build-arg` and with Kamal you set them via:

```
  args:
    ENVIRONMENT: production
```

You'd still set "build args" with pack via the same `args` section but they ultimately get passed as `--env` to the pack command. Trying to reduce confusion of when to use env/arg if you're testing out your builds. 

Code after:
class Kamal::Commands::Builder::Pack < Kamal::Commands::Builder::Base
  def push(export_action = "registry")
    combine \
      build,
      export(export_action)
  end

  def remove;end

  def info
    pack :builder, :inspect, pack_builder
  end
  alias_method :inspect_builder, :info

  private
    def build
      pack(:build,
        config.repository,
        "--platform", platform,
        "--creation-time", "now",
        "--builder", pack_builder,
        buildpacks,
        "-t", config.absolute_image,
        "-t", config.latest_image,
        "--env", "BP_IMAGE_LABELS=service=#{config.service}",
        *argumentize("--env", args),
        *argumentize("--env", secrets, sensitive: true),
        "--path", build_context)
    end

    def export(export_action)
      return unless export_action == "registry"

      combine \
        docker(:push, config.absolute_image),
        docker(:push, config.latest_image)
    end

    def platform
      "linux/#{local_arches.first}"
    end

    def buildpacks
      (pack_buildpacks << "paketo-buildpacks/image-labels").map { |buildpack| [ "--buildpack", buildpack ] }
    end
end
Repository: mrsked/mrsk
File: lib/kamal/secrets.rb (Ruby)

Code before:
require "dotenv"

class Kamal::Secrets
  attr_reader :secrets_file

  Kamal::Secrets::Dotenv::InlineCommandSubstitution.install!

  def initialize(destination: nil)
    @secrets_file = [ *(".kamal/secrets.#{destination}" if destination), ".kamal/secrets" ].find { |f| File.exist?(f) }
  end

  def [](key)
    secrets.fetch(key)
  rescue KeyError
    if secrets_file
      raise Kamal::ConfigurationError, "Secret '#{key}' not found in #{secrets_file}"
    else
      raise Kamal::ConfigurationError, "Secret '#{key}' not found, no secret files provided"
    end
  end

  def to_h
    secrets
  end

  private
    def secrets
      @secrets ||= parse_secrets
    end

    def parse_secrets
      if secrets_file
        ::Dotenv.parse(secrets_file)
      else
        {}

Review comment (security):
Stylistic suggestion: `secrets.common` would be more inline with the rest of the destination file naming format.

Code after:
require "dotenv"

class Kamal::Secrets
  attr_reader :secrets_files

  Kamal::Secrets::Dotenv::InlineCommandSubstitution.install!

  def initialize(destination: nil)
    @secrets_files = \
      [ ".kamal/secrets-common", ".kamal/secrets#{(".#{destination}" if destination)}" ].select { |f| File.exist?(f) }
  end

  def [](key)
    secrets.fetch(key)
  rescue KeyError
    if secrets_files
      raise Kamal::ConfigurationError, "Secret '#{key}' not found in #{secrets_files.join(", ")}"
    else
      raise Kamal::ConfigurationError, "Secret '#{key}' not found, no secret files provided"
    end
  end

  def to_h
    secrets
  end

  private
    def secrets
      @secrets ||= secrets_files.inject({}) do |secrets, secrets_file|
        secrets.merge!(::Dotenv.parse(secrets_file))
      end
    end
end
Repository: mrsked/mrsk
File: lib/kamal/commands/base.rb (Ruby)

Code before:
module Kamal::Commands
  class Base
    delegate :sensitive, :argumentize, to: Kamal::Utils

    DOCKER_HEALTH_STATUS_FORMAT = "'{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}'"

    attr_accessor :config

    def initialize(config)
      @config = config
    end

    def run_over_ssh(*command, host:)
      "ssh".then do |cmd|
        if config.ssh.proxy && config.ssh.proxy.is_a?(Net::SSH::Proxy::Jump)
          cmd = "#{cmd} -J #{config.ssh.proxy.jump_proxies}"
        elsif config.ssh.proxy && config.ssh.proxy.is_a?(Net::SSH::Proxy::Command)
          cmd = "#{cmd} -o ProxyCommand='#{config.ssh.proxy.command_line_template}'"
        end
        "#{cmd} -t #{config.ssh.user}@#{host} -p #{config.ssh.port} '#{command.join(" ").gsub("'", "'\\\\''")}'"
      end
    end

    def container_id_for(container_name:, only_running: false)
      docker :container, :ls, *("--all" unless only_running), "--filter", "name=^#{container_name}$", "--quiet"
    end

    def make_directory_for(remote_file)
      make_directory Pathname.new(remote_file).dirname.to_s
    end

    def make_directory(path)
      [ :mkdir, "-p", path ]
    end

    def remove_directory(path)
      [ :rm, "-r", path ]
    end

    def remove_file(path)
      [ :rm, path ]
    end

    private
      def combine(*commands, by: "&&")

Review comment (refactor):
Would extract an `ssh_proxy_args` method to sidestep this:
```ruby
def run_over_ssh(*command, host:)
  "ssh #{ssh_proxy_args} -t …"
end

def ssh_proxy_args
  case config.ssh.proxy
  when Net::SSH::Proxy::Jump
    "-J #{config.ssh.proxy.jump_proxies}"
  when Net::SSH::Proxy::Command
    "-o ProxyCommand='#{config.ssh.proxy.command_line_template}'"
  end
end
```

Code after:
module Kamal::Commands
  class Base
    delegate :sensitive, :argumentize, to: Kamal::Utils

    DOCKER_HEALTH_STATUS_FORMAT = "'{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}'"

    attr_accessor :config

    def initialize(config)
      @config = config
    end

    def run_over_ssh(*command, host:)
      "ssh#{ssh_proxy_args} -t #{config.ssh.user}@#{host} -p #{config.ssh.port} '#{command.join(" ").gsub("'", "'\\\\''")}'"
    end

    def container_id_for(container_name:, only_running: false)
      docker :container, :ls, *("--all" unless only_running), "--filter", "name=^#{container_name}$", "--quiet"
    end

    def make_directory_for(remote_file)
      make_directory Pathname.new(remote_file).dirname.to_s
    end

    def make_directory(path)
      [ :mkdir, "-p", path ]
    end

    def remove_directory(path)
      [ :rm, "-r", path ]
    end

    def remove_file(path)
      [ :rm, path ]
    end

    private
      def combine(*commands, by: "&&")
        commands
          .compact
          .collect { |command| Array(command) + [ by ] }.flatten # Join commands
          .tap     { |commands| commands.pop } # Remove trailing combiner
      end

      def chain(*commands)
Repository: mrsked/mrsk
File: lib/kamal/commands/base.rb (Ruby)

Code before:
        [ :sh, "-c", "'#{command.flatten.join(" ").gsub("'", "'\\\\''")}'" ]
      end

      def docker(*args)
        args.compact.unshift :docker
      end

      def git(*args, path: nil)
        [ :git, *([ "-C", path ] if path), *args.compact ]
      end

      def grep(*args)
        args.compact.unshift :grep
      end

      def tags(**details)
        Kamal::Tags.from_config(config, **details)
      end

      def ssh_proxy_args
        if config.ssh.proxy && config.ssh.proxy.is_a?(Net::SSH::Proxy::Jump)
          "-J #{config.ssh.proxy.jump_proxies}"
        elsif config.ssh.proxy && config.ssh.proxy.is_a?(Net::SSH::Proxy::Command)
          "-o ProxyCommand='#{config.ssh.proxy.command_line_template}'"
        end
      end
  end
end


Review comment (suggestion):
```suggestion
      def ssh_proxy_args
        case config.ssh.proxy
        when Net::SSH::Proxy::Jump
          "-J #{config.ssh.proxy.jump_proxies}"
        when Net::SSH::Proxy::Command
          "-o ProxyCommand='#{config.ssh.proxy.command_line_template}'"
        end
      end
```


Code after:
        [ :sh, "-c", "'#{command.flatten.join(" ").gsub("'", "'\\\\''")}'" ]
      end

      def docker(*args)
        args.compact.unshift :docker
      end

      def git(*args, path: nil)
        [ :git, *([ "-C", path ] if path), *args.compact ]
      end

      def grep(*args)
        args.compact.unshift :grep
      end

      def tags(**details)
        Kamal::Tags.from_config(config, **details)
      end

      def ssh_proxy_args
        case config.ssh.proxy
        when Net::SSH::Proxy::Jump
          " -J #{config.ssh.proxy.jump_proxies}"
        when Net::SSH::Proxy::Command
          " -o ProxyCommand='#{config.ssh.proxy.command_line_template}'"
        end
      end
  end
end
Repository: mrsked/mrsk
File: lib/kamal/commands/base.rb (Ruby)

Code before:
        [ :xargs, command ].flatten
      end

      def shell(command)
        [ :sh, "-c", "'#{command.flatten.join(" ").gsub("'", "'\\\\''")}'" ]
      end

      def docker(*args)
        args.compact.unshift :docker
      end

      def git(*args, path: nil)
        [ :git, *([ "-C", path ] if path), *args.compact ]
      end

      def grep(*args)
        args.compact.unshift :grep
      end

      def tags(**details)
        Kamal::Tags.from_config(config, **details)
      end

      def ssh_proxy_args
        case config.ssh.proxy
        when Net::SSH::Proxy::Jump then "-J #{config.ssh.proxy.jump_proxies}"
        when Net::SSH::Proxy::Command
          "-o ProxyCommand='#{config.ssh.proxy.command_line_template}'"
        end
      end
  end
end


Review comment (suggestion):
```suggestion
        when Net::SSH::Proxy::Jump
          "-J #{config.ssh.proxy.jump_proxies}"
```


Code after:
        [ :xargs, command ].flatten
      end

      def shell(command)
        [ :sh, "-c", "'#{command.flatten.join(" ").gsub("'", "'\\\\''")}'" ]
      end

      def docker(*args)
        args.compact.unshift :docker
      end

      def git(*args, path: nil)
        [ :git, *([ "-C", path ] if path), *args.compact ]
      end

      def grep(*args)
        args.compact.unshift :grep
      end

      def tags(**details)
        Kamal::Tags.from_config(config, **details)
      end

      def ssh_proxy_args
        case config.ssh.proxy
        when Net::SSH::Proxy::Jump
          " -J #{config.ssh.proxy.jump_proxies}"
        when Net::SSH::Proxy::Command
          " -o ProxyCommand='#{config.ssh.proxy.command_line_template}'"
        end
      end
  end
end
Repository: mrsked/mrsk
File: lib/kamal/commands/base.rb (Ruby)

Code before:
module Kamal::Commands
  class Base
    delegate :sensitive, :argumentize, to: Kamal::Utils

    DOCKER_HEALTH_STATUS_FORMAT = "'{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}'"

    attr_accessor :config

    def initialize(config)
      @config = config
    end

    def run_over_ssh(*command, host:)
      "ssh #{ssh_proxy_args} -t #{config.ssh.user}@#{host} -p #{config.ssh.port} '#{command.join(" ").gsub("'", "'\\\\''")}'".squish
    end

    def container_id_for(container_name:, only_running: false)
      docker :container, :ls, *("--all" unless only_running), "--filter", "name=^#{container_name}$", "--quiet"
    end

    def make_directory_for(remote_file)
      make_directory Pathname.new(remote_file).dirname.to_s
    end

    def make_directory(path)
      [ :mkdir, "-p", path ]
    end

    def remove_directory(path)
      [ :rm, "-r", path ]
    end

    def remove_file(path)
      [ :rm, path ]
    end

    private
      def combine(*commands, by: "&&")
        commands

Review comment (suggestion):
```suggestion
      "ssh#{ssh_proxy_args} -t #{config.ssh.user}@#{host} -p #{config.ssh.port} '#{command.join(" ").gsub("'", "'\\\\''")}'".squish
```

Code after:
module Kamal::Commands
  class Base
    delegate :sensitive, :argumentize, to: Kamal::Utils

    DOCKER_HEALTH_STATUS_FORMAT = "'{{if .State.Health}}{{.State.Health.Status}}{{else}}{{.State.Status}}{{end}}'"

    attr_accessor :config

    def initialize(config)
      @config = config
    end

    def run_over_ssh(*command, host:)
      "ssh#{ssh_proxy_args} -t #{config.ssh.user}@#{host} -p #{config.ssh.port} '#{command.join(" ").gsub("'", "'\\\\''")}'"
    end

    def container_id_for(container_name:, only_running: false)
      docker :container, :ls, *("--all" unless only_running), "--filter", "name=^#{container_name}$", "--quiet"
    end

    def make_directory_for(remote_file)
      make_directory Pathname.new(remote_file).dirname.to_s
    end

    def make_directory(path)
      [ :mkdir, "-p", path ]
    end

    def remove_directory(path)
      [ :rm, "-r", path ]
    end

    def remove_file(path)
      [ :rm, path ]
    end

    private
      def combine(*commands, by: "&&")
        commands
Repository: mrsked/mrsk
File: lib/kamal/commands/base.rb (Ruby)

Code before:
      end

      def shell(command)
        [ :sh, "-c", "'#{command.flatten.join(" ").gsub("'", "'\\\\''")}'" ]
      end

      def docker(*args)
        args.compact.unshift :docker
      end

      def git(*args, path: nil)
        [ :git, *([ "-C", path ] if path), *args.compact ]
      end

      def grep(*args)
        args.compact.unshift :grep
      end

      def tags(**details)
        Kamal::Tags.from_config(config, **details)
      end

      def ssh_proxy_args
        case config.ssh.proxy
        when Net::SSH::Proxy::Jump
          "-J #{config.ssh.proxy.jump_proxies}"
        when Net::SSH::Proxy::Command
          "-o ProxyCommand='#{config.ssh.proxy.command_line_template}'"
        end
      end
  end
end


Review comment (suggestion):
```suggestion
          " -J #{config.ssh.proxy.jump_proxies}"
```

Code after:
      end

      def shell(command)
        [ :sh, "-c", "'#{command.flatten.join(" ").gsub("'", "'\\\\''")}'" ]
      end

      def docker(*args)
        args.compact.unshift :docker
      end

      def git(*args, path: nil)
        [ :git, *([ "-C", path ] if path), *args.compact ]
      end

      def grep(*args)
        args.compact.unshift :grep
      end

      def tags(**details)
        Kamal::Tags.from_config(config, **details)
      end

      def ssh_proxy_args
        case config.ssh.proxy
        when Net::SSH::Proxy::Jump
          " -J #{config.ssh.proxy.jump_proxies}"
        when Net::SSH::Proxy::Command
          " -o ProxyCommand='#{config.ssh.proxy.command_line_template}'"
        end
      end
  end
end
Repository: mrsked/mrsk
File: lib/kamal/commands/base.rb (Ruby)

Code before:
      def shell(command)
        [ :sh, "-c", "'#{command.flatten.join(" ").gsub("'", "'\\\\''")}'" ]
      end

      def docker(*args)
        args.compact.unshift :docker
      end

      def git(*args, path: nil)
        [ :git, *([ "-C", path ] if path), *args.compact ]
      end

      def grep(*args)
        args.compact.unshift :grep
      end

      def tags(**details)
        Kamal::Tags.from_config(config, **details)
      end

      def ssh_proxy_args
        case config.ssh.proxy
        when Net::SSH::Proxy::Jump
          "-J #{config.ssh.proxy.jump_proxies}"
        when Net::SSH::Proxy::Command
          "-o ProxyCommand='#{config.ssh.proxy.command_line_template}'"
        end
      end
  end
end


Review comment (suggestion):
```suggestion
          " -o ProxyCommand='#{config.ssh.proxy.command_line_template}'"
```

Code after:
      def shell(command)
        [ :sh, "-c", "'#{command.flatten.join(" ").gsub("'", "'\\\\''")}'" ]
      end

      def docker(*args)
        args.compact.unshift :docker
      end

      def git(*args, path: nil)
        [ :git, *([ "-C", path ] if path), *args.compact ]
      end

      def grep(*args)
        args.compact.unshift :grep
      end

      def tags(**details)
        Kamal::Tags.from_config(config, **details)
      end

      def ssh_proxy_args
        case config.ssh.proxy
        when Net::SSH::Proxy::Jump
          " -J #{config.ssh.proxy.jump_proxies}"
        when Net::SSH::Proxy::Command
          " -o ProxyCommand='#{config.ssh.proxy.command_line_template}'"
        end
      end
  end
end