From 59deaa5329ae4cefe7651e83607714ecca193027 Mon Sep 17 00:00:00 2001 From: Alexandre de Souza Date: Fri, 12 Apr 2024 10:36:16 -0300 Subject: [PATCH] Send more fields to Teams API when deploying an app (#2563) --- lib/livebook/teams.ex | 30 ++++++++++++------- lib/livebook/teams/requests.ex | 7 +++-- .../live/session_live/app_teams_component.ex | 13 ++++---- test/livebook_teams/hubs/team_client_test.exs | 7 +++-- test/livebook_teams/teams/connection_test.exs | 10 ++++--- test/livebook_teams/teams_test.exs | 20 +++++++++++-- .../web/hub/deployment_group_test.exs | 9 +++--- 7 files changed, 64 insertions(+), 32 deletions(-) diff --git a/lib/livebook/teams.ex b/lib/livebook/teams.ex index 95d1c7c4c..320957de9 100644 --- a/lib/livebook/teams.ex +++ b/lib/livebook/teams.ex @@ -4,6 +4,7 @@ defmodule Livebook.Teams do alias Livebook.Hubs alias Livebook.Hubs.Team alias Livebook.Hubs.TeamClient + alias Livebook.Notebook.AppSettings alias Livebook.Teams.{Agent, AppDeployment, DeploymentGroup, Org, Requests} import Ecto.Changeset, @@ -49,13 +50,9 @@ defmodule Livebook.Teams do {:error, %Ecto.Changeset{} = changeset} -> {:error, changeset} - {:error, %{"errors" => errors_map}} -> - errors_map = - if errors = errors_map["key_hash"], - do: Map.put_new(errors_map, "teams_key", errors), - else: errors_map - - {:error, add_org_errors(changeset, errors_map)} + {:error, %{"errors" => errors}} -> + errors = map_teams_field_to_livebook_field(errors, "key_hash", "teams_key") + {:error, add_org_errors(changeset, errors)} any -> any @@ -194,12 +191,12 @@ defmodule Livebook.Teams do @doc """ Creates a new app deployment. """ - @spec deploy_app(Team.t(), AppDeployment.t()) :: + @spec deploy_app(Team.t(), AppDeployment.t(), AppSettings.t()) :: :ok | {:error, Ecto.Changeset.t()} | {:transport_error, String.t()} - def deploy_app(%Team{} = team, %AppDeployment{} = app_deployment) do - case Requests.deploy_app(team, app_deployment) do + def deploy_app(%Team{} = team, %AppDeployment{} = app_deployment, %AppSettings{} = app_settings) do + case Requests.deploy_app(team, app_deployment, app_settings) do {:ok, %{"id" => _id}} -> :ok @@ -207,6 +204,11 @@ defmodule Livebook.Teams do {:error, Requests.add_errors(app_deployment, %{"file" => [error]})} {:error, %{"errors" => errors}} -> + errors = + errors + |> map_teams_field_to_livebook_field("multi_session", "file") + |> map_teams_field_to_livebook_field("access_type", "file") + {:error, Requests.add_errors(app_deployment, errors)} any -> @@ -229,4 +231,12 @@ defmodule Livebook.Teams do def get_agents(team) do TeamClient.get_agents(team.id) end + + defp map_teams_field_to_livebook_field(map, teams_field, livebook_field) do + if value = map[teams_field] do + Map.put_new(map, livebook_field, value) + else + map + end + end end diff --git a/lib/livebook/teams/requests.ex b/lib/livebook/teams/requests.ex index 32b89dabc..d2e12b937 100644 --- a/lib/livebook/teams/requests.ex +++ b/lib/livebook/teams/requests.ex @@ -2,6 +2,7 @@ defmodule Livebook.Teams.Requests do alias Livebook.FileSystem alias Livebook.FileSystems alias Livebook.Hubs.Team + alias Livebook.Notebook.AppSettings alias Livebook.Secrets.Secret alias Livebook.Teams alias Livebook.Teams.{AppDeployment, DeploymentGroup, Org} @@ -183,14 +184,16 @@ defmodule Livebook.Teams.Requests do @doc """ Send a request to Livebook Team API to deploy an app. """ - @spec deploy_app(Team.t(), AppDeployment.t()) :: + @spec deploy_app(Team.t(), AppDeployment.t(), AppSettings.t()) :: {:ok, map()} | {:error, map() | String.t()} | {:transport_error, String.t()} - def deploy_app(team, app_deployment) do + def deploy_app(team, app_deployment, app_settings) do secret_key = Teams.derive_key(team.teams_key) params = %{ title: app_deployment.title, slug: app_deployment.slug, + multi_session: app_settings.multi_session, + access_type: app_settings.access_type, deployment_group_id: app_deployment.deployment_group_id, sha: app_deployment.sha } diff --git a/lib/livebook_web/live/session_live/app_teams_component.ex b/lib/livebook_web/live/session_live/app_teams_component.ex index cb2fc0e94..a86fe7b81 100644 --- a/lib/livebook_web/live/session_live/app_teams_component.ex +++ b/lib/livebook_web/live/session_live/app_teams_component.ex @@ -165,8 +165,10 @@ defmodule LivebookWeb.SessionLive.AppTeamsComponent do end def handle_event("deploy_app", _, socket) do - with {:ok, app_deployment} <- pack_app(socket), - :ok <- deploy_app(socket, app_deployment) do + notebook = Livebook.Session.get_notebook(socket.assigns.session.pid) + + with {:ok, app_deployment} <- pack_app(socket, notebook), + :ok <- deploy_app(socket, app_deployment, notebook.app_settings) do message = "App deployment for #{app_deployment.slug} with title #{app_deployment.title} created successfully" @@ -174,8 +176,7 @@ defmodule LivebookWeb.SessionLive.AppTeamsComponent do end end - defp pack_app(socket) do - notebook = Livebook.Session.get_notebook(socket.assigns.session.pid) + defp pack_app(socket, notebook) do files_dir = socket.assigns.session.files_dir case Livebook.Teams.AppDeployment.new(notebook, files_dir) do @@ -192,8 +193,8 @@ defmodule LivebookWeb.SessionLive.AppTeamsComponent do end end - defp deploy_app(socket, app_deployment) do - case Livebook.Teams.deploy_app(socket.assigns.hub, app_deployment) do + defp deploy_app(socket, app_deployment, app_settings) do + case Livebook.Teams.deploy_app(socket.assigns.hub, app_deployment, app_settings) do :ok -> :ok diff --git a/test/livebook_teams/hubs/team_client_test.exs b/test/livebook_teams/hubs/team_client_test.exs index a5aa12956..7baac3c74 100644 --- a/test/livebook_teams/hubs/team_client_test.exs +++ b/test/livebook_teams/hubs/team_client_test.exs @@ -121,10 +121,11 @@ defmodule Livebook.Hubs.TeamClientTest do # creates the app deployment slug = Livebook.Utils.random_short_id() title = "MyNotebook-#{slug}" + app_settings = %{Livebook.Notebook.AppSettings.new() | slug: slug} notebook = %{ Livebook.Notebook.new() - | app_settings: %{Livebook.Notebook.AppSettings.new() | slug: slug}, + | app_settings: app_settings, name: title, hub_id: team.id, deployment_group_id: id @@ -132,7 +133,7 @@ defmodule Livebook.Hubs.TeamClientTest do files_dir = Livebook.FileSystem.File.local(tmp_dir) {:ok, app_deployment} = Livebook.Teams.AppDeployment.new(notebook, files_dir) - :ok = Livebook.Teams.deploy_app(team, app_deployment) + :ok = Livebook.Teams.deploy_app(team, app_deployment, app_settings) sha = app_deployment.sha @@ -749,7 +750,7 @@ defmodule Livebook.Hubs.TeamClientTest do # Since the app deployment struct generation is from Livebook side, # we don't have yet the information about who deployed the app, - # so we need to add it ourselves + # so we need to add it ourselves. app_deployment = %{ app_deployment | id: to_string(teams_app_deployment.id), diff --git a/test/livebook_teams/teams/connection_test.exs b/test/livebook_teams/teams/connection_test.exs index a8a57a587..7b2a96c62 100644 --- a/test/livebook_teams/teams/connection_test.exs +++ b/test/livebook_teams/teams/connection_test.exs @@ -119,10 +119,11 @@ defmodule Livebook.Teams.ConnectionTest do deployment_group_id = to_string(id) slug = Livebook.Utils.random_short_id() title = "MyNotebook3-#{slug}" + app_settings = %{Livebook.Notebook.AppSettings.new() | slug: slug} notebook = %{ Livebook.Notebook.new() - | app_settings: %{Livebook.Notebook.AppSettings.new() | slug: slug}, + | app_settings: app_settings, name: title, hub_id: hub.id, deployment_group_id: deployment_group_id @@ -135,7 +136,7 @@ defmodule Livebook.Teams.ConnectionTest do # since we want to fetch the app deployment from connection event, # we need to persist it before we connect to the WebSocket - :ok = Livebook.Teams.deploy_app(hub, app_deployment) + :ok = Livebook.Teams.deploy_app(hub, app_deployment, app_settings) assert {:ok, _conn} = Connection.start_link(self(), headers) assert_receive :connected @@ -163,10 +164,11 @@ defmodule Livebook.Teams.ConnectionTest do # creates a new app deployment slug = Livebook.Utils.random_short_id() title = "MyNotebook3-#{slug}" + app_settings = %{Livebook.Notebook.AppSettings.new() | slug: slug} notebook = %{ Livebook.Notebook.new() - | app_settings: %{Livebook.Notebook.AppSettings.new() | slug: slug}, + | app_settings: app_settings, name: title, hub_id: hub.id, deployment_group_id: to_string(id) @@ -179,7 +181,7 @@ defmodule Livebook.Teams.ConnectionTest do # since we want to fetch the app deployment from connection event, # we need to persist it before we connect to the WebSocket - :ok = Livebook.Teams.deploy_app(hub, app_deployment) + :ok = Livebook.Teams.deploy_app(hub, app_deployment, app_settings) # As we need to be Agent to receive the app deployments list to be deployed, # we will create another connection here diff --git a/test/livebook_teams/teams_test.exs b/test/livebook_teams/teams_test.exs index f373a3076..7b03aac4f 100644 --- a/test/livebook_teams/teams_test.exs +++ b/test/livebook_teams/teams_test.exs @@ -196,9 +196,11 @@ defmodule Livebook.TeamsTest do {:ok, id} = Teams.create_deployment_group(team, deployment_group) + app_settings = %{Notebook.AppSettings.new() | slug: Utils.random_short_id()} + notebook = %{ Notebook.new() - | app_settings: %{Notebook.AppSettings.new() | slug: Utils.random_short_id()}, + | app_settings: app_settings, name: "MyNotebook", hub_id: team.id, deployment_group_id: to_string(id) @@ -207,7 +209,21 @@ defmodule Livebook.TeamsTest do files_dir = Livebook.FileSystem.File.local(tmp_dir) assert {:ok, app_deployment} = Teams.AppDeployment.new(notebook, files_dir) - assert Teams.deploy_app(team, app_deployment) == :ok + assert Teams.deploy_app(team, app_deployment, app_settings) == :ok + + assert {:error, + %{errors: [slug: {"should only contain alphanumeric characters and dashes", []}]}} = + Teams.deploy_app(team, %{app_deployment | slug: "@abc"}, app_settings) + + # Since the fields below belongs to AppSettings, we're mapping the errors to `:file` field. + assert {:error, %{errors: [file: {"can't be blank", []}]}} = + Teams.deploy_app(team, app_deployment, %{app_settings | multi_session: nil}) + + assert {:error, %{errors: [file: {"can't be blank", []}]}} = + Teams.deploy_app(team, app_deployment, %{app_settings | access_type: nil}) + + assert {:error, %{errors: [file: {"is invalid", []}]}} = + Teams.deploy_app(team, app_deployment, %{app_settings | access_type: :abc}) end end end diff --git a/test/livebook_teams/web/hub/deployment_group_test.exs b/test/livebook_teams/web/hub/deployment_group_test.exs index 2b1af0838..22f7ffae4 100644 --- a/test/livebook_teams/web/hub/deployment_group_test.exs +++ b/test/livebook_teams/web/hub/deployment_group_test.exs @@ -267,12 +267,11 @@ defmodule LivebookWeb.Integration.Hub.DeploymentGroupTest do |> Floki.text() |> String.trim() == "0" + app_settings = %{Livebook.Notebook.AppSettings.new() | slug: Livebook.Utils.random_short_id()} + notebook = %{ Livebook.Notebook.new() - | app_settings: %{ - Livebook.Notebook.AppSettings.new() - | slug: Livebook.Utils.random_short_id() - }, + | app_settings: app_settings, name: "MyNotebook", hub_id: hub.id, deployment_group_id: to_string(id) @@ -281,7 +280,7 @@ defmodule LivebookWeb.Integration.Hub.DeploymentGroupTest do files_dir = Livebook.FileSystem.File.local(tmp_dir) {:ok, app_deployment} = Livebook.Teams.AppDeployment.new(notebook, files_dir) - :ok = Livebook.Teams.deploy_app(hub, app_deployment) + :ok = Livebook.Teams.deploy_app(hub, app_deployment, app_settings) assert_receive {:app_deployment_started, _}