Repository: GoogleCloudPlatform/composer-local-dev

Issue:
Postgres Data volume bind-mounted to wrong target location
I noticed that task history and connections were being lost after `restart`ing a local environment. This was confusing to me since I thought that data would be persisted between restarts in the local `...composer/<env-name>/postgresql_data` directory. I inspected the postgres container and noticed this:

```json
{
    "Type": "bind",
    "Source": "/path/to/my/composer-local-dev/composer/test-env/postgresql_data",
    "Target": "/home/airflow//var/lib/postgresql/data"
}
```

It appears that we always prepend the `AIRFLOW_HOME` constant in this case, but that shouldn't be the case in the database container:

https://github.com/GoogleCloudPlatform/composer-local-dev/blob/d020b67ff80ce034136df209363999018042cb15/composer_local_dev/environment.py#L76-L83

Hints from issue comments:
@jlhawn: I think this can be most easily fixed by specifying the `PGDATA` env variable on the container. It's probably also a good idea to clean the path to remove the double-`/`.@jlhawn: I wouldn't release v0.10.0 with this major bug when using the *default* database engine.@jlhawn: > I think this can be most easily fixed by specifying the `PGDATA` env variable on the container. It's probably also a good idea to clean the path to remove the double-`/`.

I ended up fixing it a different way since there was also an issue with the `fix_file_permissions` function. #99 @ahidalgob: Hi @jlhawn . Thank you very much, I'll review your PR shortly.

I was able to confirm https://github.com/GoogleCloudPlatform/composer-local-dev/issues/95 which indeed is a significant issue, nonetheless the regression seemed to have happened both for postgresql and sqlite, so it wasn't related to the change of default and no one had a chance to fix before the release of 0.10.0, but it was next on our backlog.@ahidalgob: Released in 0.10.1, thank you for contributing :)

Solution patch:
diff --git a/composer_local_dev/environment.py b/composer_local_dev/environment.py
index 99dcef3..813b959 100644
--- a/composer_local_dev/environment.py
+++ b/composer_local_dev/environment.py
@@ -76,7 +76,12 @@ def get_image_mounts(
     return [
         docker.types.Mount(
             source=str(source),
-            target=f"{constants.AIRFLOW_HOME}/{target}",
+            # If target is absolute path, use it as is.
+            # Otherwise, prepend AIRFLOW_HOME to the target.
+            target=(
+                target if target.startswith("/")
+                else f"{constants.AIRFLOW_HOME}/{target}"
+            ),
             type="bind",
         )
         for source, target in mount_paths.items()
diff --git a/composer_local_dev/files.py b/composer_local_dev/files.py
index 116dc41..aa69b0b 100644
--- a/composer_local_dev/files.py
+++ b/composer_local_dev/files.py
@@ -165,7 +165,10 @@ def fix_file_permissions(
     make_file_readable_and_executable(entrypoint)
     make_file_readable_and_executable(run)
     make_file_writeable(requirements)
-    make_file_writeable(db_path)
+    if db_path.is_dir():
+        make_file_readable_and_executable(db_path)
+    else:
+        make_file_writeable(db_path)
 
 
 def make_file_readable_and_executable(file_path: pathlib.Path) -> None:


Test patch:
diff --git a/tests/unit/test_environment.py b/tests/unit/test_environment.py
index da4a575..5739132 100644
--- a/tests/unit/test_environment.py
+++ b/tests/unit/test_environment.py
@@ -1164,7 +1164,9 @@ def test_get_image_mounts(mocked_mount):
     gcloud_path = "config/path"
     kubeconfig_path = "/kube"
     requirements = path / "requirements.txt"
-    airflow_db_path = path / "airflow.db"
+    sqlite_db_path = path / "airflow.db"
+    postgresql_data_path = path / "postgresql_data"
+    postgresql_keep_path = path / ".keep"
     expected_mounts = [
         mock.call(
             source=str(requirements),
@@ -1190,10 +1192,20 @@ def test_get_image_mounts(mocked_mount):
             type="bind",
         ),
         mock.call(
-            source=str(airflow_db_path),
+            source=str(sqlite_db_path),
             target="/home/airflow/airflow/airflow.db",
             type="bind",
         ),
+        mock.call(
+            source=str(postgresql_data_path),
+            target="/var/lib/postgresql/data",
+            type="bind",
+        ),
+        mock.call(
+            source=str(postgresql_keep_path),
+            target="/home/airflow/airflow/.keep",
+            type="bind",
+        ),
         mock.call(
             source=kubeconfig_path,
             target="/home/airflow/.kube/",
@@ -1206,7 +1218,14 @@ def test_get_image_mounts(mocked_mount):
         gcloud_path,
         kubeconfig_path,
         requirements,
-        {airflow_db_path: "airflow/airflow.db"},
+        {
+            # There is no situation where we would specify both sqlite and
+            # postgresql mounts, but this lets us test the logic for both
+            # types of database driver at once.
+            sqlite_db_path: "airflow/airflow.db",
+            postgresql_data_path: "/var/lib/postgresql/data",
+            postgresql_keep_path: "airflow/.keep",
+        },
     )
     assert len(expected_mounts) == len(actual_mounts)
     mocked_mount.assert_has_calls(expected_mounts)
diff --git a/tests/unit/test_files.py b/tests/unit/test_files.py
index 2966de2..5f629ca 100644
--- a/tests/unit/test_files.py
+++ b/tests/unit/test_files.py
@@ -240,3 +240,67 @@ def test_dos2unix_file(tmp_path):
     with open(tmp_file, "rb") as fp:
         content = fp.read()
     assert content == linux_string
+
+
+class TestFixFilePermissions:
+    @mock.patch("pathlib.Path.chmod")
+    @mock.patch("composer_local_dev.utils.is_windows_os", return_value=True)
+    def test_fix_file_permissions_windows(self, mocked_is_windows, mocked_chmod):
+        """Test that no permission changes are made on Windows."""
+        files.fix_file_permissions(
+            entrypoint=pathlib.Path("entrypoint.sh"),
+            run=pathlib.Path("run.sh"),
+            requirements=pathlib.Path("requirements.txt"),
+            db_path=pathlib.Path("airflow.db"),
+        )
+        mocked_chmod.assert_not_called()
+
+    @mock.patch("pathlib.Path.chmod")
+    @mock.patch("composer_local_dev.utils.is_windows_os", return_value=False)
+    def test_fix_file_permissions_linux_sqlite(self, mocked_is_windows, mocked_chmod, tmp_path):
+        """Test permission changes on Linux with SQLite database."""
+        entrypoint = tmp_path / "entrypoint.sh"
+        run = tmp_path / "run.sh"
+        requirements = tmp_path / "requirements.txt"
+        db_path = tmp_path / "airflow.db"
+        
+        # Create the files
+        entrypoint.touch()
+        run.touch()
+        requirements.touch()
+        db_path.touch()
+        
+        files.fix_file_permissions(entrypoint, run, requirements, db_path)
+        
+        # Check that chmod was called with correct permissions
+        mocked_chmod.assert_has_calls([
+            mock.call(0o0755),  # entrypoint.sh
+            mock.call(0o0755),  # run.sh
+            mock.call(0o0666),  # requirements.txt
+            mock.call(0o0666),  # airflow.db
+        ])
+
+    @mock.patch("pathlib.Path.chmod")
+    @mock.patch("composer_local_dev.utils.is_windows_os", return_value=False)
+    def test_fix_file_permissions_linux_postgresql(self, mocked_is_windows, mocked_chmod, tmp_path):
+        """Test permission changes on Linux with PostgreSQL database directory."""
+        entrypoint = tmp_path / "entrypoint.sh"
+        run = tmp_path / "run.sh"
+        requirements = tmp_path / "requirements.txt"
+        db_path = tmp_path / "postgresql_data"
+        
+        # Create the files and directory
+        entrypoint.touch()
+        run.touch()
+        requirements.touch()
+        db_path.mkdir()
+        
+        files.fix_file_permissions(entrypoint, run, requirements, db_path)
+        
+        # Check that chmod was called with correct permissions
+        mocked_chmod.assert_has_calls([
+            mock.call(0o0755),  # entrypoint.sh
+            mock.call(0o0755),  # run.sh
+            mock.call(0o0666),  # requirements.txt
+            mock.call(0o0755),  # postgresql_data directory
+        ])