Introduce guest.download() to centralise downloads on guest

tests/unit/artifact/test_file.py

@@ -60,7 +60,7 @@ def test_download_artifact(tmp_path, artifact_provider):
         guest,
         Path("/remote/foo-1.0-1.fc43.x86_64.rpm"),
     )
-    guest.execute.assert_called_once()
+    guest.download.assert_called_once()
 
 
 @pytest.mark.parametrize(

tests/unit/artifact/test_repository.py

@@ -230,24 +230,11 @@ def test_url_no_path(root_logger):
 # ================================================================================
 
 
-@pytest.fixture
-def mock_repo_file_fetch():
-    with patch(
-        'tmt.steps.prepare.artifact.providers.tmt.utils.retry_session'
-    ) as mock_retry_session:
-        mock_session = MagicMock()
-        mock_response = MagicMock()
-        mock_response.text = VALID_REPO_CONTENT
-        mock_response.raise_for_status.return_value = None
-        mock_session.get.return_value = mock_response
-        mock_retry_session.return_value.__enter__.return_value = mock_session
-        yield mock_session
-
-
 @pytest.fixture
 def mock_guest_and_pm(mock_package_manager):
     mock_guest = MagicMock()
     mock_guest.package_manager = mock_package_manager
+    mock_guest.execute.return_value.stdout = VALID_REPO_CONTENT
     return mock_guest, mock_package_manager
 
 
@@ -260,7 +247,7 @@ def test_id_extraction(artifact_provider):
     assert provider.id == "https://download.docker.com/linux/centos/docker-ce.repo"
 
 
-def test_artifacts_before_fetch(mock_repo_file_fetch, artifact_provider):
+def test_artifacts_before_fetch(artifact_provider):
     """Test that repository provider artifacts returns empty list"""
 
     provider = artifact_provider("repository-file:https://example.com/test.repo")
@@ -270,7 +257,7 @@ def test_artifacts_before_fetch(mock_repo_file_fetch, artifact_provider):
     assert provider.artifacts == []
 
 
-def test_fetch_contents(mock_repo_file_fetch, mock_guest_and_pm, artifact_provider, tmppath):
+def test_fetch_contents(mock_guest_and_pm, artifact_provider, tmppath):
     """Test that fetch_contents is a no-op for repository providers"""
 
     mock_guest, _ = mock_guest_and_pm
@@ -291,9 +278,7 @@ def test_fetch_contents(mock_repo_file_fetch, mock_guest_and_pm, artifact_provid
     assert len(artifacts) == 0
 
 
-def test_contribute_to_shared_repo(
-    mock_repo_file_fetch, mock_guest_and_pm, artifact_provider, tmppath
-):
+def test_contribute_to_shared_repo(mock_guest_and_pm, artifact_provider, tmppath):
     """Test that contribute_to_shared_repo does nothing for repository providers"""
 
     mock_guest, mock_package_manager = mock_guest_and_pm
@@ -316,7 +301,7 @@ def test_contribute_to_shared_repo(
     assert provider.artifacts == []
 
 
-def test_get_repositories(mock_repo_file_fetch, mock_guest_and_pm, artifact_provider, tmppath):
+def test_get_repositories(mock_guest_and_pm, artifact_provider, tmppath):
     """Test that get_repositories returns the initialized repository"""
 
     mock_guest, _ = mock_guest_and_pm

tmt/guest/__init__.py

@@ -114,6 +114,11 @@ def default_connect_waiting() -> Waiting:
 #: ``TMT_REBOOT_TIMEOUT``.
 REBOOT_TIMEOUT: int = configure_constant(DEFAULT_REBOOT_TIMEOUT, 'TMT_REBOOT_TIMEOUT')
 
+#: Number of attempts to download a file from a URL.
+DOWNLOAD_ATTEMPTS = 3
+#: Number of seconds to wait between download attempts.
+DOWNLOAD_INTERVAL = 5
+
 
 def default_reboot_waiting() -> Waiting:
     """
@@ -459,6 +464,12 @@ class RebootMode(enum.Enum):
 HardRebootModes = Literal[RebootMode.HARD]
 
 
+class DownloadError(tmt.utils.GeneralError):
+    """
+    Raised when download fails.
+    """
+
+
 class RebootModeNotSupportedError(ProvisionError):
     """A requested reboot mode is not supported by the guest"""
 
@@ -2389,6 +2400,35 @@ def pull(
 
         raise NotImplementedError
 
+    def download(self, url: str, destination: Path) -> None:
+        """
+        Download a file from a URL to a path on the guest.
+
+        :param url: URL to download from.
+        :param destination: path on the guest to save the file to.
+        :raises DownloadError: if the download fails after all attempts.
+        """
+
+        def _do_download() -> None:
+            self.execute(
+                ShellScript(
+                    f"{self.facts.sudo_prefix} curl -L --fail"
+                    f" -o {quote(str(destination))} {quote(url)}"
+                ),
+                silent=True,
+            )
+
+        try:
+            tmt.utils.retry(
+                _do_download,
+                DOWNLOAD_ATTEMPTS,
+                DOWNLOAD_INTERVAL,
+                f"Download '{url}'",
+                self._logger,
+            )
+        except tmt.utils.RetryError as error:
+            raise DownloadError(f"Failed to download '{url}' to '{destination}'.") from error
+
     @abc.abstractmethod
     def stop(self) -> None:
         """

tmt/package_managers/__init__.py

@@ -24,6 +24,10 @@
     PackageOrigin: TypeAlias = Union[str, 'SpecialPackageOrigin']
 
 
+#: Directory where DNF/YUM repository files are stored.
+YUM_REPOS_DIR = Path("/etc/yum.repos.d")
+
+
 @container(frozen=True)
 class Version:
     """

tmt/package_managers/dnf.py

@@ -4,6 +4,7 @@
 
 from tmt._compat.pathlib import Path
 from tmt.package_managers import (
+    YUM_REPOS_DIR,
     FileSystemPath,
     Installable,
     Options,
@@ -179,7 +180,7 @@ def install_debuginfo(
         return script
 
     def install_repository(self, repository: Repository) -> ShellScript:
-        repo_path = f"/etc/yum.repos.d/{repository.filename}"
+        repo_path = YUM_REPOS_DIR / repository.filename
         return ShellScript(
             f"{self.guest.facts.sudo_prefix} tee {repo_path} <<'EOF'\n{repository.content}\nEOF"
         )

tmt/steps/prepare/artifact/providers/__init__.py

@@ -14,7 +14,7 @@
 import tmt.utils
 from tmt._compat.typing import TypeAlias
 from tmt.container import container, simple_field
-from tmt.guest import Guest
+from tmt.guest import DownloadError, Guest
 from tmt.package_managers import Repository, Version
 from tmt.plugins import PluginRegistry
 from tmt.utils import Path, ShellScript
@@ -24,12 +24,6 @@
 )
 
 
-class DownloadError(tmt.utils.GeneralError):
-    """
-    Raised when download fails.
-    """
-
-
 class UnsupportedOperationError(RuntimeError):
     """
     Raised when an operation is intentionally unsupported by a provider.
@@ -141,18 +135,22 @@ def artifacts(self) -> Sequence[ArtifactInfo]:
 
         return self._artifacts
 
-    @abstractmethod
     def _download_artifact(
         self, artifact: ArtifactInfo, guest: Guest, destination: tmt.utils.Path
     ) -> None:
         """
         Download a single artifact to the specified destination on a given guest.
 
+        :param artifact: the artifact to download.
         :param guest: the guest on which the artifact should be downloaded.
         :param destination: path into which the artifact should be downloaded.
+        :raises DownloadError: if the download fails.
         """
 
-        raise NotImplementedError
+        try:
+            guest.download(artifact.location, destination)
+        except tmt.utils.GeneralError as error:
+            raise DownloadError(f"Failed to download '{artifact}'.") from error
 
     def fetch_contents(
         self,

tmt/steps/prepare/artifact/providers/copr_build.py

@@ -19,7 +19,6 @@
     ArtifactInfo,
     ArtifactProvider,
     ArtifactProviderId,
-    DownloadError,
     provides_artifact_provider,
 )
 from tmt.utils import ShellScript
@@ -136,20 +135,6 @@ def _extract_provider_id(cls, raw_id: str) -> ArtifactProviderId:
             ) from error
         return value
 
-    def _download_artifact(
-        self, artifact: ArtifactInfo, guest: Guest, destination: tmt.utils.Path
-    ) -> None:
-        try:
-            # Destination directory is guaranteed to exist, download the artifact
-            guest.execute(
-                tmt.utils.ShellScript(
-                    f"curl -L --fail -o {quote(str(destination))} {quote(artifact.location)}"
-                ),
-                silent=True,
-            )
-        except Exception as error:
-            raise DownloadError(f"Failed to download '{artifact}'.") from error
-
     @cached_property
     def result_url(self) -> str:
         """

tmt/steps/prepare/artifact/providers/copr_repository.py

@@ -10,6 +10,7 @@
 from tmt._compat.typing import Self
 from tmt.container import container
 from tmt.guest import Guest
+from tmt.package_managers import YUM_REPOS_DIR
 from tmt.steps.prepare.artifact.providers import (
     ArtifactInfo,
     ArtifactProvider,
@@ -114,9 +115,7 @@ def fetch_contents(
         repo_filename = f"_copr:copr.fedorainfracloud.org:{owner}:{repo.project}.repo"
 
         try:
-            output = guest.execute(
-                tmt.utils.Command("cat", Path(f"/etc/yum.repos.d/{repo_filename}"))
-            )
+            output = guest.execute(tmt.utils.Command("cat", YUM_REPOS_DIR / repo_filename))
         except tmt.utils.RunError as error:
             raise tmt.utils.PrepareError(
                 f"Failed to read '{repo_filename}' from the guest."

tmt/steps/prepare/artifact/providers/file.py

@@ -109,12 +109,7 @@ def _download_artifact(
     ) -> None:
         try:
             if self._is_url:  # Remote file, download it
-                guest.execute(
-                    tmt.utils.ShellScript(
-                        f"curl -L --fail -o {quote(str(destination))} {quote(artifact.location)}"
-                    ),
-                    silent=True,
-                )
+                guest.download(artifact.location, destination)
             else:  # Local file, push it to the guest
                 # When pushing a single file, use recursive=False. The default recursive=True
                 # treats the source as a directory (appending "/."), which only works for

tmt/steps/prepare/artifact/providers/koji.py

@@ -20,7 +20,6 @@
     ArtifactInfo,
     ArtifactProvider,
     ArtifactProviderId,
-    DownloadError,
     provides_artifact_provider,
 )
 from tmt.utils import ShellScript
@@ -178,27 +177,6 @@ def _extract_provider_id(cls, raw_id: str) -> ArtifactProviderId:
             ) from exc
         return value
 
-    def _download_artifact(
-        self, artifact: ArtifactInfo, guest: Guest, destination: tmt.utils.Path
-    ) -> None:
-        """
-        Download the specified artifact to the given destination on the guest.
-
-        :param artifact: The artifact to download
-        :param guest: The guest where the artifact should be downloaded
-        :param destination: The destination path on the guest
-        """
-        try:
-            # Destination directory is guaranteed to exist, download the artifact
-            guest.execute(
-                tmt.utils.ShellScript(
-                    f"curl -L --fail -o {quote(str(destination))} {quote(artifact.location)}"
-                ),
-                silent=True,
-            )
-        except Exception as error:
-            raise DownloadError(f"Failed to download '{artifact}'.") from error
-
     def contribute_to_shared_repo(
         self,
         guest: Guest,

tmt/steps/prepare/artifact/providers/repository.py

@@ -9,7 +9,8 @@
 import tmt.log
 import tmt.utils
 from tmt.container import container, simple_field
-from tmt.guest import Guest
+from tmt.guest import DownloadError, Guest
+from tmt.package_managers import YUM_REPOS_DIR
 from tmt.steps import DefaultNameGenerator
 from tmt.steps.prepare.artifact.providers import (
     ArtifactInfo,
@@ -75,18 +76,28 @@ def fetch_contents(
         self.logger.info(f"Initializing repository provider with URL: {self.id}")
 
         parsed = urlparse(self.id)
+        parsed_path = Path(parsed.path)
         if parsed.scheme == 'file':
             # Read .repo file from the local (controller) filesystem.
-            self.logger.info(f"Reading repository file from local path: {parsed.path}")
-            self.logger.debug(
-                f"Absolute path of the repository file: '{Path(parsed.path).resolve()}'"
-            )
-            self.repository = Repository.from_file_path(
-                file_path=Path(parsed.path), logger=self.logger
-            )
+            self.logger.info(f"Reading repository file from local path: {parsed_path}")
+            self.logger.debug(f"Absolute path of the repository file: '{parsed_path.resolve()}'")
+            self.repository = Repository.from_file_path(file_path=parsed_path, logger=self.logger)
         else:
-            # TODO: This should not be using Repository.from_url
-            self.repository = Repository.from_url(url=self.id, logger=self.logger)
+            repo_filename = parsed_path.name
+            remote_path = YUM_REPOS_DIR / repo_filename
+            try:
+                guest.download(self.id, remote_path)
+            except DownloadError as error:
+                raise PrepareError(
+                    f"Failed to download repository file '{self.id}' to the guest."
+                ) from error
+            try:
+                output = guest.execute(tmt.utils.Command("cat", remote_path))
+            except tmt.utils.RunError as error:
+                raise PrepareError(f"Failed to read '{repo_filename}' from the guest.") from error
+            self.repository = Repository.from_content(
+                output.stdout or '', repo_filename.removesuffix('.repo'), self.logger
+            )
 
         self.logger.info(
             f"Repository initialized: {self.repository.name} "