[SYNPY-1731] Implement DockerRepository entity

[linglp]

Problem

DockerRepository class was not implemented in OOP way.

Solution

• Implement DockerRepository class in OOP
• Adjusted get and delete in factory_operations.oy so that we could do something like:

from synapseclient.operations import get
docker_repo = get(synapse_id="syn11111")

to get and delete a docker repo

[Copilot]

Pull request overview

This PR implements the DockerRepository entity class in an object-oriented programming (OOP) style to match the design of other entity types in the Synapse Python client. The implementation provides methods for getting, storing, and deleting Docker repositories, and integrates with the factory operations (get, delete) to support DockerRepository entities.

Changes:

• Added new DockerRepository entity model with full CRUD operations (get, store, delete) in both sync and async variants
• Integrated DockerRepository into factory operations (get, delete) and entity factory for type mapping
• Created Docker services API module for repository name lookup functionality

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
synapseclient/models/docker.py	Main DockerRepository dataclass with get, store, and delete methods
synapseclient/models/protocols/docker_protocol.py	Protocol definition for synchronous DockerRepository interface
synapseclient/api/docker_services.py	API service for looking up Docker repository ID by name
synapseclient/api/init.py	Exports the new get_entity_id_by_repository_name service
synapseclient/api/entity_factory.py	Adds DockerRepository to entity type mapping
synapseclient/operations/factory_operations.py	Integrates DockerRepository into get/get_async operations
synapseclient/operations/delete_operations.py	Integrates DockerRepository into delete/delete_async operations
synapseclient/models/init.py	Exports DockerRepository model
tests/unit/synapseclient/models/synchronous/unit_test_docker.py	Unit tests for synchronous DockerRepository methods
tests/unit/synapseclient/models/async/unit_test_docker_async.py	Unit tests for async DockerRepository methods
tests/integration/synapseclient/models/synchronous/test_docker.py	Integration tests for synchronous DockerRepository operations
tests/integration/synapseclient/models/async/test_docker_async.py	Integration tests for async DockerRepository operations
tests/integration/synapseclient/operations/synchronous/test_factory_operations.py	Tests for get operation with DockerRepository
tests/integration/synapseclient/operations/synchronous/test_delete_operations.py	Tests for delete operation with DockerRepository
tests/integration/synapseclient/operations/async/test_factory_operations_async.py	Tests for async get operation with DockerRepository
tests/integration/synapseclient/operations/async/test_delete_operations_async.py	Tests for async delete operation with DockerRepository

Comments suppressed due to low confidence (14)

tests/integration/synapseclient/models/synchronous/test_docker.py:32

• The project is scheduled for cleanup twice: once on line 25 and again on line 31. The second call on line 31 is redundant and should be removed. Additionally, the docker_repo itself should be scheduled for cleanup after it is created, not the project again.

        schedule_for_cleanup(project.id)
        docker_repo.store(synapse_client=self.syn)

tests/integration/synapseclient/models/synchronous/test_docker.py:114

• The assertions on lines 110-114 expect that etag, modified_on, modified_by, etc. remain unchanged after an update. However, when an entity is updated in Synapse, these fields typically change. The etag always changes on update (as per OCC), and modified_on and modified_by should also change to reflect the update. These assertions will likely fail. Consider removing these assertions or changing them to verify that etag and modified_on have changed, similar to the async version of this test which has a comment noting these may change.

        assert updated_repo.etag == test_docker_repo.etag
        assert updated_repo.created_on == test_docker_repo.created_on
        assert updated_repo.modified_on == test_docker_repo.modified_on
        assert updated_repo.created_by == test_docker_repo.created_by
        assert updated_repo.modified_by == test_docker_repo.modified_by

tests/integration/synapseclient/operations/async/test_delete_operations_async.py:516

• Function name has a typo: "test_delete_docker_repo_by_objec_async" is missing the final 't' in "object". This should be "test_delete_docker_repo_by_object_async" to be consistent with the synchronous version.

    async def test_delete_docker_repo_by_objec_async(

tests/integration/synapseclient/operations/async/test_delete_operations_async.py:514

• The assertion is placed inside the pytest.raises context manager, which means it will never be executed. When DockerRepository.get() raises a SynapseHTTPError, the exception is caught and the code after it (the assertion) is not executed. The assertion should be moved outside the context manager block to check the exception message after it has been caught. This is a bug that prevents the test from verifying the correct error message.

        with pytest.raises(SynapseHTTPError) as e:
            DockerRepository(id=docker_repo.id).get(synapse_client=self.syn)
            assert f"404 Client Error: Entity {docker_repo.id} is in trash can" in str(
                e.value
            )

synapseclient/models/docker.py:294

• The get_async, store_async, and delete_async methods are missing OpenTelemetry tracing decorators. Other entity types in the codebase (Project, Folder, File) use the @otel_trace_method decorator on these async methods for observability. For consistency with codebase conventions, these methods should include the decorator.

    async def get_async(
        self,
        *,
        synapse_client: Optional[Synapse] = None,
    ) -> "DockerRepository":
        """Get the Docker repository metadata from Synapse.

        You can retrieve a Docker repository by either:

        - `id`: The Synapse ID of the Docker repository (e.g., "syn123"). This works
          for both managed and external Docker repositories.
        - `repository_name`: The name of a **managed** Docker repository
          (e.g., "docker.synapse.org/syn123/my-repo"). This lookup method **only works
          for managed repositories** hosted on Synapse's Docker registry. External
          (unmanaged) Docker repositories must be retrieved using their Synapse `id`.

        If both are provided, `id` takes precedence.

        Arguments:
            synapse_client: If not passed in and caching was not disabled by
                `Synapse.allow_client_caching(False)` this will use the last created
                instance from the Synapse class constructor.

        Returns:
            The DockerRepository object.

        Raises:
            ValueError: If neither id nor repository_name is set.
            SynapseHTTPError: If retrieving by repository_name and the repository
                is not found or is not a managed repository.

        Example: Using this function
            Retrieve a Docker repository by Synapse ID (works for both managed and
            external repositories):

            ```python
            from synapseclient import Synapse
            from synapseclient.models import DockerRepository

            syn = Synapse()
            syn.login()

            docker_repo = DockerRepository(id="syn123").get()
            print(docker_repo)
            ```

            Retrieve a managed Docker repository by repository name (only works for
            managed repositories hosted at docker.synapse.org):

            ```python
            from synapseclient import Synapse
            from synapseclient.models import DockerRepository

            syn = Synapse()
            syn.login()

            docker_repo = DockerRepository(
                repository_name="docker.synapse.org/syn123/my-repo"
            ).get()
            print(docker_repo)
            ```
        """
        if not self.id and not self.repository_name:
            raise ValueError(
                "The Docker repository must have either an id or repository_name set."
            )

        # If we only have repository_name, look up the entity ID first
        if not self.id and self.repository_name:
            self.id = await self._get_entity_id_by_repository_name(
                synapse_client=synapse_client,
            )

        await get_from_entity_factory(
            entity_to_update=self,
            synapse_id_or_path=self.id,
            synapse_client=synapse_client,
        )

        self._set_last_persistent_instance()
        Synapse.get_client(synapse_client=synapse_client).logger.debug(
            f"Got DockerRepository {self.repository_name}, id: {self.id}"
        )
        return self

tests/unit/synapseclient/models/async/unit_test_docker_async.py:50

• The type hint uses 'any' (lowercase) instead of 'Any' from typing. In Python, 'any' is a built-in function, not a type. This should be 'Dict[str, Any]' with 'Any' imported from typing, which follows Python typing conventions.

    def get_example_bundle_response(self) -> dict[str, any]:

tests/integration/synapseclient/operations/synchronous/test_delete_operations.py:490

• The assertion is placed inside the pytest.raises context manager, which means it will never be executed. When DockerRepository.get() raises a SynapseHTTPError, the exception is caught and the code after it (the assertion) is not executed. The assertion should be moved outside the context manager block to check the exception message after it has been caught. This is a bug that prevents the test from verifying the correct error message.

        with pytest.raises(SynapseHTTPError) as e:
            DockerRepository(id=docker_repo.id).get(synapse_client=self.syn)
            assert f"404 Client Error: Entity {docker_repo.id} is in trash can" in str(
                e.value
            )

synapseclient/models/docker.py:441

• The delete_async method is using the delete operation function instead of calling synapse_client.delete() directly. This is inconsistent with how other entity types (File, Folder, Project) implement their delete_async methods. For consistency with the codebase, this should use synapse_client.delete(obj=self.id) wrapped in run_in_executor, similar to how Project.delete_async and Folder.delete_async work.

        loop = asyncio.get_event_loop()
        await loop.run_in_executor(
            None,
            lambda: delete(
                self.id,
                synapse_client=synapse_client,
            ),
        )

tests/integration/synapseclient/models/synchronous/test_docker.py:84

• The docker_repo created on line 79-84 is not scheduled for cleanup. This could leave test artifacts in Synapse. Add schedule_for_cleanup(docker_repo.id) after the store() call to ensure proper cleanup.

        docker_repo = DockerRepository(
            parent_id=project.id,
            repository_name="username/test",
            name="My Test Repo",
            description="A test repository with all fields",
        ).store(synapse_client=self.syn)

tests/unit/synapseclient/models/synchronous/unit_test_docker.py:33

• The type hint uses 'any' (lowercase) instead of 'Any' from typing. In Python, 'any' is a built-in function, not a type. This should be 'Dict[str, Any]' with 'Any' imported from typing, which follows Python typing conventions.

    def get_example_docker_output(self) -> dict[str, any]:

tests/integration/synapseclient/operations/synchronous/test_delete_operations.py:509

• The assertion is placed inside the pytest.raises context manager, which means it will never be executed. When DockerRepository.get() raises a SynapseHTTPError, the exception is caught and the code after it (the assertion) is not executed. The assertion should be moved outside the context manager block to check the exception message after it has been caught. This is a bug that prevents the test from verifying the correct error message.

        with pytest.raises(SynapseHTTPError) as e:
            DockerRepository(id=docker_repo.id).get(synapse_client=self.syn)
            assert f"404 Client Error: Entity {docker_repo.id} is in trash can" in str(
                e.value
            )

tests/integration/synapseclient/operations/async/test_delete_operations_async.py:535

• The assertion is placed inside the pytest.raises context manager, which means it will never be executed. When DockerRepository.get() raises a SynapseHTTPError, the exception is caught and the code after it (the assertion) is not executed. The assertion should be moved outside the context manager block to check the exception message after it has been caught. This is a bug that prevents the test from verifying the correct error message.

        with pytest.raises(SynapseHTTPError) as e:
            DockerRepository(id=docker_repo.id).get(synapse_client=self.syn)
            assert f"404 Client Error: Entity {docker_repo.id} is in trash can" in str(
                e.value
            )

tests/unit/synapseclient/models/synchronous/unit_test_docker.py:50

• The type hint uses 'any' (lowercase) instead of 'Any' from typing. In Python, 'any' is a built-in function, not a type. This should be 'Dict[str, Any]' with 'Any' imported from typing, which follows Python typing conventions.

    def get_example_bundle_response(self) -> dict[str, any]:

tests/unit/synapseclient/models/async/unit_test_docker_async.py:33

• The type hint uses 'any' (lowercase) instead of 'Any' from typing. In Python, 'any' is a built-in function, not a type. This should be 'Dict[str, Any]' with 'Any' imported from typing, which follows Python typing conventions.

    def get_example_docker_output(self) -> dict[str, any]:

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[linglp]

@BryanFauble only added in the async test because I think we are not going to add a sync test to prevent the test from running too long?