Deploy notebooks from CLI - #3034
Conversation
5b3ae00 to
cb6594a
Compare
cb6594a to
ecfa697
Compare
| """ | ||
| end | ||
|
|
||
| defp format_exception(%RuntimeError{} = exception, _, _) do |
There was a problem hiding this comment.
Unfortunately regular Elixir code can also emit RuntimeError, and it would get mixed up with this. If you want this functionality, I would suggest defning a LivebookCLI.Error and raise that instead.
| @deploy_key_prefix Teams.Requests.deploy_key_prefix() | ||
| @teams_key_prefix Teams.Org.teams_key_prefix() |
There was a problem hiding this comment.
It would be nice if we could have these in a single place. What about having a Teams.Prefixes module or Teams.Constants module which only has this stuff? This will also help keep compile cycles small, as those files won't have any logic in them.
| path | ||
| |> File.ls!() | ||
| |> Enum.map(&Path.join(path, &1)) | ||
| |> Enum.reject(&File.dir?/1) | ||
| |> Enum.filter(&String.ends_with?(&1, ".livemd")) |
There was a problem hiding this comment.
| path | |
| |> File.ls!() | |
| |> Enum.map(&Path.join(path, &1)) | |
| |> Enum.reject(&File.dir?/1) | |
| |> Enum.filter(&String.ends_with?(&1, ".livemd")) | |
| path | |
| |> Path.join("*.livemd") | |
| |> Path.wildcard() | |
| |> Enum.filter(&File.regular?/1) |
However, should we deploy only the direct files in a directory or all files recursively? Another option is to not support directories and instead allow a list of files to be given. This way the user can write this:
livebook deploy path/*.livemd
The shell will expand *.livemd into multiple entries and we just deploy all of them. WDYT?
There was a problem hiding this comment.
FTR for LIVEBOOK_APPS_PATH we do path/**/*.livemd:
Lines 244 to 247 in 6806ef8
But passing multiple files sounds good to me.
| if persist_storage? do | ||
| load_or_create_table() | ||
| else | ||
| :ets.new(__MODULE__, [:protected, :duplicate_bag]) |
There was a problem hiding this comment.
Currently we duplicate :ets.new(__MODULE__, [:protected, :duplicate_bag]) in two places. Let's pass persist_storage? to load_or_create_table instead and have the condition there, such that we create the table in a single place only.
| """ | ||
| @spec fetch_cli_session(map()) :: api_result() | ||
| def fetch_cli_session(config) do | ||
| post("/api/v1/cli/auth", %{}, config) |
There was a problem hiding this comment.
We call this with config = %Team{}, do we actually want to send all the fields?
There was a problem hiding this comment.
To call this function, we don't really use the %Team{} struct, we send a map to fetch some data from Teams, so then we can build the %Team{} struct.
| path | ||
| |> File.ls!() | ||
| |> Enum.map(&Path.join(path, &1)) | ||
| |> Enum.reject(&File.dir?/1) | ||
| |> Enum.filter(&String.ends_with?(&1, ".livemd")) |
There was a problem hiding this comment.
FTR for LIVEBOOK_APPS_PATH we do path/**/*.livemd:
Lines 244 to 247 in 6806ef8
But passing multiple files sounds good to me.
| """ | ||
|
|
||
| {:transport_error, reason} -> | ||
| print_text([:red, " * #{app_deployment.title} failed to deploy."]) |
There was a problem hiding this comment.
It feels we are a bit inconsistent. We use print_text([:red, ...]) but we also have log_error. Perhaps we should remove print_text and use either log_error (which would be moved to Utils)orlog_infoorlog_debugorlog_warn`.
josevalim
left a comment
josevalim
left a comment
There was a problem hiding this comment.
I dropped some small nits but otherwise this is good to go!
3 participants
The status feedback message is clunky, but the deploy feature is working.
Kooha-2025-07-24-13-28-02.mp4