Apply review comments

This commit is contained in:
Alexandre de Souza 2025-07-24 12:14:50 -03:00
parent 066df147ed
commit 081d84867d
No known key found for this signature in database
GPG key ID: E39228FFBA346545
14 changed files with 161 additions and 176 deletions

View file

@ -94,23 +94,17 @@ defmodule Livebook.Hubs.Team do
changeset
end
end
@doc """
Returns the public key prefix
"""
@spec public_key_prefix() :: String.t()
def public_key_prefix(), do: "lb_opk_"
end
defimpl Livebook.Hubs.Provider, for: Livebook.Hubs.Team do
alias Livebook.Hubs.{Team, TeamClient}
alias Livebook.Teams.Requests
alias Livebook.Teams
alias Livebook.FileSystem
alias Livebook.Secrets.Secret
@teams_key_prefix Livebook.Teams.Org.teams_key_prefix()
@public_key_prefix Team.public_key_prefix()
@deploy_key_prefix Requests.deploy_key_prefix()
@teams_key_prefix Teams.Constants.teams_key_prefix()
@public_key_prefix Teams.Constants.public_key_prefix()
@deploy_key_prefix Teams.Constants.deploy_key_prefix()
def load(team, fields) do
{offline?, fields} = Map.pop(fields, :offline?, false)
@ -162,7 +156,7 @@ defimpl Livebook.Hubs.Provider, for: Livebook.Hubs.Team do
@teams_key_prefix <> teams_key = team.teams_key
token = Livebook.Stamping.chapoly_encrypt(metadata, notebook_source, teams_key)
case Requests.org_sign(team, token) do
case Teams.Requests.org_sign(team, token) do
{:ok, %{"signature" => token_signature}} ->
stamp = %{"version" => 1, "token" => token, "token_signature" => token_signature}
{:ok, stamp}
@ -203,7 +197,7 @@ defimpl Livebook.Hubs.Provider, for: Livebook.Hubs.Team do
def get_secrets(team), do: TeamClient.get_secrets(team.id)
def create_secret(%Team{} = team, %Secret{} = secret) do
case Requests.create_secret(team, secret) do
case Teams.Requests.create_secret(team, secret) do
{:ok, %{"id" => _}} -> :ok
{:error, %{"errors" => errors}} -> {:error, parse_secret_errors(errors)}
any -> any
@ -211,7 +205,7 @@ defimpl Livebook.Hubs.Provider, for: Livebook.Hubs.Team do
end
def update_secret(%Team{} = team, %Secret{} = secret) do
case Requests.update_secret(team, secret) do
case Teams.Requests.update_secret(team, secret) do
{:ok, %{"id" => _}} -> :ok
{:error, %{"errors" => errors}} -> {:error, parse_secret_errors(errors)}
any -> any
@ -219,7 +213,7 @@ defimpl Livebook.Hubs.Provider, for: Livebook.Hubs.Team do
end
def delete_secret(%Team{} = team, %Secret{} = secret) do
case Requests.delete_secret(team, secret) do
case Teams.Requests.delete_secret(team, secret) do
{:ok, _} -> :ok
{:error, %{"errors" => errors}} -> {:error, parse_secret_errors(errors)}
any -> any
@ -229,7 +223,7 @@ defimpl Livebook.Hubs.Provider, for: Livebook.Hubs.Team do
def get_file_systems(team), do: TeamClient.get_file_systems(team.id)
def create_file_system(%Team{} = team, file_system) do
case Requests.create_file_system(team, file_system) do
case Teams.Requests.create_file_system(team, file_system) do
{:ok, %{"id" => _}} -> :ok
{:error, %{"errors" => errors}} -> {:error, parse_file_system_errors(file_system, errors)}
any -> any
@ -237,7 +231,7 @@ defimpl Livebook.Hubs.Provider, for: Livebook.Hubs.Team do
end
def update_file_system(%Team{} = team, file_system) do
case Requests.update_file_system(team, file_system) do
case Teams.Requests.update_file_system(team, file_system) do
{:ok, %{"id" => _}} -> :ok
{:error, %{"errors" => errors}} -> {:error, parse_file_system_errors(file_system, errors)}
any -> any
@ -245,7 +239,7 @@ defimpl Livebook.Hubs.Provider, for: Livebook.Hubs.Team do
end
def delete_file_system(%Team{} = team, file_system) do
case Requests.delete_file_system(team, file_system) do
case Teams.Requests.delete_file_system(team, file_system) do
{:ok, _} -> :ok
{:error, %{"errors" => errors}} -> {:error, parse_file_system_errors(file_system, errors)}
any -> any
@ -266,12 +260,12 @@ defimpl Livebook.Hubs.Provider, for: Livebook.Hubs.Team do
end
defp parse_secret_errors(errors_map) do
Requests.to_error_list(Secret, errors_map)
Teams.Requests.to_error_list(Secret, errors_map)
end
defp parse_file_system_errors(%struct{} = file_system, errors_map) do
%{error_field: field} = FileSystem.external_metadata(file_system)
errors_map = Map.new(errors_map, fn {_key, values} -> {field, values} end)
Requests.to_error_list(struct, errors_map)
Teams.Requests.to_error_list(struct, errors_map)
end
end

View file

@ -187,15 +187,9 @@ defmodule Livebook.Storage do
Process.flag(:trap_exit, true)
persist_storage? = Application.get_env(:livebook, :persist_storage, true)
table =
if persist_storage? do
load_or_create_table()
else
:ets.new(__MODULE__, [:protected, :duplicate_bag])
end
table = load_or_create_table(persist_storage?)
:persistent_term.put(__MODULE__, table)
{:ok, %{table: table, persist?: persist_storage?}}
end
@ -239,7 +233,11 @@ defmodule Livebook.Storage do
defp table_name(), do: :persistent_term.get(__MODULE__)
defp load_or_create_table() do
defp load_or_create_table(false) do
:ets.new(__MODULE__, [:protected, :duplicate_bag])
end
defp load_or_create_table(true) do
tab =
if path = config_file_path_for_restore() do
path

View file

@ -11,7 +11,7 @@ defmodule Livebook.Teams do
import Ecto.Changeset,
only: [add_error: 3, apply_action: 2, apply_action!: 2, get_field: 2]
@prefix Org.teams_key_prefix()
@teams_key_prefix Teams.Constants.teams_key_prefix()
@doc """
Creates an Org.
@ -148,7 +148,7 @@ defmodule Livebook.Teams do
Derives the secret and sign secret from given `teams_key`.
"""
@spec derive_key(String.t()) :: bitstring()
def derive_key(@prefix <> teams_key) do
def derive_key(@teams_key_prefix <> teams_key) do
binary_key = Base.url_decode64!(teams_key, padding: false)
Plug.Crypto.KeyGenerator.generate(binary_key, "notebook secret", cache: Plug.Crypto.Keys)
end
@ -245,9 +245,6 @@ defmodule Livebook.Teams do
id = "team-#{name}"
hub =
if Hubs.hub_exists?(id) do
%{Hubs.fetch_hub!(id) | user_id: nil, session_token: config.session_token}
else
Hubs.save_hub(%Team{
id: id,
hub_name: name,
@ -259,7 +256,6 @@ defmodule Livebook.Teams do
teams_key: config.teams_key,
org_public_key: attrs["public_key"]
})
end
{:ok, hub}
end

View file

@ -0,0 +1,25 @@
defmodule Livebook.Teams.Constants do
@doc """
Returns the public key prefix
"""
@spec public_key_prefix() :: String.t()
def public_key_prefix(), do: "lb_opk_"
@doc """
Returns the Agent Key prefix
"""
@spec agent_key_prefix() :: String.t()
def agent_key_prefix, do: "lb_ak_"
@doc """
Returns the Deploy Key prefix
"""
@spec deploy_key_prefix() :: String.t()
def deploy_key_prefix, do: "lb_dk_"
@doc """
Returns the Teams Key prefix
"""
@spec teams_key_prefix() :: String.t()
def teams_key_prefix(), do: "lb_tk_"
end

View file

@ -2,8 +2,6 @@ defmodule Livebook.Teams.Org do
use Ecto.Schema
import Ecto.Changeset
@prefix "lb_tk_"
@type t :: %__MODULE__{
id: pos_integer() | nil,
emoji: String.t() | nil,
@ -31,7 +29,7 @@ defmodule Livebook.Teams.Org do
@spec teams_key() :: String.t()
def teams_key() do
key = :crypto.strong_rand_bytes(@secret_key_size)
@prefix <> Base.url_encode64(key, padding: false)
Livebook.Teams.Constants.teams_key_prefix() <> Base.url_encode64(key, padding: false)
end
@doc """
@ -50,10 +48,4 @@ defmodule Livebook.Teams.Org do
message: "should only contain lowercase alphanumeric characters and dashes"
)
end
@doc """
Returns the teams key prefix
"""
@spec teams_key_prefix() :: String.t()
def teams_key_prefix(), do: @prefix
end

View file

@ -5,7 +5,7 @@ defmodule Livebook.Teams.Requests do
alias Livebook.Secrets.Secret
alias Livebook.Teams
@deploy_key_prefix "lb_dk_"
@deploy_key_prefix Teams.Constants.deploy_key_prefix()
@error_message "Something went wrong, try again later or please file a bug if it persists"
@unauthorized_error_message "You are not authorized to perform this action, make sure you have the access and you are not in a Livebook App Server/Offline instance"
@ -15,9 +15,6 @@ defmodule Livebook.Teams.Requests do
@doc false
def error_message(), do: @error_message
@doc false
def deploy_key_prefix(), do: @deploy_key_prefix
@doc """
Send a request to Livebook Team API to create a new org.
"""
@ -303,8 +300,6 @@ defmodule Livebook.Teams.Requests do
defp upload(path, content, params, team) do
build_req(team)
|> Req.Request.put_header("content-length", "#{byte_size(content)}")
|> Req.Request.put_private(:cli, path =~ "cli")
|> Req.Request.put_private(:deploy, true)
|> Req.post(url: path, params: params, body: content)
|> handle_response()
|> dispatch_messages(team)

View file

@ -21,7 +21,6 @@ defmodule LivebookCLI do
extract_priv!()
:ok = Application.load(:livebook)
Application.put_env(:livebook, :persist_storage, false)
if unix?() do
Application.put_env(:elixir, :ansi_enabled, true)
@ -30,7 +29,7 @@ defmodule LivebookCLI do
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, arg] when arg in @help_args -> Task.usage(name)
[name | args] -> Task.call(name, List.delete(args, name))
_args -> Utils.print_text(usage())
end

View file

@ -4,8 +4,8 @@ defmodule LivebookCLI.Deploy do
@behaviour LivebookCLI.Task
@deploy_key_prefix Teams.Requests.deploy_key_prefix()
@teams_key_prefix Teams.Org.teams_key_prefix()
@deploy_key_prefix Teams.Constants.deploy_key_prefix()
@teams_key_prefix Teams.Constants.teams_key_prefix()
@impl true
def usage() do
@ -16,7 +16,7 @@ defmodule LivebookCLI.Deploy do
--deploy-key Sets the deploy key to authenticate with Livebook Teams
--teams-key Sets the Teams key to authenticate with Livebook Teams and encrypt the Livebook app
--deployment-group The deployment group name which you want to deploy
--deployment-group The deployment group name which you want to deploy to
The --help option can be given to print this notice.
@ -24,11 +24,11 @@ defmodule LivebookCLI.Deploy do
Deploys a single notebook:
livebook deploy --deploy-key="lb_dk_..." --teams-key="lb_tk_..." -deployment-group "online" path/to/file.livemd
livebook deploy --deploy-key="lb_dk_..." --teams-key="lb_tk_..." --deployment-group "online" path/to/app1.livemd
Deploys a folder:
Deploys multiple notebooks:
livebook deploy --deploy-key="lb_dk_..." --teams-key="lb_tk_..." -deployment-group "online" path/to\
livebook deploy --deploy-key="lb_dk_..." --teams-key="lb_tk_..." --deployment-group "online" path/to/*.livemd\
"""
end
@ -40,6 +40,7 @@ defmodule LivebookCLI.Deploy do
@impl true
def call(args) do
Application.put_env(:livebook, :persist_storage, false)
{:ok, _} = Application.ensure_all_started(:livebook)
config = config_from_args(args)
ensure_config!(config)
@ -49,11 +50,10 @@ defmodule LivebookCLI.Deploy do
end
defp config_from_args(args) do
{opts, filename_or_directory} = OptionParser.parse!(args, strict: @switches)
filename_or_directory = Path.expand(filename_or_directory)
{opts, path} = OptionParser.parse!(args, strict: @switches)
%{
path: filename_or_directory,
path: List.flatten(path),
session_token: opts[:deploy_key],
teams_key: opts[:teams_key],
deployment_group: opts[:deployment_group]
@ -83,11 +83,7 @@ defmodule LivebookCLI.Deploy do
end
{:path, value}, acc ->
if not File.exists?(value) do
add_error(acc, normalize_key(:path), "must be a valid path")
else
acc
end
Enum.reduce_while(value, acc, &validate_path/2)
_otherwise, acc ->
acc
@ -96,7 +92,7 @@ defmodule LivebookCLI.Deploy do
if Map.keys(errors) == [] do
:ok
else
raise """
raise LivebookCLI.Error, """
You configuration is invalid, make sure you are using the correct options for this task.
#{format_errors(errors, " * ")}\
@ -104,21 +100,39 @@ defmodule LivebookCLI.Deploy do
end
end
defp validate_path(value, acc) do
cond do
not File.exists?(value) ->
{:halt, add_error(acc, normalize_key(:path), "must be a valid path")}
File.dir?(value) ->
{:halt, add_error(acc, normalize_key(:path), "must be a file path")}
true ->
{:cont, acc}
end
end
defp authenticate_cli!(config) do
log_debug("Authenticating CLI...")
case Teams.fetch_cli_session(config) do
{:ok, team} -> team
{:error, error} -> raise error
{:transport_error, error} -> raise error
{:error, error} -> raise LivebookCLI.Error, error
{:transport_error, error} -> raise LivebookCLI.Error, error
end
end
defp deploy_to_teams(team, config) do
notebook_paths = list_notebooks!(config.path)
if length(config.path) == 1 do
log_debug("Found 1 notebook")
else
log_debug("Found #{length(config.path)} notebooks")
end
log_info("Deploying notebooks:")
for path <- notebook_paths do
for path <- config.path do
log_info(" * Preparing to deploy notebook #{Path.basename(path)}")
files_dir = Livebook.FileSystem.File.local(path)
@ -129,18 +143,18 @@ defmodule LivebookCLI.Deploy do
print_text([:green, " * #{app_deployment.title} deployed successfully. (#{url})"])
{:error, errors} ->
print_text([:red, " * #{app_deployment.title} failed to deployed."])
print_text([:red, " * #{app_deployment.title} failed to deploy."])
errors = normalize_errors(errors)
raise """
raise LivebookCLI.Error, """
#{format_errors(errors, " * ")}
#{Teams.Requests.error_message()}\
"""
{:transport_error, reason} ->
print_text([:red, " * #{app_deployment.title} failed to deployed."])
raise reason
print_text([:red, " * #{app_deployment.title} failed to deploy."])
raise LivebookCLI.Error, reason
end
end
end
@ -148,46 +162,19 @@ defmodule LivebookCLI.Deploy do
:ok
end
defp list_notebooks!(path) do
log_debug("Listing notebooks from: #{path}")
files =
if File.dir?(path) do
path
|> File.ls!()
|> Enum.map(&Path.join(path, &1))
|> Enum.reject(&File.dir?/1)
|> Enum.filter(&String.ends_with?(&1, ".livemd"))
else
[path]
end
if files == [] do
raise "There's no notebook available to deploy"
else
if length(files) == 1 do
log_debug("Found 1 notebook")
else
log_debug("Found #{length(files)} notebooks")
end
files
end
end
defp prepare_app_deployment(path, content, files_dir) do
case Livebook.Teams.AppDeployment.new(content, files_dir) do
{:ok, app_deployment} ->
{:ok, app_deployment}
{:warning, warnings} ->
raise """
raise LivebookCLI.Error, """
Deployment for notebook #{Path.basename(path)} failed because the notebook has some warnings:
#{format_list(warnings, " * ")}
"""
{:error, reason} ->
raise "Failed to handle I/O operations: #{reason}"
raise LivebookCLI.Error, "Failed to handle I/O operations: #{reason}"
end
end

View file

@ -0,0 +1,3 @@
defmodule LivebookCLI.Error do
defexception [:message]
end

View file

@ -88,7 +88,8 @@ defmodule LivebookCLI.Server do
open_from_args(base_url, extra_args)
:taken ->
raise "Another application is already running on port #{port}." <>
raise LivebookCLI.Error,
"Another application is already running on port #{port}." <>
" Either ensure this port is free or specify a different port using the --port option"
:available ->
@ -106,7 +107,7 @@ defmodule LivebookCLI.Server do
Process.sleep(:infinity)
{:error, error} ->
raise "Livebook failed to start with reason: #{inspect(error)}"
raise LivebookCLI.Error, "Livebook failed to start with reason: #{inspect(error)}"
end
end
@ -174,7 +175,9 @@ defmodule LivebookCLI.Server do
end
defp open_from_args(_base_url, _extra_args) do
raise "Too many arguments entered. Ensure only one argument is used to specify the file path and all other arguments are preceded by the relevant switch"
raise OptionParser.ParseError,
"Too many arguments entered. Ensure only one argument is used to" <>
"specify the file path and all other arguments are preceded by the relevant switch"
end
@switches [

View file

@ -68,8 +68,14 @@ defmodule LivebookCLI.Task do
"""
end
defp format_exception(%RuntimeError{} = exception, _, _) do
Exception.message(exception)
defp format_exception(%LivebookCLI.Error{} = exception, command_name, _) do
"""
#{Exception.message(exception)}
For more information try:
livebook #{command_name} --help
"""
end
defp format_exception(exception, _, stacktrace) do

View file

@ -4,7 +4,6 @@ defmodule LivebookCLI.Integration.DeployTest do
import Livebook.AppHelpers
alias Livebook.Utils
alias LivebookCLI.Deploy
@url "http://localhost:4200"
@ -15,7 +14,6 @@ defmodule LivebookCLI.Integration.DeployTest do
@moduletag subscribe_to_teams_topics: [:clients, :deployment_groups, :app_deployments]
@moduletag :tmp_dir
@moduletag :capture_io
describe "CLI deploy integration" do
test "successfully deploys a notebook via CLI",
@ -42,15 +40,12 @@ defmodule LivebookCLI.Integration.DeployTest do
output =
ExUnit.CaptureIO.capture_io(fn ->
assert Deploy.call([
"--deploy-key",
assert deploy(
key,
"--teams-key",
team.teams_key,
"--deployment-group",
deployment_group.name,
app_path
]) == :ok
) == :ok
end)
assert output =~ "* Preparing to deploy notebook #{slug}.livemd"
@ -94,15 +89,12 @@ defmodule LivebookCLI.Integration.DeployTest do
output =
ExUnit.CaptureIO.capture_io(fn ->
assert Deploy.call([
"--deploy-key",
assert deploy(
key,
"--teams-key",
team.teams_key,
"--deployment-group",
deployment_group.name,
tmp_dir
]) == :ok
Path.join(tmp_dir, "*.livemd")
) == :ok
end)
for {slug, title} <- apps do
@ -131,16 +123,13 @@ defmodule LivebookCLI.Integration.DeployTest do
# Test App
""")
assert_raise RuntimeError, ~r/Deploy Key must be a Livebook Teams Deploy Key/s, fn ->
Deploy.call([
"--deploy-key",
assert_raise LivebookCLI.Error, ~r/Deploy Key must be a Livebook Teams Deploy Key/s, fn ->
deploy(
"invalid_key",
"--teams-key",
team.teams_key,
"--deployment-group",
deployment_group.name,
app_path
])
)
end
end
@ -156,16 +145,13 @@ defmodule LivebookCLI.Integration.DeployTest do
# Test App
""")
assert_raise RuntimeError, ~r/Teams Key must be a Livebook Teams Key/s, fn ->
Deploy.call([
"--deploy-key",
assert_raise LivebookCLI.Error, ~r/Teams Key must be a Livebook Teams Key/s, fn ->
deploy(
key,
"--teams-key",
"invalid-key",
"--deployment-group",
deployment_group.name,
app_path
])
)
end
end
@ -181,14 +167,13 @@ defmodule LivebookCLI.Integration.DeployTest do
# Test App
""")
assert_raise RuntimeError, ~r/Deployment Group can't be blank/s, fn ->
Deploy.call([
"--deploy-key",
assert_raise LivebookCLI.Error, ~r/Deployment Group can't be blank/s, fn ->
deploy(
key,
"--teams-key",
team.teams_key,
"",
app_path
])
)
end
end
@ -214,17 +199,14 @@ defmodule LivebookCLI.Integration.DeployTest do
```
""")
assert_raise RuntimeError, ~r/Deployment Group does not exist/s, fn ->
assert_raise LivebookCLI.Error, ~r/Deployment Group does not exist/s, fn ->
ExUnit.CaptureIO.capture_io(fn ->
Deploy.call([
"--deploy-key",
deploy(
key,
"--teams-key",
team.teams_key,
"--deployment-group",
Utils.random_short_id(),
app_path
])
)
end)
end
@ -242,38 +224,42 @@ defmodule LivebookCLI.Integration.DeployTest 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 ->
Deploy.call([
"--deploy-key",
assert_raise LivebookCLI.Error, ~r/Path must be a valid path/s, fn ->
deploy(
key,
"--teams-key",
team.teams_key,
"--deployment-group",
deployment_group.name,
Path.join(tmp_dir, "app.livemd")
])
)
end
end
test "fails when directory contains no notebooks",
%{team: team, node: node, org: org, tmp_dir: tmp_dir} do
test "fails with directory argument", %{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)
File.write!(Path.join(tmp_dir, "readme.txt"), "No notebooks here")
File.write!(Path.join(tmp_dir, "config.json"), "{}")
assert_raise RuntimeError, ~r/There's no notebook available to deploy/, fn ->
Deploy.call([
"--deploy-key",
assert_raise LivebookCLI.Error, ~r/Path must be a file path/s, fn ->
deploy(
key,
"--teams-key",
team.teams_key,
"--deployment-group",
deployment_group.name,
tmp_dir
)
end
end
end
defp deploy(deploy_key, teams_key, deployment_group_name, path) do
path = if Path.wildcard(path) == [], do: path, else: Path.wildcard(path)
LivebookCLI.Deploy.call([
"--deploy-key",
deploy_key,
"--teams-key",
teams_key,
"--deployment-group",
deployment_group_name,
path
])
end
end
end
end

View file

@ -258,6 +258,7 @@ defmodule Livebook.TeamsTest do
test "authenticates the deploy key", %{team: team} do
config = %{teams_key: team.teams_key, session_token: team.session_token}
refute Livebook.Hubs.hub_exists?(team.id)
assert Teams.fetch_cli_session(config) == {:ok, team}
assert Livebook.Hubs.hub_exists?(team.id)
end
@ -272,7 +273,7 @@ defmodule Livebook.TeamsTest do
{:ok,
%Livebook.Hubs.Team{
billing_status: team.billing_status,
hub_emoji: team.hub_emoji,
hub_emoji: "🚀",
hub_name: team.hub_name,
id: team.id,
offline: nil,
@ -284,9 +285,8 @@ defmodule Livebook.TeamsTest do
user_id: nil
}}
# If the hub already exist, we don't update them from storage
assert Livebook.Hubs.hub_exists?(team.id)
refute Livebook.Hubs.fetch_hub!(team.id).session_token == key
assert Livebook.Hubs.fetch_hub!(team.id).session_token == key
end
@tag teams_persisted: false

View file

@ -21,7 +21,8 @@ defmodule Livebook.Factory do
org_id: 1,
user_id: 1,
org_key_id: 1,
org_public_key: Livebook.Hubs.Team.public_key_prefix() <> Livebook.Utils.random_long_id(),
org_public_key:
Livebook.Teams.Constants.public_key_prefix() <> Livebook.Utils.random_long_id(),
teams_key: org.teams_key,
session_token: Livebook.Utils.random_short_id(),
offline: nil