ronantakizawa/codereview-bench huggingface dataset:
Repository: mrsked/mrsk
File: lib/kamal/configuration/accessory.rb (Ruby)

Code before:
    end

    def specific_volumes
      specifics["volumes"] || []
    end

    def remote_files_as_volumes
      specifics["files"]&.collect do |local_to_remote_mapping|
        _, remote_file = local_to_remote_mapping.split(":")
        "#{service_data_directory + remote_file}:#{remote_file}"
      end || []
    end

    def remote_directories_as_volumes
      specifics["directories"]&.collect do |host_to_container_mapping|
        host_path, container_path = host_to_container_mapping.split(":")
        [ expand_host_path(host_path), container_path ].join(":")
      end || []
    end

    def expand_host_path(host_path)
      absolute_path?(host_path) ? host_path : "#{service_data_directory}/#{host_path}"
    end

    def absolute_path?(path)
      path.slice(0) == "/"
    end

    def service_data_directory
      "$PWD/#{service_name}"
    end

    def hosts_from_host
      if specifics.key?("host")
        host = specifics["host"]
        if host
          [host]
        else
          raise ArgumentError, "Missing host for accessory `#{name}`"
        end
      end
    end

    def hosts_from_hosts
      if specifics.key?("hosts")
        hosts = specifics["hosts"]
        if hosts.is_a?(Array)
          hosts
        else
          raise ArgumentError, "Hosts should be an Array for accessory `#{name}`"
        end

Review comment (suggestion):
```suggestion
      Pathname.new(path).absolute?
```

Code after:
    end

    def specific_volumes
      specifics["volumes"] || []
    end

    def remote_files_as_volumes
      specifics["files"]&.collect do |local_to_remote_mapping|
        _, remote_file = local_to_remote_mapping.split(":")
        "#{service_data_directory + remote_file}:#{remote_file}"
      end || []
    end

    def remote_directories_as_volumes
      specifics["directories"]&.collect do |host_to_container_mapping|
        host_path, container_path = host_to_container_mapping.split(":")
        [ expand_host_path(host_path), container_path ].join(":")
      end || []
    end

    def expand_host_path(host_path)
      absolute_path?(host_path) ? host_path : "#{service_data_directory}/#{host_path}"
    end

    def absolute_path?(path)
      Pathname.new(path).absolute?
    end

    def service_data_directory
      "$PWD/#{service_name}"
    end

    def hosts_from_host
      if specifics.key?("host")
        host = specifics["host"]
        if host
          [host]
        else
          raise ArgumentError, "Missing host for accessory `#{name}`"
        end
      end
    end

    def hosts_from_hosts
      if specifics.key?("hosts")
        hosts = specifics["hosts"]
        if hosts.is_a?(Array)
          hosts
        else
          raise ArgumentError, "Hosts should be an Array for accessory `#{name}`"
        end
Repository: mrsked/mrsk
File: lib/kamal/cli/main.rb (Ruby)

Code before:
class Kamal::Cli::Main < Kamal::Cli::Base
  desc "setup", "Setup all accessories, push the env, and deploy app to servers"
  option :skip_push, aliases: "-P", type: :boolean, default: false, desc: "Skip image build and push"
  def setup
    print_runtime do
      mutating do
        say "Ensure Docker is installed...", :magenta
        invoke "kamal:cli:server:bootstrap"

        say "Push env files...", :magenta
        invoke "kamal:cli:env:push"

        invoke "kamal:cli:accessory:boot", [ "all" ]
        deploy
      end
    end
  end

  desc "deploy", "Deploy app to servers"
  option :skip_push, aliases: "-P", type: :boolean, default: false, desc: "Skip image build and push"
  def deploy
    runtime = print_runtime do
      mutating do
        invoke_options = deploy_options

        say "Log into image registry...", :magenta
        invoke "kamal:cli:registry:login", [], invoke_options


Review comment (suggestion):
Please add tests for the `skip_push` option similar to https://github.com/basecamp/kamal/blob/main/test/cli/main_test.rb#L40

Code after:
class Kamal::Cli::Main < Kamal::Cli::Base
  desc "setup", "Setup all accessories, push the env, and deploy app to servers"
  option :skip_push, aliases: "-P", type: :boolean, default: false, desc: "Skip image build and push"
  def setup
    print_runtime do
      mutating do
        invoke_options = deploy_options

        say "Ensure Docker is installed...", :magenta
        invoke "kamal:cli:server:bootstrap", [], invoke_options

        say "Push env files...", :magenta
        invoke "kamal:cli:env:push", [], invoke_options

        invoke "kamal:cli:accessory:boot", [ "all" ], invoke_options
        deploy
      end
    end
  end

  desc "deploy", "Deploy app to servers"
  option :skip_push, aliases: "-P", type: :boolean, default: false, desc: "Skip image build and push"
  def deploy
    runtime = print_runtime do
      mutating do
        invoke_options = deploy_options

        say "Log into image registry...", :magenta
Repository: mrsked/mrsk
File: lib/kamal/cli/healthcheck.rb (Ruby)

Code before:
class Kamal::Cli::Healthcheck < Kamal::Cli::Base
  default_command :perform

  desc "perform", "Health check current app version"
  def perform
    return unless KAMAL.primary_role.running_traefik?
    on(KAMAL.primary_host) do
      begin
        execute *KAMAL.healthcheck.run
        Poller.wait_for_healthy { capture_with_info(*KAMAL.healthcheck.status) }
      rescue Poller::HealthcheckError => e
        error capture_with_info(*KAMAL.healthcheck.logs)
        error capture_with_pretty_json(*KAMAL.healthcheck.container_health_log)
        raise
      ensure
        execute *KAMAL.healthcheck.stop, raise_on_non_zero_exit: false
        execute *KAMAL.healthcheck.remove, raise_on_non_zero_exit: false
      end
    end
  end
end


Review comment (bug):
Could we raise an exception here instead and protect the callers with if statements? If someone managed to call this without a web role, I think it would be better to fail then to silently pass the check.

Code after:
class Kamal::Cli::Healthcheck < Kamal::Cli::Base
  default_command :perform

  desc "perform", "Health check current app version"
  def perform
    raise "The primary host is not configured to run Traefik" unless KAMAL.config.role(KAMAL.config.primary_role).running_traefik?
    on(KAMAL.primary_host) do
      begin
        execute *KAMAL.healthcheck.run
        Poller.wait_for_healthy { capture_with_info(*KAMAL.healthcheck.status) }
      rescue Poller::HealthcheckError => e
        error capture_with_info(*KAMAL.healthcheck.logs)
        error capture_with_pretty_json(*KAMAL.healthcheck.container_health_log)
        raise
      ensure
        execute *KAMAL.healthcheck.stop, raise_on_non_zero_exit: false
        execute *KAMAL.healthcheck.remove, raise_on_non_zero_exit: false
      end
    end
  end
end
Repository: mrsked/mrsk
File: lib/kamal/cli/healthcheck.rb (Ruby)

Code before:
class Kamal::Cli::Healthcheck < Kamal::Cli::Base
  default_command :perform

  desc "perform", "Health check current app version"
  def perform
    on(KAMAL.primary_host) do
      begin
        execute *KAMAL.healthcheck.run
        Poller.wait_for_healthy { capture_with_info(*KAMAL.healthcheck.status) }
      rescue Poller::HealthcheckError => e
        error capture_with_info(*KAMAL.healthcheck.logs)
        error capture_with_pretty_json(*KAMAL.healthcheck.container_health_log)
        raise
      ensure
        execute *KAMAL.healthcheck.stop, raise_on_non_zero_exit: false
        execute *KAMAL.healthcheck.remove, raise_on_non_zero_exit: false
      end
    end
  end
end


Review comment (suggestion):
`KAMAL.config.role(KAMAL.config.primary_role)` might be too verbose.

In my original changes, `KAMAL.config.primary_role` was an instance of `Role`.

However, with #577, `KAMAL.config.primary_web_role` was introduced as a String.

I didn't want to change that to make the review simpler and focusing on what matters. However, if you think  a cleanup might be interesting, I can submit another PR afterwards

Code after:
class Kamal::Cli::Healthcheck < Kamal::Cli::Base
  default_command :perform

  desc "perform", "Health check current app version"
  def perform
    raise "The primary host is not configured to run Traefik" unless KAMAL.config.role(KAMAL.config.primary_role).running_traefik?
    on(KAMAL.primary_host) do
      begin
        execute *KAMAL.healthcheck.run
        Poller.wait_for_healthy { capture_with_info(*KAMAL.healthcheck.status) }
      rescue Poller::HealthcheckError => e
        error capture_with_info(*KAMAL.healthcheck.logs)
        error capture_with_pretty_json(*KAMAL.healthcheck.container_health_log)
        raise
      ensure
        execute *KAMAL.healthcheck.stop, raise_on_non_zero_exit: false
        execute *KAMAL.healthcheck.remove, raise_on_non_zero_exit: false
      end
    end
  end
end
Repository: mrsked/mrsk
File: lib/kamal/cli/build.rb (Ruby)

Code before:
    run_locally do
      puts "Builder: #{KAMAL.builder.name}"
      puts capture(*KAMAL.builder.info)
    end
  end

  private
    def verify_local_dependencies
      run_locally do
        begin
          execute *KAMAL.builder.ensure_local_dependencies_installed
        rescue SSHKit::Command::Failed => e
          build_error = e.message =~ /command not found/ ?
            "Docker is not installed locally" :
            "Docker buildx plugin is not installed locally"

          raise BuildError, build_error
        end
      end
    end

    def connect_to_remote_host(remote_host)
      remote_uri = URI.parse(remote_host)
      if remote_uri.scheme == "ssh"
        options = { user: remote_uri.user, port: remote_uri.port }.compact
        on(remote_host.gsub('ssh://', ''), options) do
          execute "true"
        end
      end
    end
end


Review comment (suggestion):
I think we could do:

```ruby
host = SSHKit::Host.new(
  hostname: remote_uri.host,
  ssh_options: { user: remote_uri.user, port: remote_uri.port }
)
on(host) do
  execute "true"
end
```

which would avoid needing to alter the options in sshkit_with_ext.rb

Code after:
    run_locally do
      puts "Builder: #{KAMAL.builder.name}"
      puts capture(*KAMAL.builder.info)
    end
  end

  private
    def verify_local_dependencies
      run_locally do
        begin
          execute *KAMAL.builder.ensure_local_dependencies_installed
        rescue SSHKit::Command::Failed => e
          build_error = e.message =~ /command not found/ ?
            "Docker is not installed locally" :
            "Docker buildx plugin is not installed locally"

          raise BuildError, build_error
        end
      end
    end

    def connect_to_remote_host(remote_host)
      remote_uri = URI.parse(remote_host)
      if remote_uri.scheme == "ssh"
        host = SSHKit::Host.new(
          hostname: remote_uri.host,
          ssh_options: { user: remote_uri.user, port: remote_uri.port }.compact
        )
        on(host, options) do
          execute "true"
        end
      end
    end
end
Repository: mrsked/mrsk
File: lib/kamal/cli/server.rb (Ruby)

Code before:
class Kamal::Cli::Server < Kamal::Cli::Base
  desc "exec", "Run a custom command on the server(use --help to show options)"
  option :interactive, type: :boolean, aliases: "-i", default: false, desc: "Run the command interactively(use for console/bash)"
  def exec(cmd)
    hosts = KAMAL.hosts | KAMAL.accessory_hosts

    case
    when options[:interactive]
      host = hosts.first

      say "Running '#{cmd}' on #{host} interactively...", :magenta

      run_locally { exec KAMAL.server.run_over_ssh(cmd, host: host) }
    else
      say "Running '#{cmd}' on #{hosts.join(', ')}...", :magenta

      on(hosts) do |host|
        execute *KAMAL.auditor.record("Executed cmd '#{cmd}' on #{host}"), verbosity: :debug
        puts_by_host host, capture_with_info(cmd)
      end
    end
  end

  desc "bootstrap", "Set up Docker to run Kamal apps"
  def bootstrap
    missing = []


Review comment (suggestion):
```suggestion
  desc "exec", "Run a custom command on the server (use --help to show options)"
```

Code after:
class Kamal::Cli::Server < Kamal::Cli::Base
  desc "exec", "Run a custom command on the server (use --help to show options)"
  option :interactive, type: :boolean, aliases: "-i", default: false, desc: "Run the command interactively (use for console/bash)"
  def exec(cmd)
    hosts = KAMAL.hosts | KAMAL.accessory_hosts

    case
    when options[:interactive]
      host = KAMAL.primary_host

      say "Running '#{cmd}' on #{host} interactively...", :magenta

      run_locally { exec KAMAL.server.run_over_ssh(cmd, host: host) }
    else
      say "Running '#{cmd}' on #{hosts.join(', ')}...", :magenta

      on(hosts) do |host|
        execute *KAMAL.auditor.record("Executed cmd '#{cmd}' on #{host}"), verbosity: :debug
        puts_by_host host, capture_with_info(cmd)
      end
    end
  end

  desc "bootstrap", "Set up Docker to run Kamal apps"
  def bootstrap
    missing = []
Repository: mrsked/mrsk
File: lib/kamal/cli/server.rb (Ruby)

Code before:
class Kamal::Cli::Server < Kamal::Cli::Base
  desc "exec", "Run a custom command on the server(use --help to show options)"
  option :interactive, type: :boolean, aliases: "-i", default: false, desc: "Run the command interactively(use for console/bash)"
  def exec(cmd)
    hosts = KAMAL.hosts | KAMAL.accessory_hosts

    case
    when options[:interactive]
      host = hosts.first

      say "Running '#{cmd}' on #{host} interactively...", :magenta

      run_locally { exec KAMAL.server.run_over_ssh(cmd, host: host) }
    else
      say "Running '#{cmd}' on #{hosts.join(', ')}...", :magenta

      on(hosts) do |host|
        execute *KAMAL.auditor.record("Executed cmd '#{cmd}' on #{host}"), verbosity: :debug
        puts_by_host host, capture_with_info(cmd)
      end
    end
  end

  desc "bootstrap", "Set up Docker to run Kamal apps"
  def bootstrap
    missing = []

    on(KAMAL.hosts | KAMAL.accessory_hosts) do |host|
      unless execute(*KAMAL.docker.installed?, raise_on_non_zero_exit: false)
        if execute(*KAMAL.docker.superuser?, raise_on_non_zero_exit: false)
          info "Missing Docker on #{host}. Installing…"
          execute *KAMAL.docker.install
        else
          missing << host

Review comment (suggestion):
```suggestion
      host = KAMAL.primary_host
```

Code after:
class Kamal::Cli::Server < Kamal::Cli::Base
  desc "exec", "Run a custom command on the server (use --help to show options)"
  option :interactive, type: :boolean, aliases: "-i", default: false, desc: "Run the command interactively (use for console/bash)"
  def exec(cmd)
    hosts = KAMAL.hosts | KAMAL.accessory_hosts

    case
    when options[:interactive]
      host = KAMAL.primary_host

      say "Running '#{cmd}' on #{host} interactively...", :magenta

      run_locally { exec KAMAL.server.run_over_ssh(cmd, host: host) }
    else
      say "Running '#{cmd}' on #{hosts.join(', ')}...", :magenta

      on(hosts) do |host|
        execute *KAMAL.auditor.record("Executed cmd '#{cmd}' on #{host}"), verbosity: :debug
        puts_by_host host, capture_with_info(cmd)
      end
    end
  end

  desc "bootstrap", "Set up Docker to run Kamal apps"
  def bootstrap
    missing = []

    on(KAMAL.hosts | KAMAL.accessory_hosts) do |host|
      unless execute(*KAMAL.docker.installed?, raise_on_non_zero_exit: false)
        if execute(*KAMAL.docker.superuser?, raise_on_non_zero_exit: false)
          info "Missing Docker on #{host}. Installing…"
          execute *KAMAL.docker.install
        else
          missing << host
Repository: mrsked/mrsk
File: lib/kamal/cli/server.rb (Ruby)

Code before:
class Kamal::Cli::Server < Kamal::Cli::Base
  desc "exec", "Run a custom command on the server (use --help to show options)"
  option :interactive, type: :boolean, aliases: "-i", default: false, desc: "Run the command interactively(use for console/bash)"
  def exec(cmd)
    hosts = KAMAL.hosts | KAMAL.accessory_hosts

    case
    when options[:interactive]
      host = KAMAL.primary_host

      say "Running '#{cmd}' on #{host} interactively...", :magenta

      run_locally { exec KAMAL.server.run_over_ssh(cmd, host: host) }
    else
      say "Running '#{cmd}' on #{hosts.join(', ')}...", :magenta

      on(hosts) do |host|
        execute *KAMAL.auditor.record("Executed cmd '#{cmd}' on #{host}"), verbosity: :debug
        puts_by_host host, capture_with_info(cmd)
      end
    end
  end

  desc "bootstrap", "Set up Docker to run Kamal apps"
  def bootstrap
    missing = []

    on(KAMAL.hosts | KAMAL.accessory_hosts) do |host|

Review comment (suggestion):
```suggestion
  option :interactive, type: :boolean, aliases: "-i", default: false, desc: "Run the command interactively (use for console/bash)"
```


Code after:
class Kamal::Cli::Server < Kamal::Cli::Base
  desc "exec", "Run a custom command on the server (use --help to show options)"
  option :interactive, type: :boolean, aliases: "-i", default: false, desc: "Run the command interactively (use for console/bash)"
  def exec(cmd)
    hosts = KAMAL.hosts | KAMAL.accessory_hosts

    case
    when options[:interactive]
      host = KAMAL.primary_host

      say "Running '#{cmd}' on #{host} interactively...", :magenta

      run_locally { exec KAMAL.server.run_over_ssh(cmd, host: host) }
    else
      say "Running '#{cmd}' on #{hosts.join(', ')}...", :magenta

      on(hosts) do |host|
        execute *KAMAL.auditor.record("Executed cmd '#{cmd}' on #{host}"), verbosity: :debug
        puts_by_host host, capture_with_info(cmd)
      end
    end
  end

  desc "bootstrap", "Set up Docker to run Kamal apps"
  def bootstrap
    missing = []

    on(KAMAL.hosts | KAMAL.accessory_hosts) do |host|
Repository: mrsked/mrsk
File: lib/kamal/cli/prune.rb (Ruby)

Code before:
class Kamal::Cli::Prune < Kamal::Cli::Base
  desc "all", "Prune unused images and stopped containers"
  def all
    mutating do
      containers
      images
    end
  end

  desc "images", "Prune unused images"
  def images
    mutating do
      on(KAMAL.hosts) do
        execute *KAMAL.auditor.record("Pruned images"), verbosity: :debug
        execute *KAMAL.prune.dangling_images
        execute *KAMAL.prune.tagged_images
      end
    end
  end

  desc "containers", "Prune all stopped containers, except the last n (default 5)"
  option :retain, type: :numeric, default: nil, desc: "Number of containers to retain"
  def containers
    retain = options.fetch(:retain, KAMAL.config.retain_containers)
    raise "retain must be at least 1" unless retain >= 1

    mutating do
      on(KAMAL.hosts) do
        execute *KAMAL.auditor.record("Pruned containers"), verbosity: :debug
        execute *KAMAL.prune.app_containers(retain: retain)
        execute *KAMAL.prune.healthcheck_containers
      end
    end
  end
end


Review comment (suggestion):
It is a matter of taste, but seems it will be easier to read
``` ruby
raise "retain must be at least 1" if retain < 1
```

Code after:
class Kamal::Cli::Prune < Kamal::Cli::Base
  desc "all", "Prune unused images and stopped containers"
  def all
    mutating do
      containers
      images
    end
  end

  desc "images", "Prune unused images"
  def images
    mutating do
      on(KAMAL.hosts) do
        execute *KAMAL.auditor.record("Pruned images"), verbosity: :debug
        execute *KAMAL.prune.dangling_images
        execute *KAMAL.prune.tagged_images
      end
    end
  end

  desc "containers", "Prune all stopped containers, except the last n (default 5)"
  option :retain, type: :numeric, default: nil, desc: "Number of containers to retain"
  def containers
    retain = options.fetch(:retain, KAMAL.config.retain_containers)
    raise "retain must be at least 1" if retain < 1

    mutating do
      on(KAMAL.hosts) do
        execute *KAMAL.auditor.record("Pruned containers"), verbosity: :debug
        execute *KAMAL.prune.app_containers(retain: retain)
        execute *KAMAL.prune.healthcheck_containers
      end
    end
  end
end
Repository: mrsked/mrsk
File: lib/kamal/commands/lock.rb (Ruby)

Code before:
      read_lock_details
  end

  private
    def write_lock_details(message, version)
      write \
        [ :echo, "\"#{Base64.encode64(lock_details(message, version))}\"" ],
        lock_details_file
    end

    def read_lock_details
      pipe \
        [ :cat, lock_details_file ],
        [ :base64, "-d" ]
    end

    def stat_lock_dir
      write \
        [ :stat, lock_dir ],
        "/dev/null"
    end

    def lock_dir
      dir_name = [ config.service, config.destination ].compact.join("-")

      "#{config.run_directory}/lock-#{dir_name}"
    end

    def lock_details_file
      [ lock_dir, :details ].join("/")
    end

    def lock_details(message, version)
      <<~DETAILS.strip
        Locked by: #{locked_by} at #{Time.now.utc.iso8601}
        Version: #{version}
        Message: #{message}
      DETAILS
    end

    def locked_by
      Kamal::Git.user_name
    rescue Errno::ENOENT
      "Unknown"
    end
end


Review comment (suggestion):
Could we put the locks in `"#{config.run_directory}/locks/#{dir_name}"`? And also use `File.join` to create the path?

We'll need `mkdir -p` when making the directory if we do that.

Code after:
      read_lock_details
  end

  def ensure_locks_directory
    [ :mkdir, "-p", locks_dir ]
  end

  private
    def write_lock_details(message, version)
      write \
        [ :echo, "\"#{Base64.encode64(lock_details(message, version))}\"" ],
        lock_details_file
    end

    def read_lock_details
      pipe \
        [ :cat, lock_details_file ],
        [ :base64, "-d" ]
    end

    def stat_lock_dir
      write \
        [ :stat, lock_dir ],
        "/dev/null"
    end

    def locks_dir
      File.join(config.run_directory, "locks")
    end

    def lock_dir
      dir_name = [ config.service, config.destination ].compact.join("-")

      File.join(locks_dir, dir_name)
    end

    def lock_details_file
      File.join(lock_dir, "details")
    end

    def lock_details(message, version)
      <<~DETAILS.strip
        Locked by: #{locked_by} at #{Time.now.utc.iso8601}
        Version: #{version}
        Message: #{message}
      DETAILS
    end

    def locked_by
      Kamal::Git.user_name
    rescue Errno::ENOENT