Apply review comments

config/config.exs

@@ -24,7 +24,6 @@ config :mime, :types, %{
 
 config :livebook,
   agent_name: "default",
-  mode: :app,
   allowed_uri_schemes: [],
   app_service_name: nil,
   app_service_url: nil,

lib/livebook/application.ex

@@ -326,7 +326,7 @@ defmodule Livebook.Application do
         Application.put_env(:livebook, :apps_path_hub_id, hub_id)
         fun
 
-      Application.get_env(:livebook, :mode) == :app and (teams_key || auth) ->
+      teams_key || auth ->
         Livebook.Config.abort!(
           "You must specify both LIVEBOOK_TEAMS_KEY and LIVEBOOK_TEAMS_AUTH."
         )

lib/livebook/teams/requests.ex

@@ -344,7 +344,12 @@ defmodule Livebook.Teams.Requests do
   defp transform_response({request, response}) do
     case {request, response} do
       {request, %{status: 404}} when request.private.cli and request.private.deploy ->
-        {request, %{response | status: 422, body: %{"errors" => %{"name" => ["does not exist"]}}}}
+        {request,
+         %{
+           response
+           | status: 422,
+             body: %{"errors" => %{"deployment_group" => ["does not exist"]}}
+         }}
 
       {request, %{status: 400, body: %{"errors" => %{"detail" => error}}}}
       when request.private.deploy ->

lib/livebook_cli.ex

@@ -1,32 +1,11 @@
 defmodule LivebookCLI do
   alias LivebookCLI.{Task, Utils}
 
-  @switches [
+  @help_args ["--help", "-h"]
-    help: :boolean,
+  @version_args ["--version", "-v"]
-    version: :boolean
-  ]
-
-  @aliases [
-    h: :help,
-    v: :version
-  ]
-
-  def main(args) do
-    Utils.setup()
-
-    case Utils.option_parse(args, strict: @switches, aliases: @aliases) do
-      {parsed, [], _} when parsed.help -> display_help()
-      {parsed, [name], _} when parsed.help -> Task.usage(name)
-      {parsed, _, _} when parsed.version -> display_version()
-      # We want to keep the switches for the task
-      {_, [name | _], _} -> Task.call(name, List.delete(args, name))
-    end
-  end
-
-  defp display_help() do
-    Utils.print_text("""
-    Livebook is an interactive notebook system for Elixir
 
+  def usage,
+    do: """
     Usage: livebook [command] [options]
 
     Available commands:
@@ -35,6 +14,34 @@ defmodule LivebookCLI do
       livebook deploy    Deploys a notebook to Livebook Teams
 
     The --help and --version options can be given instead of a command for usage and versioning information.\
+    """
+
+  def main(args) do
+    {:ok, _} = Application.ensure_all_started(:elixir)
+    extract_priv!()
+
+    :ok = Application.load(:livebook)
+
+    if unix?() do
+      Application.put_env(:elixir, :ansi_enabled, true)
+    end
+
+    case args do
+      [arg] when arg in @help_args -> display_help()
+      [arg] when arg in @version_args -> display_version()
+      [name | [arg]] when arg in @help_args -> Task.usage(name)
+      [name | args] -> Task.call(name, List.delete(args, name))
+      _args -> Utils.print_text(usage())
+    end
+  end
+
+  defp unix?(), do: match?({:unix, _}, :os.type())
+
+  defp display_help() do
+    Utils.print_text("""
+    Livebook is an interactive notebook system for Elixir
+
+    #{usage()}\
     """)
   end
 
@@ -42,7 +49,45 @@ defmodule LivebookCLI do
     Utils.print_text("""
     #{:erlang.system_info(:system_version)}
     Elixir #{System.build_info()[:build]}
+
     Livebook #{Livebook.Config.app_version()}\
     """)
   end
+
+  import Record
+  defrecord(:zip_file, extract(:zip_file, from_lib: "stdlib/include/zip.hrl"))
+
+  defp extract_priv!() do
+    archive_dir = Path.join(Livebook.Config.tmp_path(), "escript")
+    extracted_path = Path.join(archive_dir, "extracted")
+    in_archive_priv_path = ~c"livebook/priv"
+
+    # In dev we want to extract fresh directory on every boot
+    if Livebook.Config.app_version() =~ "-dev" do
+      File.rm_rf!(archive_dir)
+    end
+
+    # When temporary directory is cleaned by the OS, the directories
+    # may be left in place, so we use a regular file (extracted) to
+    # check if the extracted archive is already available
+    if not File.exists?(extracted_path) do
+      {:ok, sections} = :escript.extract(:escript.script_name(), [])
+      archive = Keyword.fetch!(sections, :archive)
+
+      file_filter = fn zip_file(name: name) ->
+        List.starts_with?(name, in_archive_priv_path)
+      end
+
+      opts = [cwd: String.to_charlist(archive_dir), file_filter: file_filter]
+
+      with {:error, error} <- :zip.extract(archive, opts) do
+        raise "Livebook failed to extract archive files, reason: #{inspect(error)}"
+      end
+
+      File.touch!(extracted_path)
+    end
+
+    priv_dir = Path.join(archive_dir, in_archive_priv_path)
+    Application.put_env(:livebook, :priv_dir, priv_dir, persistent: true)
+  end
 end

lib/livebook_cli/deploy.ex

@@ -40,8 +40,6 @@ defmodule LivebookCLI.Deploy do
 
   @impl true
   def call(args) do
-    Application.put_env(:livebook, :mode, :cli)
-
     {:ok, _} = Application.ensure_all_started(:livebook)
     config = config_from_args(args)
     ensure_config!(config)
@@ -67,35 +65,26 @@ defmodule LivebookCLI.Deploy do
 
     errors =
       Enum.reduce(config, %{}, fn
-        {:session_token, value}, acc when value in ["", nil] ->
+        {key, value}, acc when value in ["", nil] ->
-          add_error(acc, "Deploy Key", "can't be blank")
+          add_error(acc, normalize_key(key), "can't be blank")
 
         {:session_token, value}, acc ->
           if not String.starts_with?(value, @deploy_key_prefix) do
-            add_error(acc, "Deploy Key", "must be a Livebook Teams Deploy Key")
+            add_error(acc, normalize_key(:session_token), "must be a Livebook Teams Deploy Key")
           else
             acc
           end
 
-        {:teams_key, value}, acc when value in ["", nil] ->
-          add_error(acc, "Teams Key", "can't be blank")
-
         {:teams_key, value}, acc ->
           if not String.starts_with?(value, @teams_key_prefix) do
-            add_error(acc, "Teams Key", "must be a Livebook Teams Key")
+            add_error(acc, normalize_key(:teams_key), "must be a Livebook Teams Key")
           else
             acc
           end
 
-        {:deployment_group, value}, acc when value in ["", nil] ->
-          add_error(acc, "Deployment Group", "can't be blank")
-
-        {:path, value}, acc when value in ["", nil] ->
-          add_error(acc, "Path", "can't be blank")
-
         {:path, value}, acc ->
           if not File.exists?(value) do
-            add_error(acc, "Path", "must be a valid path")
+            add_error(acc, normalize_key(:path), "must be a valid path")
           else
             acc
           end
@@ -110,7 +99,7 @@ defmodule LivebookCLI.Deploy do
       raise """
       You configuration is invalid, make sure you are using the correct options for this task.
 
-      #{format_errors(errors)}\
+      #{format_errors(errors, " * ")}\
       """
     end
   end
@@ -126,16 +115,32 @@ defmodule LivebookCLI.Deploy do
   end
 
   defp deploy_to_teams(team, config) do
-    for path <- list_notebooks!(config.path) do
+    notebook_paths = list_notebooks!(config.path)
-      log_debug("Deploying notebook: #{path}")
+    log_info("Deploying notebooks:")
+
+    for path <- notebook_paths do
+      log_info(" * Preparing to deploy notebook #{Path.basename(path)}")
       files_dir = Livebook.FileSystem.File.local(path)
 
       with {:ok, content} <- File.read(path),
            {:ok, app_deployment} <- prepare_app_deployment(path, content, files_dir) do
         case Livebook.Teams.deploy_app_from_cli(team, app_deployment, config.deployment_group) do
-          {:ok, url} -> print_deployment(app_deployment, url)
+          {:ok, url} ->
-          {:error, errors} -> raise format_errors(errors)
+            print_text([:green, "  * #{app_deployment.title} deployed successfully. (#{url})"])
-          {:transport_error, reason} -> raise reason
+
+          {:error, errors} ->
+            print_text([:red, "  * #{app_deployment.title} failed to deployed."])
+            errors = normalize_errors(errors)
+
+            raise """
+            #{format_errors(errors, "  * ")}
+
+            #{Teams.Requests.error_message()}\
+            """
+
+          {:transport_error, reason} ->
+            print_text([:red, "  * #{app_deployment.title} failed to deployed."])
+            raise reason
         end
       end
     end
@@ -170,25 +175,6 @@ defmodule LivebookCLI.Deploy do
     end
   end
 
-  defp add_error(errors, key, message) do
-    Map.update(errors, key, [message], &[message | &1])
-  end
-
-  defp format_errors(%{} = errors_map) do
-    errors_map
-    |> Enum.map(fn {key, errors} ->
-      """
-      * #{key}
-      #{format_list(errors)}\
-      """
-    end)
-    |> Enum.join("\n")
-  end
-
-  defp format_list(errors) when is_list(errors) do
-    errors |> Enum.map(&"  * #{&1}") |> Enum.join("\n")
-  end
-
   defp prepare_app_deployment(path, content, files_dir) do
     case Livebook.Teams.AppDeployment.new(content, files_dir) do
       {:ok, app_deployment} ->
@@ -197,7 +183,7 @@ defmodule LivebookCLI.Deploy do
       {:warning, warnings} ->
         raise """
         Deployment for notebook #{Path.basename(path)} failed because the notebook has some warnings:
-        #{format_list(warnings)}
+        #{format_list(warnings, " * ")}
         """
 
       {:error, reason} ->
@@ -205,22 +191,31 @@ defmodule LivebookCLI.Deploy do
     end
   end
 
-  defp print_deployment(app_deployment, url) do
+  defp add_error(errors, key, message) do
-    print_text([
+    Map.update(errors, key, [message], &[message | &1])
-      :green,
+  end
-      "App deployment created successfully.\n\n",
+
-      :magenta,
+  def normalize_errors(%{} = errors) do
-      :bright,
+    for {key, values} <- errors, into: %{} do
-      "Slug: ",
+      {normalize_key(key), values}
-      :reset,
+    end
-      :white,
+  end
-      "#{app_deployment.slug} (#{url})\n",
+
-      :magenta,
+  defp normalize_key(key) when is_atom(key), do: to_string(key) |> normalize_key()
-      :bright,
+  defp normalize_key("session_token"), do: "Deploy Key"
-      "Title: ",
+  defp normalize_key("teams_key"), do: "Teams Key"
-      :reset,
+  defp normalize_key("deployment_group"), do: "Deployment Group"
-      :white,
+  defp normalize_key("path"), do: "Path"
-      app_deployment.title
+
-    ])
+  defp format_errors(errors, prefix) do
+    errors
+    |> Enum.map(fn {key, values} ->
+      values |> Enum.map(&"#{prefix}#{key} #{&1}") |> Enum.join("\n")
+    end)
+    |> Enum.join("\n")
+  end
+
+  defp format_list(errors, prefix) do
+    errors |> Enum.map(&"#{prefix}#{&1}") |> Enum.join("\n")
   end
 end

lib/livebook_cli/task.ex

@@ -18,8 +18,6 @@ defmodule LivebookCLI.Task do
   def call(name, args) do
     task = fetch_task!(name)
     task.call(args)
-
-    :ok
   rescue
     exception -> log_exception(exception, name, __STACKTRACE__)
   end
@@ -31,14 +29,50 @@ defmodule LivebookCLI.Task do
   def usage(name) do
     task = fetch_task!(name)
     print_text(task.usage())
-
-    :ok
   rescue
     exception -> log_exception(exception, name, __STACKTRACE__)
   end
 
-  @spec fetch_task!(String.t()) :: module() | no_return()
   defp fetch_task!("server"), do: LivebookCLI.Server
   defp fetch_task!("deploy"), do: LivebookCLI.Deploy
-  defp fetch_task!(name), do: raise("Unknown command #{name}")
+
+  defp fetch_task!(name) do
+    log_error("Unknown command #{name}")
+    print_text(LivebookCLI.usage())
+
+    System.halt(1)
+  end
+
+  defp log_error(message) do
+    [:red, message]
+    |> IO.ANSI.format()
+    |> IO.puts()
+  end
+
+  @spec log_exception(Exception.t(), String.t(), Exception.stacktrace()) :: no_return()
+  defp log_exception(exception, command_name, stacktrace) when is_exception(exception) do
+    [:red, format_exception(exception, command_name, stacktrace)]
+    |> IO.ANSI.format()
+    |> IO.puts()
+
+    System.halt(1)
+  end
+
+  defp format_exception(%OptionParser.ParseError{} = exception, command_name, _) do
+    """
+    #{Exception.message(exception)}
+
+    For more information try:
+
+       livebook #{command_name} --help
+    """
+  end
+
+  defp format_exception(%RuntimeError{} = exception, _, _) do
+    Exception.message(exception)
+  end
+
+  defp format_exception(exception, _, stacktrace) do
+    Exception.format(:error, exception, stacktrace)
+  end
 end

lib/livebook_cli/utils.ex

@@ -1,18 +1,4 @@
 defmodule LivebookCLI.Utils do
-  def setup do
-    {:ok, _} = Application.ensure_all_started(:elixir)
-
-    extract_priv!()
-
-    :ok = Application.load(:livebook)
-
-    if unix?() do
-      Application.put_env(:elixir, :ansi_enabled, true)
-    end
-  end
-
-  defp unix?(), do: match?({:unix, _}, :os.type())
-
   def option_parse(argv, opts \\ []) do
     {parsed, argv, errors} = OptionParser.parse(argv, opts)
     {Enum.into(parsed, %{}), argv, errors}
@@ -37,7 +23,7 @@ defmodule LivebookCLI.Utils do
   def log_warning(message) do
     [:yellow, message]
     |> IO.ANSI.format()
-    |> IO.warn()
+    |> IO.puts()
   end
 
   def print_text(message) do
@@ -45,68 +31,4 @@ defmodule LivebookCLI.Utils do
     |> IO.ANSI.format()
     |> IO.puts()
   end
-
-  @spec log_exception(Exception.t(), String.t(), Exception.stacktrace()) :: no_return()
-  def log_exception(exception, command_name, stacktrace) when is_exception(exception) do
-    [:red, format_exception(exception, command_name, stacktrace)]
-    |> IO.ANSI.format()
-    |> IO.puts()
-
-    System.halt(1)
-  end
-
-  defp format_exception(%OptionParser.ParseError{} = exception, command_name, _) do
-    """
-    #{Exception.message(exception)}
-
-    For more information try:
-
-       livebook #{command_name} --help
-    """
-  end
-
-  defp format_exception(%RuntimeError{} = exception, _, _) do
-    Exception.message(exception)
-  end
-
-  defp format_exception(exception, _, stacktrace) do
-    Exception.format(:error, exception, stacktrace)
-  end
-
-  import Record
-  defrecord(:zip_file, extract(:zip_file, from_lib: "stdlib/include/zip.hrl"))
-
-  defp extract_priv!() do
-    archive_dir = Path.join(Livebook.Config.tmp_path(), "escript")
-    extracted_path = Path.join(archive_dir, "extracted")
-    in_archive_priv_path = ~c"livebook/priv"
-
-    # In dev we want to extract fresh directory on every boot
-    if Livebook.Config.app_version() =~ "-dev" do
-      File.rm_rf!(archive_dir)
-    end
-
-    # When temporary directory is cleaned by the OS, the directories
-    # may be left in place, so we use a regular file (extracted) to
-    # check if the extracted archive is already available
-    if not File.exists?(extracted_path) do
-      {:ok, sections} = :escript.extract(:escript.script_name(), [])
-      archive = Keyword.fetch!(sections, :archive)
-
-      file_filter = fn zip_file(name: name) ->
-        List.starts_with?(name, in_archive_priv_path)
-      end
-
-      opts = [cwd: String.to_charlist(archive_dir), file_filter: file_filter]
-
-      with {:error, error} <- :zip.extract(archive, opts) do
-        raise "Livebook failed to extract archive files, reason: #{inspect(error)}"
-      end
-
-      File.touch!(extracted_path)
-    end
-
-    priv_dir = Path.join(archive_dir, in_archive_priv_path)
-    Application.put_env(:livebook, :priv_dir, priv_dir, persistent: true)
-  end
 end

test/livebook_teams/cli/deploy_test.exs

@@ -53,9 +53,8 @@ defmodule LivebookCLI.Integration.DeployTest do
                  ]) == :ok
         end)
 
-      assert output =~ "App deployment created successfully."
+      assert output =~ "* Preparing to deploy notebook #{slug}.livemd"
-      assert output =~ "#{slug} (#{@url}/apps/#{slug})"
+      assert output =~ "  * #{title} deployed successfully. (#{@url}/apps/#{slug})"
-      assert output =~ title
 
       assert_receive {:app_deployment_started,
                       %{
@@ -78,7 +77,7 @@ defmodule LivebookCLI.Integration.DeployTest do
         for i <- 1..3 do
           title = "Test App #{i}"
           slug = "app-#{i}-#{Utils.random_short_id()}"
-          app_path = Path.join(tmp_dir, "app_#{i}.livemd")
+          app_path = Path.join(tmp_dir, "#{slug}.livemd")
 
           stamp_notebook(app_path, """
           <!-- livebook:{"app_settings":{"access_type":"public","slug":"#{slug}"},"hub_id":"#{hub_id}"} -->
@@ -107,9 +106,8 @@ defmodule LivebookCLI.Integration.DeployTest do
         end)
 
       for {slug, title} <- apps do
-        assert output =~ "App deployment created successfully."
+        assert output =~ "* Preparing to deploy notebook #{slug}.livemd"
-        assert output =~ "#{slug} (#{@url}/apps/#{slug})"
+        assert output =~ "  * #{title} deployed successfully. (#{@url}/apps/#{slug})"
-        assert output =~ title
 
         assert_receive {:app_deployment_started,
                         %{
@@ -133,7 +131,7 @@ defmodule LivebookCLI.Integration.DeployTest do
       # Test App
       """)
 
-      assert_raise RuntimeError, ~r/Deploy Key.*must be a Livebook Teams Deploy Key/s, fn ->
+      assert_raise RuntimeError, ~r/Deploy Key must be a Livebook Teams Deploy Key/s, fn ->
         Deploy.call([
           "--deploy-key",
           "invalid_key",
@@ -158,7 +156,7 @@ defmodule LivebookCLI.Integration.DeployTest do
       # Test App
       """)
 
-      assert_raise RuntimeError, ~r/Teams Key.*must be a Livebook Teams Key/s, fn ->
+      assert_raise RuntimeError, ~r/Teams Key must be a Livebook Teams Key/s, fn ->
         Deploy.call([
           "--deploy-key",
           key,
@@ -183,7 +181,7 @@ defmodule LivebookCLI.Integration.DeployTest do
       # Test App
       """)
 
-      assert_raise RuntimeError, ~r/Deployment Group.*can't be blank/s, fn ->
+      assert_raise RuntimeError, ~r/Deployment Group can't be blank/s, fn ->
         Deploy.call([
           "--deploy-key",
           key,
@@ -194,11 +192,57 @@ defmodule LivebookCLI.Integration.DeployTest do
       end
     end
 
+    test "fails with invalid deployment group",
+         %{team: team, node: node, org: org, tmp_dir: tmp_dir} do
+      title = "Test CLI Deploy App"
+      slug = Utils.random_short_id()
+      app_path = Path.join(tmp_dir, "#{slug}.livemd")
+      {key, _} = TeamsRPC.create_deploy_key(node, org: org)
+      deployment_group = TeamsRPC.create_deployment_group(node, org: org, url: @url)
+      hub_id = team.id
+      deployment_group_id = to_string(deployment_group.id)
+
+      stamp_notebook(app_path, """
+      <!-- livebook:{"app_settings":{"access_type":"public","slug":"#{slug}"},"hub_id":"#{hub_id}"} -->
+
+      # #{title}
+
+      ## Test Section
+
+      ```elixir
+      IO.puts("Hello from CLI deployed app!")
+      ```
+      """)
+
+      assert_raise RuntimeError, ~r/Deployment Group does not exist/s, fn ->
+        ExUnit.CaptureIO.capture_io(fn ->
+          Deploy.call([
+            "--deploy-key",
+            key,
+            "--teams-key",
+            team.teams_key,
+            "--deployment-group",
+            Utils.random_short_id(),
+            app_path
+          ])
+        end)
+      end
+
+      refute_receive {:app_deployment_started,
+                      %{
+                        title: ^title,
+                        slug: ^slug,
+                        deployment_group_id: ^deployment_group_id,
+                        hub_id: ^hub_id,
+                        deployed_by: "CLI"
+                      }}
+    end
+
     test "fails with non-existent file", %{team: team, node: node, org: org, tmp_dir: tmp_dir} do
       {key, _} = TeamsRPC.create_deploy_key(node, org: org)
       deployment_group = TeamsRPC.create_deployment_group(node, org: org, url: @url)
 
-      assert_raise RuntimeError, ~r/Path.*must be a valid path/s, fn ->
+      assert_raise RuntimeError, ~r/Path must be a valid path/s, fn ->
         Deploy.call([
           "--deploy-key",
           key,

test/livebook_teams/teams_test.exs

@@ -355,7 +355,7 @@ defmodule Livebook.TeamsTest do
                       } = app_deployment2}
 
       assert Teams.deploy_app_from_cli(team, app_deployment, "foo") ==
-               {:error, %{"name" => ["does not exist"]}}
+               {:error, %{"deployment_group" => ["does not exist"]}}
 
       assert Teams.deploy_app_from_cli(team, %{app_deployment | slug: "@abc"}, name) ==
                {:error, %{"slug" => ["should only contain alphanumeric characters and dashes"]}}