fix!: updates to path mapping

Two changes related to updates in the spec: 1. The field name `source_os` in a path mapping rule's definition was determined to be somewhat ambiguous in its meaning. So, it is being changed to `source_path_format`. 2. We passed path mapping rules in an environment variable before the introduction of the Session.PathMappingRulesFile value. With the creation of that value for format strings, we deprecated the PATH_MAPPING_RULES environment variable. It is time to delete it. **BREAKING CHANGE** 1. The format of the path mapping rules JSON is changed such that the 'source_os' key is now called 'source_path_format' 2. The PATH_MAPPING_RULES environment variable is no longer injected in to a running Session's environment. Signed-off-by: Daniel Neilson <[email protected]>
OpenJobDescription · Sep 14, 2023 · 2321af9 · 2321af9
1 parent 8907765
commit 2321af9
Show file tree

Hide file tree

Showing 5 changed files with 98 additions and 149 deletions.
diff --git a/src/openjd/sessions/__init__.py b/src/openjd/sessions/__init__.py
@@ -1,7 +1,7 @@
 # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 
 from ._logging import LOG
-from ._path_mapping import PathMappingOS, PathMappingRule
+from ._path_mapping import PathFormat, PathMappingRule
 from ._session import ActionStatus, Session, SessionCallbackType, SessionState
 from ._session_user import PosixSessionUser, SessionUser
 from ._types import (
@@ -23,7 +23,7 @@
     "LOG",
     "Parameter",
     "ParameterType",
-    "PathMappingOS",
+    "PathFormat",
     "PathMappingRule",
     "PosixSessionUser",
     "Session",

diff --git a/src/openjd/sessions/_path_mapping.py b/src/openjd/sessions/_path_mapping.py
@@ -6,29 +6,33 @@
 from pathlib import PurePath, PurePosixPath, PureWindowsPath
 
 
-class PathMappingOS(str, Enum):
+class PathFormat(str, Enum):
     POSIX = "POSIX"
     WINDOWS = "WINDOWS"
 
 
 @dataclass(frozen=True)
 class PathMappingRule:
-    source_os: PathMappingOS
+    source_path_format: PathFormat
     source_path: PurePath
     destination_path: PurePath
 
     def __init__(
-        self, *, source_os: PathMappingOS, source_path: PurePath, destination_path: PurePath
+        self, *, source_path_format: PathFormat, source_path: PurePath, destination_path: PurePath
     ):
-        if source_os == PathMappingOS.POSIX:
+        if source_path_format == PathFormat.POSIX:
             if not isinstance(source_path, PurePosixPath):
-                raise ValueError("Path mapping rule source_os does not match source_path type")
+                raise ValueError(
+                    "Path mapping rule source_path_format does not match source_path type"
+                )
         else:
             if not isinstance(source_path, PureWindowsPath):
-                raise ValueError("Path mapping rule source_os does not match source_path type")
+                raise ValueError(
+                    "Path mapping rule source_path_format does not match source_path type"
+                )
 
         # This roundabout way can set the attributes of a frozen dataclass
-        object.__setattr__(self, "source_os", source_os)
+        object.__setattr__(self, "source_path_format", source_path_format)
         object.__setattr__(self, "source_path", source_path)
         object.__setattr__(self, "destination_path", destination_path)
 
@@ -44,9 +48,9 @@ def from_dict(rule: dict[str, str]) -> "PathMappingRule":
             if name not in rule:
                 raise ValueError(f"Path mapping rule requires the following fields: {field_names}")
 
-        source_os = PathMappingOS(rule["source_os"].upper())
+        source_path_format = PathFormat(rule["source_path_format"].upper())
         source_path: PurePath
-        if source_os == PathMappingOS.POSIX:
+        if source_path_format == PathFormat.POSIX:
             source_path = PurePosixPath(rule["source_path"])
         else:
             source_path = PureWindowsPath(rule["source_path"])
@@ -59,13 +63,15 @@ def from_dict(rule: dict[str, str]) -> "PathMappingRule":
             )
 
         return PathMappingRule(
-            source_os=source_os, source_path=source_path, destination_path=destination_path
+            source_path_format=source_path_format,
+            source_path=source_path,
+            destination_path=destination_path,
         )
 
     def to_dict(self) -> dict[str, str]:
         """Returns a dictionary representation of the PathMappingRule."""
         return {
-            "source_os": self.source_os.name,
+            "source_path_format": self.source_path_format.name,
             "source_path": str(self.source_path),
             "destination_path": str(self.destination_path),
         }
@@ -78,7 +84,7 @@ def apply(self, *, path: str) -> tuple[bool, str]:
         mapped path. If it doesn't match, then it returns the original path unmodified.
         """
         pure_path: PurePath
-        if self.source_os == PathMappingOS.POSIX:
+        if self.source_path_format == PathFormat.POSIX:
             pure_path = PurePosixPath(path)
         else:
             pure_path = PureWindowsPath(path)
@@ -91,17 +97,17 @@ def apply(self, *, path: str) -> tuple[bool, str]:
         )
         if os_name == "posix":
             result = str(PurePosixPath(*remapped_parts))
-            if self._has_trailing_slash(self.source_os, path):
+            if self._has_trailing_slash(self.source_path_format, path):
                 result += "/"
         else:
             result = str(PureWindowsPath(*remapped_parts))
-            if self._has_trailing_slash(self.source_os, path):
+            if self._has_trailing_slash(self.source_path_format, path):
                 result += "\\"
 
         return True, result
 
-    def _has_trailing_slash(self, os: PathMappingOS, path: str) -> bool:
-        if os == PathMappingOS.POSIX:
+    def _has_trailing_slash(self, os: PathFormat, path: str) -> bool:
+        if os == PathFormat.POSIX:
             return path.endswith("/")
         else:
             return path.endswith("\\")
diff --git a/src/openjd/sessions/_session.py b/src/openjd/sessions/_session.py
@@ -777,24 +777,21 @@ def _materialize_path_mapping(
         """Materialize path mapping rules to disk and the os environment variables."""
         if self._path_mapping_rules:
             rules_dict = {
+                "version": "pathmapping-1.0",
                 "path_mapping_rules": [
                     {
-                        "source_os": rule.source_os.value,
+                        "source_path_format": rule.source_path_format.value,
                         "source_path": str(rule.source_path),
                         "destination_path": str(rule.destination_path),
                     }
                     for rule in self._path_mapping_rules
-                ]
+                ],
             }
             symtab[ValueReferenceConstants_2023_09.HAS_PATH_MAPPING_RULES.value] = "true"
         else:
             rules_dict = dict()
             symtab[ValueReferenceConstants_2023_09.HAS_PATH_MAPPING_RULES.value] = "false"
         rules_json = json.dumps(rules_dict)
-        # TODO - Remove this environment variable before the formal release of the lib/spec.
-        #  Reason: This is an interim workaround for functionality that was missing.
-        os_env["PATH_MAPPING_RULES"] = rules_json
-        # TODO /stop
         file_handle, filename = mkstemp(dir=self.working_directory, suffix=".json", text=True)
         os.close(file_handle)
         write_file_for_user(Path(filename), rules_json, self._user)

diff --git a/test/openjd/sessions/test_path_mapping.py b/test/openjd/sessions/test_path_mapping.py
@@ -6,7 +6,7 @@
 
 import pytest
 
-from openjd.sessions import PathMappingOS, PathMappingRule
+from openjd.sessions import PathFormat, PathMappingRule
 from openjd.sessions import _path_mapping as path_mapping_impl_mod
 
 
@@ -21,7 +21,7 @@ class TestPathMapping:
         [
             pytest.param(
                 PathMappingRule(
-                    source_os=PathMappingOS.POSIX,
+                    source_path_format=PathFormat.POSIX,
                     source_path=PurePosixPath("/mnt/shared/"),
                     destination_path=PurePosixPath("/newprefix"),
                 ),
@@ -52,7 +52,7 @@ class TestPathMapping:
         + [
             pytest.param(
                 PathMappingRule(
-                    source_os=PathMappingOS.POSIX,
+                    source_path_format=PathFormat.POSIX,
                     source_path=PurePosixPath("/mnt/shared/"),
                     destination_path=PureWindowsPath("c:\\newprefix"),
                 ),
@@ -91,7 +91,7 @@ class TestPathMapping:
         + [
             pytest.param(
                 PathMappingRule(
-                    source_os=PathMappingOS.WINDOWS,
+                    source_path_format=PathFormat.WINDOWS,
                     source_path=PureWindowsPath("c:\\mnt\\shared\\"),
                     destination_path=PurePosixPath("/newprefix"),
                 ),
@@ -130,7 +130,7 @@ class TestPathMapping:
         + [
             pytest.param(
                 PathMappingRule(
-                    source_os=PathMappingOS.WINDOWS,
+                    source_path_format=PathFormat.WINDOWS,
                     source_path=PureWindowsPath("c:\\mnt\\shared\\"),
                     destination_path=PureWindowsPath("c:\\newprefix"),
                 ),
@@ -173,7 +173,7 @@ class TestPathMapping:
         + [
             pytest.param(
                 PathMappingRule(
-                    source_os=PathMappingOS.WINDOWS,
+                    source_path_format=PathFormat.WINDOWS,
                     source_path=PureWindowsPath("\\\\128.0.0.1\\share\\assets"),
                     destination_path=PureWindowsPath("z:\\assets"),
                 ),
@@ -198,7 +198,7 @@ class TestPathMapping:
         + [
             pytest.param(
                 PathMappingRule(
-                    source_os=PathMappingOS.WINDOWS,
+                    source_path_format=PathFormat.WINDOWS,
                     source_path=PureWindowsPath("z:\\assets"),
                     destination_path=PureWindowsPath("\\\\128.0.0.1\\share\\assets"),
                 ),
@@ -223,7 +223,7 @@ class TestPathMapping:
         + [
             pytest.param(
                 PathMappingRule(
-                    source_os=PathMappingOS.WINDOWS,
+                    source_path_format=PathFormat.WINDOWS,
                     source_path=PureWindowsPath("\\\\.\\c:\\assets"),
                     destination_path=PureWindowsPath("z:\\assets"),
                 ),
@@ -248,7 +248,7 @@ class TestPathMapping:
         + [
             pytest.param(
                 PathMappingRule(
-                    source_os=PathMappingOS.WINDOWS,
+                    source_path_format=PathFormat.WINDOWS,
                     source_path=PureWindowsPath("z:\\assets"),
                     destination_path=PureWindowsPath("\\\\.\\c:\\assets"),
                 ),
@@ -273,7 +273,7 @@ class TestPathMapping:
         + [
             pytest.param(
                 PathMappingRule(
-                    source_os=PathMappingOS.WINDOWS,
+                    source_path_format=PathFormat.WINDOWS,
                     source_path=PureWindowsPath("\\\\?\\c:\\assets"),
                     destination_path=PureWindowsPath("z:\\assets"),
                 ),
@@ -298,7 +298,7 @@ class TestPathMapping:
         + [
             pytest.param(
                 PathMappingRule(
-                    source_os=PathMappingOS.WINDOWS,
+                    source_path_format=PathFormat.WINDOWS,
                     source_path=PureWindowsPath("z:\\assets"),
                     destination_path=PureWindowsPath("\\\\?\\c:\\assets"),
                 ),
@@ -323,7 +323,7 @@ class TestPathMapping:
         + [
             pytest.param(
                 PathMappingRule(
-                    source_os=PathMappingOS.WINDOWS,
+                    source_path_format=PathFormat.WINDOWS,
                     source_path=PureWindowsPath(
                         "\\\\?\\Volume{b75e2c83-0000-0000-0000-602f12345678}\\assets"
                     ),
@@ -350,7 +350,7 @@ class TestPathMapping:
         + [
             pytest.param(
                 PathMappingRule(
-                    source_os=PathMappingOS.WINDOWS,
+                    source_path_format=PathFormat.WINDOWS,
                     source_path=PureWindowsPath("z:\\assets"),
                     destination_path=PureWindowsPath(
                         "\\\\?\\Volume{b75e2c83-0000-0000-0000-602f12345678}\\assets"
@@ -393,7 +393,7 @@ def test_remaps(
         [
             pytest.param(
                 PathMappingRule(
-                    source_os=PathMappingOS.POSIX,
+                    source_path_format=PathFormat.POSIX,
                     source_path=PurePosixPath("/mnt/shared"),
                     destination_path=PureWindowsPath("c:\\newprefix"),
                 ),
@@ -409,7 +409,7 @@ def test_remaps(
         + [
             pytest.param(
                 PathMappingRule(
-                    source_os=PathMappingOS.WINDOWS,
+                    source_path_format=PathFormat.WINDOWS,
                     source_path=PureWindowsPath("c:\\mnt\\shared\\"),
                     destination_path=PureWindowsPath("c:\\newprefix"),
                 ),
@@ -437,18 +437,18 @@ def test_does_not_remap(self, rule: PathMappingRule, given: str) -> None:
         "rule_params",
         [
             {
-                "source_os": PathMappingOS.WINDOWS,
+                "source_path_format": PathFormat.WINDOWS,
                 "source_path": PurePosixPath("C:\\oldprefix"),
                 "destination_path": PureWindowsPath("c:\\newprefix"),
             },
             {
-                "source_os": PathMappingOS.POSIX,
+                "source_path_format": PathFormat.POSIX,
                 "source_path": PureWindowsPath("/mnt/oldprefix"),
                 "destination_path": PureWindowsPath("c:\\newprefix"),
             },
         ],
     )
-    def test_mismatching_source_os_path(self, rule_params):
+    def test_mismatching_source_path_format_path(self, rule_params):
         with pytest.raises(ValueError):
             PathMappingRule(**rule_params)
 
@@ -457,48 +457,48 @@ def test_mismatching_source_os_path(self, rule_params):
         [
             (
                 {
-                    "source_os": "WINDOWS",
+                    "source_path_format": "WINDOWS",
                     "source_path": "C:\\oldprefix",
                     "destination_path": "c:\\newprefix",
                 },
                 PathMappingRule(
-                    source_os=PathMappingOS.WINDOWS,
+                    source_path_format=PathFormat.WINDOWS,
                     source_path=PureWindowsPath("C:\\oldprefix"),
                     destination_path=PurePath("c:\\newprefix"),
                 ),
             ),
             (
                 {
-                    "source_os": "POSIX",
+                    "source_path_format": "POSIX",
                     "source_path": "/mnt/oldprefix",
                     "destination_path": "c:\\newprefix",
                 },
                 PathMappingRule(
-                    source_os=PathMappingOS.POSIX,
+                    source_path_format=PathFormat.POSIX,
                     source_path=PurePosixPath("/mnt/oldprefix"),
                     destination_path=PurePath("c:\\newprefix"),
                 ),
             ),
             (
                 {
-                    "source_os": "windows",
+                    "source_path_format": "windows",
                     "source_path": "C:\\oldprefix",
                     "destination_path": "c:\\newprefix",
                 },
                 PathMappingRule(
-                    source_os=PathMappingOS.WINDOWS,
+                    source_path_format=PathFormat.WINDOWS,
                     source_path=PureWindowsPath("C:\\oldprefix"),
                     destination_path=PurePath("c:\\newprefix"),
                 ),
             ),
             (
                 {
-                    "source_os": "posix",
+                    "source_path_format": "posix",
                     "source_path": "/mnt/oldprefix",
                     "destination_path": "c:\\newprefix",
                 },
                 PathMappingRule(
-                    source_os=PathMappingOS.POSIX,
+                    source_path_format=PathFormat.POSIX,
                     source_path=PurePosixPath("/mnt/oldprefix"),
                     destination_path=PurePath("c:\\newprefix"),
                 ),
@@ -514,17 +514,17 @@ def test_from_dict_success(self, dict_rule, expected):
         [
             (
                 {
-                    "source_os": "WINDOWS10",
+                    "source_path_format": "WINDOWS10",
                     "source_path": "C:\\oldprefix",
                     "destination_path": "c:\\newprefix",
                 }
             ),
             ({"source_path": "/mnt/oldprefix", "destination_path": "c:\\newprefix"}),
-            ({"source_os": "POSIX", "destination_path": "c:\\newprefix"}),
-            ({"source_os": "POSIX", "source_path": "/mnt/oldprefix"}),
+            ({"source_path_format": "POSIX", "destination_path": "c:\\newprefix"}),
+            ({"source_path_format": "POSIX", "source_path": "/mnt/oldprefix"}),
             (
                 {
-                    "source_os": "windows",
+                    "source_path_format": "windows",
                     "source_path": "C:\\oldprefix",
                     "destination_path": "c:\\newprefix",
                     "extra_field": "value",