From 4bde0e754e1720f5e516fa96f4313af056745517 Mon Sep 17 00:00:00 2001 From: Rodrigo Goya Date: Tue, 29 Apr 2025 23:35:09 -0700 Subject: [PATCH] Forward args to _get_remote_config() and honour core/no_scm if present (#10719) * Forward args to _get_remote_config and honour core/no_scm if present * Pass full config, except cache, to Repo() * Avoid modifying original kwargs contents * Simplify if config * Add test * Relax error matching.. .. Avoid windows vs unix paths * Update dvc/repo/open_repo.py --------- Co-authored-by: skshetry <18718008+skshetry@users.noreply.github.com> --- dvc/dependency/repo.py | 4 ++++ dvc/repo/open_repo.py | 22 +++++++++++++++++++--- tests/func/api/test_data.py | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 3 deletions(-) diff --git a/dvc/dependency/repo.py b/dvc/dependency/repo.py index c49efca80..c1eb39c7d 100644 --- a/dvc/dependency/repo.py +++ b/dvc/dependency/repo.py @@ -146,6 +146,10 @@ class RepoDependency(Dependency): else: config = Config.load_file(conf) + # Setup config to the new DVCFileSystem to use the remote repo, but rely on the + # local cache instead of the remote's cache. This avoids re-streaming of data, + # but messes up the call to `_get_remote_config()` downstream, which will need + # to ignore cache parameters. config["cache"] = self.repo.config["cache"] config["cache"]["dir"] = self.repo.cache.local_cache_dir diff --git a/dvc/repo/open_repo.py b/dvc/repo/open_repo.py index 43efce100..490c05952 100644 --- a/dvc/repo/open_repo.py +++ b/dvc/repo/open_repo.py @@ -1,3 +1,4 @@ +import copy import os import tempfile import threading @@ -50,7 +51,7 @@ def open_repo(url, *args, **kwargs): if os.path.exists(url): url = os.path.abspath(url) try: - config = _get_remote_config(url) + config = _get_remote_config(url, *args, **kwargs) config.update(kwargs.get("config") or {}) kwargs["config"] = config return Repo(url, *args, **kwargs) @@ -97,9 +98,24 @@ def clean_repos(): _remove(path) -def _get_remote_config(url): +def _get_remote_config(url, *args, **kwargs): try: - repo = Repo(url) + # Deepcopy to prevent modifying the original `kwargs['config']` + config = copy.deepcopy(kwargs.get("config")) + + # Import operations will use this function to get the remote's cache. However, + # while the `url` sent will point to the external repo, the cache information + # in `kwargs["config"]["cache"]["dir"]`) will point to the local repo, + # see `dvc/dependency/repo.py:RepoDependency._make_fs()` + # + # This breaks this function, since we'd be instructing `Repo()` to use the wrong + # cache to being with. We need to remove the cache info from `kwargs["config"]` + # to read the actual remote repo data. + if config: + config.pop("cache", None) + + repo = Repo(url, config=config) + except NotDvcRepoError: return {} diff --git a/tests/func/api/test_data.py b/tests/func/api/test_data.py index 47d51c7dc..27fc365de 100644 --- a/tests/func/api/test_data.py +++ b/tests/func/api/test_data.py @@ -5,6 +5,7 @@ from funcy import first, get_in from dvc import api from dvc.exceptions import OutputNotFoundError, PathMissingError +from dvc.scm import CloneError, SCMError from dvc.testing.api_tests import TestAPI # noqa: F401 from dvc.testing.tmp_dir import make_subrepo from dvc.utils.fs import remove @@ -65,6 +66,38 @@ def test_get_url_from_remote(tmp_dir, erepo_dir, cloud, local_cloud): ) +def test_get_url_ignore_scm(tmp_dir, dvc, cloud, scm): + tmp_dir.add_remote(config=cloud.config) + tmp_dir.dvc_gen("foo", "foo", commit="add foo") + + repo_posix = tmp_dir.as_posix() + expected_url = (cloud / "files" / "md5" / "ac/bd18db4cc2f85cedef654fccc4a4d8").url + + # Test baseline with scm + assert api.get_url("foo", repo=repo_posix) == expected_url + + # Simulate gitless environment (e.g. deployed container) + (tmp_dir / ".git").rename(tmp_dir / "gitless_environment") + + # Test failure mode when trying to access with git + with pytest.raises(SCMError, match="is not a git repository"): + api.get_url("foo", repo=repo_posix) + + # Test successful access by ignoring git + assert ( + api.get_url("foo", repo=repo_posix, config={"core": {"no_scm": True}}) + == expected_url + ) + + # Addressing repos with `file://` triggers git, so it fails in a gitless environment + repo_url = f"file://{repo_posix}" + with pytest.raises( + CloneError, + match="SCM error", + ): + api.get_url("foo", repo=repo_url, config={"core": {"no_scm": True}}) + + def test_open_external(tmp_dir, erepo_dir, cloud): erepo_dir.add_remote(config=cloud.config)