Apply review comments

livebook-dev · Nov 18, 2024 · 70f0503 · 70f0503
1 parent 62d0a12
commit 70f0503
Show file tree

Hide file tree

Showing 2 changed files with 13 additions and 36 deletions.
diff --git a/lib/livebook/hubs/dockerfile.ex b/lib/livebook/hubs/dockerfile.ex
@@ -9,16 +9,14 @@ defmodule Livebook.Hubs.Dockerfile do
           deploy_all: boolean(),
           docker_tag: String.t(),
           clustering: nil | :auto | :dns,
-          zta_provider: atom(),
-          environment_variables: nil | %{String.t() => String.t()}
+          environment_variables: list({String.t(), String.t()})
         }
 
   @types %{
     deploy_all: :boolean,
     docker_tag: :string,
     clustering: Ecto.ParameterizedType.init(Ecto.Enum, values: [:auto, :dns]),
-    zta_provider: :atom,
-    environment_variables: :map
+    environment_variables: {:array, :tuple}
   }
 
   @doc """
@@ -32,8 +30,7 @@ defmodule Livebook.Hubs.Dockerfile do
       deploy_all: false,
       docker_tag: default_image.tag,
       clustering: nil,
-      zta_provider: nil,
-      environment_variables: nil
+      environment_variables: []
     }
   end
 
@@ -44,13 +41,11 @@ defmodule Livebook.Hubs.Dockerfile do
   def from_deployment_group(deployment_group) do
     environment_variables =
       for environment_variable <- deployment_group.environment_variables,
-          do: {environment_variable.name, environment_variable.value},
-          into: %{}
+          do: {environment_variable.name, environment_variable.value}
 
     %{
       config_new()
       | clustering: deployment_group.clustering,
-        zta_provider: deployment_group.zta_provider,
         environment_variables: environment_variables
     }
   end
@@ -61,7 +56,7 @@ defmodule Livebook.Hubs.Dockerfile do
   @spec config_changeset(config(), map()) :: Ecto.Changeset.t()
   def config_changeset(config, attrs \\ %{}) do
     {config, @types}
-    |> cast(attrs, [:deploy_all, :docker_tag, :clustering, :zta_provider])
+    |> cast(attrs, [:deploy_all, :docker_tag, :clustering])
     |> validate_required([:deploy_all, :docker_tag])
   end
 
@@ -179,8 +174,8 @@ defmodule Livebook.Hubs.Dockerfile do
       end
 
     environment_variables =
-      if config.environment_variables do
-        envs = config.environment_variables |> Enum.into([]) |> format_envs()
+      if match?([_ | _], config.environment_variables) do
+        envs = config.environment_variables |> Enum.sort() |> format_envs()
 
         """
         # Deployment group environment variables
@@ -356,12 +351,7 @@ defmodule Livebook.Hubs.Dockerfile do
           []
       end
 
-    deployment_group_env =
-      if config.environment_variables do
-        Enum.into(config.environment_variables, [])
-      else
-        []
-      end
+    deployment_group_env = Enum.sort(config.environment_variables)
 
     %{image: image, env: base_image.env ++ env ++ clustering_env ++ deployment_group_env}
   end
@@ -429,7 +419,7 @@ defmodule Livebook.Hubs.Dockerfile do
 
         "team" ->
           [
-            if app_settings.access_type == :public and config.zta_provider != :livebook_teams do
+            if app_settings.access_type == :public do
               "This app has no password configuration and anyone with access to the server will be able" <>
                 " to use it. You may either configure a password or enable authentication with Livebook Teams."
             end

diff --git a/test/livebook/hubs/dockerfile_test.exs b/test/livebook/hubs/dockerfile_test.exs
@@ -208,10 +208,10 @@ defmodule Livebook.Hubs.DockerfileTest do
     test "deploying with deployment group environment variables" do
       config = %{
         dockerfile_config()
-        | environment_variables: %{
-            "LIVEBOOK_IDENTITY_PROVIDER" => "cloudflare:foobar",
-            "LIVEBOOK_TEAMS_URL" => "http://localhost:8000"
-          }
+        | environment_variables: [
+            {"LIVEBOOK_IDENTITY_PROVIDER", "cloudflare:foobar"},
+            {"LIVEBOOK_TEAMS_URL", "http://localhost:8000"}
+          ]
       }
 
       hub = team_hub()
@@ -383,19 +383,6 @@ defmodule Livebook.Hubs.DockerfileTest do
       assert warning =~ "This app has no password configuration"
     end
 
-    test "warns when the app has no password and no ZTA in teams hub" do
-      config = dockerfile_config(%{clustering: :auto})
-      hub = team_hub()
-      app_settings = %{Livebook.Notebook.AppSettings.new() | access_type: :public}
-
-      assert [warning] = Dockerfile.airgapped_warnings(config, hub, [], [], app_settings, [], %{})
-      assert warning =~ "This app has no password configuration"
-
-      config = %{config | zta_provider: :livebook_teams}
-
-      assert [] = Dockerfile.airgapped_warnings(config, hub, [], [], app_settings, [], %{})
-    end
-
     test "warns when no clustering is configured" do
       config = dockerfile_config()
       hub = team_hub()