Repository: prefab-cloud/prefab-cloud-python

Issue:
Does not work great in AWS lambda (or any HOME-less environment) currently
Hey there,

we are trying to use prefab-cloud-python in a AWS lambda, and since those usually don't have a HOME directory defined, the following lines throw errors:

https://github.com/prefab-cloud/prefab-cloud-python/blob/746b284749ee40ac8be9a85581f10eb9a913683d/prefab_cloud_python/options.py#L59
https://github.com/prefab-cloud/prefab-cloud-python/blob/746b284749ee40ac8be9a85581f10eb9a913683d/prefab_cloud_python/config_client.py#L248

The latter even fails, when an XDG_CACHE_HOME variable is set in the environment since .get() is not used when trying to access HOME.
Our workaround is to set a HOME environment variable that points to /tmp for the lambdas, but I'm wondering if relying on HOME to be present in a server environment (serverless or not) is such a great idea?

Discussion:

Patch:
diff --git a/prefab_cloud_python/config_client.py b/prefab_cloud_python/config_client.py
index 7b25208..2d56c6e 100644
--- a/prefab_cloud_python/config_client.py
+++ b/prefab_cloud_python/config_client.py
@@ -55,6 +55,7 @@ class ConfigClient:
         self.checkpoint_freq_secs = 60
         self.config_loader = ConfigLoader(base_client)
         self.config_resolver = ConfigResolver(base_client, self.config_loader)
+        self._cache_path = None
         self.set_cache_path()
 
         if self.options.is_local_only():
@@ -209,6 +210,8 @@ class ConfigClient:
     def cache_configs(self, configs):
         if not self.options.use_local_cache:
             return
+        if not self.cache_path:
+            return
         with open(self.cache_path, "w") as f:
             f.write(MessageToJson(configs))
             logger.debug(f"Cached configs to {self.cache_path}")
@@ -216,6 +219,8 @@ class ConfigClient:
     def load_cache(self):
         if not self.options.use_local_cache:
             return False
+        if not self.cache_path:
+            return False
         try:
             with open(self.cache_path, "r") as f:
                 configs = Parse(f.read(), Prefab.Configs())
@@ -244,15 +249,19 @@ class ConfigClient:
         logger.info(f"Unlocked config via {source}")
 
     def set_cache_path(self):
-        dir = os.environ.get(
-            "XDG_CACHE_HOME", os.path.join(os.environ["HOME"], ".cache")
-        )
-        file_name = f"prefab.cache.{self.base_client.options.api_key_id}.json"
-        self.cache_path = os.path.join(dir, file_name)
+        home_dir_cache_path = None
+        home_dir = os.environ.get("HOME")
+        if home_dir:
+            home_dir_cache_path = os.path.join(home_dir, ".cache")
+        cache_path = os.environ.get("XDG_CACHE_HOME", home_dir_cache_path)
+        if cache_path:
+            file_name = f"prefab.cache.{self.base_client.options.api_key_id}.json"
+            self.cache_path = os.path.join(cache_path, file_name)
 
     @property
     def cache_path(self):
-        os.makedirs(os.path.dirname(self._cache_path), exist_ok=True)
+        if self._cache_path:
+            os.makedirs(os.path.dirname(self._cache_path), exist_ok=True)
         return self._cache_path
 
     @cache_path.setter
diff --git a/prefab_cloud_python/config_loader.py b/prefab_cloud_python/config_loader.py
index 7f942a5..f618e09 100644
--- a/prefab_cloud_python/config_loader.py
+++ b/prefab_cloud_python/config_loader.py
@@ -12,8 +12,8 @@ class ConfigLoader:
         self.base_client = base_client
         self.options = base_client.options
         self.highwater_mark = 0
-        self.classpath_config = self.__load_classpath_config()
-        self.local_overrides = self.__load_local_overrides()
+        self.classpath_config = self.__load_classpath_config() or {}
+        self.local_overrides = self.__load_local_overrides() or {}
         self.api_config = {}
 
     def calc_config(self):
@@ -50,8 +50,9 @@ class ConfigLoader:
     def __load_local_overrides(self):
         if self.options.has_datafile():
             return {}
-        override_dir = self.options.prefab_config_override_dir
-        return self.__load_config_from(override_dir)
+        if self.options.prefab_config_override_dir:
+            return self.__load_config_from(self.options.prefab_config_override_dir)
+        return {}
 
     def __load_config_from(self, dir):
         envs = self.options.prefab_envs


Test patch:
diff --git a/tests/test_config_client.py b/tests/test_config_client.py
index b0e5842..3028724 100644
--- a/tests/test_config_client.py
+++ b/tests/test_config_client.py
@@ -1,5 +1,5 @@
 from prefab_cloud_python import Options, Client
-from prefab_cloud_python.config_client import MissingDefaultException
+from prefab_cloud_python.config_client import MissingDefaultException, ConfigClient
 import prefab_pb2 as Prefab
 import pytest
 import os
@@ -8,17 +8,61 @@ from contextlib import contextmanager
 
 
 @contextmanager
-def extended_env(new_env_vars):
+def extended_env(new_env_vars, deleted_env_vars=[]):
     old_env = os.environ.copy()
     os.environ.update(new_env_vars)
+    for deleted_env_var in deleted_env_vars:
+        os.environ.pop(deleted_env_var, None)
     yield
     os.environ.clear()
     os.environ.update(old_env)
 
 
+class ConfigClientFactoryFixture:
+    def __init__(self):
+        self.client = None
+
+    def create_config_client(self, options: Options) -> ConfigClient:
+        self.client = Client(options)
+        return self.client.config_client()
+
+    def close(self):
+        if self.client:
+            self.client.close()
+
+
+@pytest.fixture
+def config_client_factory():
+    factory_fixture = ConfigClientFactoryFixture()
+    yield factory_fixture
+    factory_fixture.close()
+
+
+@pytest.fixture
+def options():
+    def options(
+        on_no_default="RAISE",
+        x_use_local_cache=True,
+        prefab_envs=["unit_tests"],
+        api_key=None,
+        prefab_datasources="LOCAL_ONLY",
+    ):
+        return Options(
+            api_key=api_key,
+            prefab_config_classpath_dir="tests",
+            prefab_envs=prefab_envs,
+            prefab_datasources=prefab_datasources,
+            x_use_local_cache=x_use_local_cache,
+            on_no_default=on_no_default,
+            collect_sync_interval=None,
+        )
+
+    return options
+
+
 class TestConfigClient:
-    def test_get(self):
-        config_client = self.build_config_client()
+    def test_get(self, config_client_factory, options):
+        config_client = config_client_factory.create_config_client(options())
 
         assert config_client.get("sample") == "test sample value"
         assert config_client.get("sample_int") == 123
@@ -26,13 +70,13 @@ class TestConfigClient:
         assert config_client.get("sample_bool")
         assert config_client.get("log-level.app") == Prefab.LogLevel.Value("ERROR")
 
-    def test_get_with_default(self):
-        config_client = self.build_config_client()
+    def test_get_with_default(self, config_client_factory, options):
+        config_client = config_client_factory.create_config_client(options())
 
         assert config_client.get("bad key", "default value") == "default value"
 
-    def test_get_without_default_raises(self):
-        config_client = self.build_config_client()
+    def test_get_without_default_raises(self, config_client_factory, options):
+        config_client = config_client_factory.create_config_client(options())
 
         with pytest.raises(MissingDefaultException) as exception:
             config_client.get("bad key")
@@ -41,12 +85,16 @@ class TestConfigClient:
             exception.value
         )
 
-    def test_get_without_default_returns_none_if_configured(self):
-        config_client = self.build_config_client("RETURN_NONE")
+    def test_get_without_default_returns_none_if_configured(
+        self, config_client_factory, options
+    ):
+        config_client = config_client_factory.create_config_client(
+            options(on_no_default="RETURN_NONE")
+        )
         assert config_client.get("bad key") is None
 
-    def test_caching(self):
-        config_client = self.build_config_client()
+    def test_caching(self, config_client_factory, options):
+        config_client = config_client_factory.create_config_client(options())
         cached_config = Prefab.Configs(
             configs=[
                 Prefab.Config(
@@ -72,39 +120,32 @@ class TestConfigClient:
         config_client.load_cache()
         assert config_client.get("test") == "test value"
 
-    def test_cache_path(self):
-        options = Options(
-            api_key="123-API-KEY-SDK",
-            x_use_local_cache=True,
-            collect_sync_interval=None,
+    def test_cache_path(self, config_client_factory, options):
+        config_client = config_client_factory.create_config_client(
+            options(api_key="123-API-KEY-SDK", prefab_datasources="ALL")
         )
-        client = Client(options)
         assert (
-            client.config_client().cache_path
+            config_client.cache_path
             == f"{os.environ['HOME']}/.cache/prefab.cache.123.json"
         )
 
-    def test_cache_path_local_only(self):
-        config_client = self.build_config_client()
+    def test_cache_path_local_only(self, config_client_factory, options):
+        config_client = config_client_factory.create_config_client(
+            options(prefab_envs=[])
+        )
         assert (
             config_client.cache_path
             == f"{os.environ['HOME']}/.cache/prefab.cache.local.json"
         )
 
-    def test_cache_path_respects_xdg(self):
+    def test_cache_path_local_only_with_no_home_dir_or_xdg(
+        self, config_client_factory, options
+    ):
+        with extended_env({}, deleted_env_vars=["HOME"]):
+            config_client = config_client_factory.create_config_client(options())
+            assert config_client.cache_path is None
+
+    def test_cache_path_respects_xdg(self, config_client_factory, options):
         with extended_env({"XDG_CACHE_HOME": "/tmp"}):
-            config_client = self.build_config_client()
+            config_client = config_client_factory.create_config_client(options())
             assert config_client.cache_path == "/tmp/prefab.cache.local.json"
-
-    @staticmethod
-    def build_config_client(on_no_default="RAISE"):
-        options = Options(
-            prefab_config_classpath_dir="tests",
-            prefab_envs="unit_tests",
-            prefab_datasources="LOCAL_ONLY",
-            x_use_local_cache=True,
-            on_no_default=on_no_default,
-            collect_sync_interval=None,
-        )
-        client = Client(options)
-        return client.config_client()