Repository: supabase/cli (go)

Issue:
Config.toml env variable replacement fails
# Bug report

## Describe the bug

When trying to run `supabase start/stop` using CLI versions [`0.21.0`](https://github.com/supabase/cli/releases/tag/v0.21.0) or above, an error is shown if an env variable is used in the `config.toml`.
This used to work with `0.18.2`.

```sh
❯ source .env && supabase stop
Error: Error evaluating "env(GITHUB_CLIENT_ID)": environment variable GITHUB_CLIENT_ID is unset.
❯ echo $GITHUB_CLIENT_ID
some-client-id
```

**Config**
```toml
# other sections omitted
[auth.external.github]
enabled = true
client_id = "env(GITHUB_CLIENT_ID)"
secret = "env(GITHUB_SECRET)"
```

Let me know if more details are necessary.
Thanks!

## To Reproduce

1. Add the above config snippet to a Supabase project
2. Create a `.env` file with `GITHUB_CLIENT_ID=foo` as contents
4. run `source .env && supabase start`
6. See error

## Expected behavior

The env variables should be interpolated into the `config.toml`.
Ideally:
- misconfigurations in the TOML file shouldn't prevent stopping the local Docker containers
- env variable substitution should work throughout the config, and not just for auth providers

## System information

- OS: macOS 12.3
- Version of supabase-cli: `v0.22.4`
- Version of Node.js: `v16.13.0`


Pull request description:
fix(config): interpolate env on load always
Fixes #210 

Patch:
diff --git a/internal/start/start.go b/internal/start/start.go
index 4be3111e..70cea416 100644
--- a/internal/start/start.go
+++ b/internal/start/start.go
@@ -38,9 +38,6 @@ func Run() error {
 		if err := utils.LoadConfig(); err != nil {
 			return err
 		}
-		if err := utils.InterpolateEnvInConfig(); err != nil {
-			return err
-		}
 		if err := utils.AssertSupabaseStartIsRunning(); err == nil {
 			return errors.New(utils.Aqua("supabase start") + " is already running. Try running " + utils.Aqua("supabase stop") + " first.")
 		}
diff --git a/internal/utils/config.go b/internal/utils/config.go
index 26d8d938..c6b71b98 100644
--- a/internal/utils/config.go
+++ b/internal/utils/config.go
@@ -228,56 +228,48 @@ func LoadConfigFS(fsys afero.Fs) error {
 					ClientId: "",
 					Secret:   "",
 				}
-			}
-		}
-	}
-
-	return nil
-}
-
-func InterpolateEnvInConfig() error {
-	maybeLoadEnv := func(s string) (string, error) {
-		matches := regexp.MustCompile(`^env\((.*)\)$`).FindStringSubmatch(s)
-		if len(matches) == 0 {
-			return s, nil
-		}
-
-		envName := matches[1]
-		value := os.Getenv(envName)
-		if value == "" {
-			return "", errors.New(`Error evaluating "env(` + envName + `)": environment variable ` + envName + " is unset.")
-		}
-
-		return value, nil
-	}
+			} else if Config.Auth.External[ext].Enabled {
+				maybeLoadEnv := func(s string) (string, error) {
+					matches := regexp.MustCompile(`^env\((.*)\)$`).FindStringSubmatch(s)
+					if len(matches) == 0 {
+						return s, nil
+					}
+
+					envName := matches[1]
+					value := os.Getenv(envName)
+					if value == "" {
+						return "", errors.New(`Error evaluating "env(` + envName + `)": environment variable ` + envName + " is unset.")
+					}
+
+					return value, nil
+				}
 
-	for _, ext := range authExternalProviders {
-		if Config.Auth.External[ext].Enabled {
-			var clientId, secret string
+				var clientId, secret string
 
-			if Config.Auth.External[ext].ClientId == "" {
-				return fmt.Errorf("Missing required field in config: auth.external.%s.client_id", ext)
-			} else {
-				v, err := maybeLoadEnv(Config.Auth.External[ext].ClientId)
-				if err != nil {
-					return err
+				if Config.Auth.External[ext].ClientId == "" {
+					return fmt.Errorf("Missing required field in config: auth.external.%s.client_id", ext)
+				} else {
+					v, err := maybeLoadEnv(Config.Auth.External[ext].ClientId)
+					if err != nil {
+						return err
+					}
+					clientId = v
 				}
-				clientId = v
-			}
-			if Config.Auth.External[ext].Secret == "" {
-				return fmt.Errorf("Missing required field in config: auth.external.%s.secret", ext)
-			} else {
-				v, err := maybeLoadEnv(Config.Auth.External[ext].Secret)
-				if err != nil {
-					return err
+				if Config.Auth.External[ext].Secret == "" {
+					return fmt.Errorf("Missing required field in config: auth.external.%s.secret", ext)
+				} else {
+					v, err := maybeLoadEnv(Config.Auth.External[ext].Secret)
+					if err != nil {
+						return err
+					}
+					secret = v
 				}
-				secret = v
-			}
 
-			Config.Auth.External[ext] = provider{
-				Enabled:  true,
-				ClientId: clientId,
-				Secret:   secret,
+				Config.Auth.External[ext] = provider{
+					Enabled:  true,
+					ClientId: clientId,
+					Secret:   secret,
+				}
 			}
 		}
 	}


Test patch:
diff --git a/internal/db/remote/set/set_test.go b/internal/db/remote/set/set_test.go
index e5633c81..5475f567 100644
--- a/internal/db/remote/set/set_test.go
+++ b/internal/db/remote/set/set_test.go
@@ -17,7 +17,7 @@ func TestDbRemoteSetCommand(t *testing.T) {
 	t.Run("sets the remote database url", func(t *testing.T) {
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
-		require.NoError(t, utils.WriteConfig(fsys, true))
+		require.NoError(t, utils.WriteConfig(fsys, false))
 		// Setup initial migration
 		version := "20220727064247"
 		_, err := fsys.Create("supabase/migrations/" + version + "_init.sql")
@@ -36,7 +36,7 @@ func TestDbRemoteSetCommand(t *testing.T) {
 	t.Run("creates migrations table if absent", func(t *testing.T) {
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
-		require.NoError(t, utils.WriteConfig(fsys, true))
+		require.NoError(t, utils.WriteConfig(fsys, false))
 		// Setup mock postgres
 		conn := pgtest.NewConn()
 		defer conn.Close(t)
@@ -61,7 +61,7 @@ func TestDbRemoteSetCommand(t *testing.T) {
 	t.Run("throws error on invalid postgres url", func(t *testing.T) {
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
-		require.NoError(t, utils.WriteConfig(fsys, true))
+		require.NoError(t, utils.WriteConfig(fsys, false))
 		// Run test
 		assert.Error(t, Run("invalid", fsys))
 	})
@@ -69,7 +69,7 @@ func TestDbRemoteSetCommand(t *testing.T) {
 	t.Run("throws error on failture to connect", func(t *testing.T) {
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
-		require.NoError(t, utils.WriteConfig(fsys, true))
+		require.NoError(t, utils.WriteConfig(fsys, false))
 		// Run test
 		assert.Error(t, Run(postgresUrl, fsys))
 	})
@@ -77,7 +77,7 @@ func TestDbRemoteSetCommand(t *testing.T) {
 	t.Run("throws error on missing server version", func(t *testing.T) {
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
-		require.NoError(t, utils.WriteConfig(fsys, true))
+		require.NoError(t, utils.WriteConfig(fsys, false))
 		// Setup mock postgres
 		conn := pgtest.NewWithStatus(map[string]string{
 			"standard_conforming_strings": "on",
@@ -90,7 +90,7 @@ func TestDbRemoteSetCommand(t *testing.T) {
 	t.Run("throws error on unsupported server version", func(t *testing.T) {
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
-		require.NoError(t, utils.WriteConfig(fsys, true))
+		require.NoError(t, utils.WriteConfig(fsys, false))
 		// Setup mock postgres
 		conn := pgtest.NewWithStatus(map[string]string{
 			"standard_conforming_strings": "on",
@@ -104,7 +104,7 @@ func TestDbRemoteSetCommand(t *testing.T) {
 	t.Run("throws error on failure to create table", func(t *testing.T) {
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
-		require.NoError(t, utils.WriteConfig(fsys, true))
+		require.NoError(t, utils.WriteConfig(fsys, false))
 		// Setup mock postgres
 		conn := pgtest.NewConn()
 		defer conn.Close(t)
@@ -119,7 +119,7 @@ func TestDbRemoteSetCommand(t *testing.T) {
 	t.Run("throws error on failure to list migrations", func(t *testing.T) {
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
-		require.NoError(t, utils.WriteConfig(fsys, true))
+		require.NoError(t, utils.WriteConfig(fsys, false))
 		// Setup mock postgres
 		conn := pgtest.NewConn()
 		defer conn.Close(t)
@@ -134,7 +134,7 @@ func TestDbRemoteSetCommand(t *testing.T) {
 	t.Run("throws error on migration mismatch", func(t *testing.T) {
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
-		require.NoError(t, utils.WriteConfig(fsys, true))
+		require.NoError(t, utils.WriteConfig(fsys, false))
 		// Setup mock postgres
 		conn := pgtest.NewConn()
 		defer conn.Close(t)
@@ -149,7 +149,7 @@ func TestDbRemoteSetCommand(t *testing.T) {
 	t.Run("throws error on malformed file name", func(t *testing.T) {
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
-		require.NoError(t, utils.WriteConfig(fsys, true))
+		require.NoError(t, utils.WriteConfig(fsys, false))
 		// Setup initial migration
 		version := "20220727064247"
 		_, err := fsys.Create("supabase/migrations/" + version + ".sql")
@@ -168,7 +168,7 @@ func TestDbRemoteSetCommand(t *testing.T) {
 	t.Run("throws error on failure to create directory", func(t *testing.T) {
 		// Setup in-memory fs
 		fsys := afero.NewMemMapFs()
-		require.NoError(t, utils.WriteConfig(fsys, true))
+		require.NoError(t, utils.WriteConfig(fsys, false))
 		// Setup mock postgres
 		conn := pgtest.NewConn()
 		defer conn.Close(t)
diff --git a/internal/utils/config_test.go b/internal/utils/config_test.go
index cffc80bf..df1a92e2 100644
--- a/internal/utils/config_test.go
+++ b/internal/utils/config_test.go
@@ -21,7 +21,6 @@ func TestConfigParsing(t *testing.T) {
 		t.Setenv("AZURE_CLIENT_ID", "hello")
 		t.Setenv("AZURE_SECRET", "this is cool")
 		assert.NoError(t, LoadConfigFS(fsys))
-		assert.NoError(t, InterpolateEnvInConfig())
 
 		assert.Equal(t, "hello", Config.Auth.External["azure"].ClientId)
 		assert.Equal(t, "this is cool", Config.Auth.External["azure"].Secret)
@@ -30,7 +29,6 @@ func TestConfigParsing(t *testing.T) {
 	t.Run("config file with environment variables fails when unset", func(t *testing.T) {
 		fsys := afero.NewMemMapFs()
 		assert.NoError(t, WriteConfig(fsys, true))
-		assert.NoError(t, LoadConfigFS(fsys))
-		assert.Error(t, InterpolateEnvInConfig())
+		assert.Error(t, LoadConfigFS(fsys))
 	})
 }