Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(job_attachments)!: Add OS-specific FileSystemPermissionSettings classes #41

Merged
merged 1 commit into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/deadline/job_attachments/_aws/deadline.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@ def get_queue(

# The API returns empty fields instead of an empty dict if there are no job attachment settings. So we need to
# double check if the s3BucketName is set.
if response["jobAttachmentSettings"] and response["jobAttachmentSettings"].get("s3BucketName"):
if response.get("jobAttachmentSettings") and response["jobAttachmentSettings"].get(
"s3BucketName"
):
job_attachment_settings = JobAttachmentS3Settings(
s3BucketName=response["jobAttachmentSettings"].get("s3BucketName", ""),
rootPrefix=response["jobAttachmentSettings"].get("rootPrefix", ""),
Expand Down
26 changes: 26 additions & 0 deletions src/deadline/job_attachments/download.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@
FileConflictResolution,
FileSystemPermissionSettings,
JobAttachmentS3Settings,
PosixFileSystemPermissionSettings,
WindowsFileSystemPermissionSettings,
)
from ._utils import _is_relative_to, _join_s3_paths

Expand Down Expand Up @@ -692,7 +694,31 @@ def _set_fs_group(
Sets file system group ownership and permissions for all files and directories
in the given paths, starting from root. It is expected that all `file_paths`
point to files, not directories.

Raises:
TypeError: If the `fs_permission_settings` are not specific to the underlying OS.
"""
if os.name == "posix":
if not isinstance(fs_permission_settings, PosixFileSystemPermissionSettings):
raise TypeError(
"The file system permission settings must be specific to Posix-based system."
)
_set_fs_group_for_posix(
file_paths=file_paths,
local_root=local_root,
fs_permission_settings=fs_permission_settings,
)
else: # if os.name is not "posix"
if not isinstance(fs_permission_settings, WindowsFileSystemPermissionSettings):
raise TypeError("The file system permission settings must be specific to Windows.")
# TODO: implement _set_fs_group_for_windows()


def _set_fs_group_for_posix(
file_paths: list[str],
local_root: str,
fs_permission_settings: PosixFileSystemPermissionSettings,
) -> None:
os_group = fs_permission_settings.os_group
dir_mode = fs_permission_settings.dir_mode
file_mode = fs_permission_settings.file_mode
Expand Down
26 changes: 21 additions & 5 deletions src/deadline/job_attachments/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from dataclasses import dataclass, field
from enum import Enum
from pathlib import Path
from typing import List, Optional, Set, Any
from typing import List, Optional, Set, Any, Union

from deadline.job_attachments.asset_manifests.base_manifest import BaseAssetManifest

Expand Down Expand Up @@ -266,11 +266,11 @@ class FileConflictResolution(Enum):


@dataclass
class FileSystemPermissionSettings:
class PosixFileSystemPermissionSettings:
"""
A data class representing file system permission-related information.
The specified permission modes will be bitwise-OR'ed with the directory
or file's existing permissions.
A dataclass representing file system permission-related information
for Posix. The specified permission modes will be bitwise-OR'ed with
the directory or file's existing permissions.

Attributes:
os_group (str): The target operating system group for ownership.
Expand All @@ -281,3 +281,19 @@ class FileSystemPermissionSettings:
os_group: str
dir_mode: int
file_mode: int


@dataclass
class WindowsFileSystemPermissionSettings:
"""
A dataclass representing file system permission-related information
for Windows.
"""

# TODO: Implement this


# A union of different file system permission settings that are based on the underlying OS.
FileSystemPermissionSettings = Union[
PosixFileSystemPermissionSettings, WindowsFileSystemPermissionSettings
]
14 changes: 9 additions & 5 deletions test/unit/deadline_job_attachments/test_download.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
from deadline.job_attachments.models import (
Attachments,
FileConflictResolution,
FileSystemPermissionSettings,
PosixFileSystemPermissionSettings,
Job,
JobAttachmentS3Settings,
Queue,
Expand Down Expand Up @@ -696,7 +696,11 @@ def test_get_job_input_output_paths_by_asset_root(
)

@mock_sts
def test_download_files_from_manifests_with_fs_permission_settings(
@pytest.mark.skipif(
sys.platform == "win32",
reason="This test is for testing file permission changes in Posix-based OS.",
)
def test_download_files_from_manifests_with_posix_fs_permission_settings(
self,
tmp_path: Path,
manifest_version: ManifestVersion,
Expand All @@ -713,7 +717,7 @@ def test_download_files_from_manifests_with_fs_permission_settings(
manifest = decode_manifest(mannifest_str)

manifests_by_root = {str(tmp_path): manifest}
fs_permission_settings = FileSystemPermissionSettings(
fs_permission_settings = PosixFileSystemPermissionSettings(
os_group="test-group",
dir_mode=0o20,
file_mode=0o20,
Expand Down Expand Up @@ -773,7 +777,7 @@ def test_download_files_from_manifests_with_fs_permission_settings(
not (has_posix_target_user() and has_posix_disjoint_user()),
reason="Must be running inside of the sudo_environment testing container.",
)
def test_download_files_from_manifests_have_correct_group(
def test_download_files_from_manifests_have_correct_group_posix(
self,
tmp_path: Path,
manifest_version: ManifestVersion,
Expand Down Expand Up @@ -803,7 +807,7 @@ def test_download_files_from_manifests_have_correct_group(
manifest = decode_manifest(mannifest_str)
manifests_by_root = {str(tmp_path): manifest}

fs_permission_settings = FileSystemPermissionSettings(
fs_permission_settings = PosixFileSystemPermissionSettings(
os_group=posix_target_group,
dir_mode=0o20,
file_mode=0o20,
Expand Down