Refactor setup_remotes() to remove --remote overriding logicgokcennurlu/remote_url_override_push_error

This change simplifies the responsibilities of `setup_remotes()` by extracting
"getting the default remotes" logic to its own function and moving "--remote"
related logic to _load(). The latter, especially, helps to remove the need of
passing the "remote_url" string and "can_push?" boolean to `setup_remotes()`.

4 files changed, 52 insertions, 36 deletions

diff --git a/buildstream/_artifactcache/artifactcache.py b/buildstream/_artifactcache/artifactcache.py
index 9c9a8484f..68fb62014 100644
--- a/buildstream/_artifactcache/artifactcache.py
+++ b/buildstream/_artifactcache/artifactcache.py
@@ -110,37 +110,42 @@ class ArtifactCache():
         # assume project and element names are not allowed to contain slashes
         return '{0}/{1}/{2}'.format(project.name, element_name, key)
 
+    # get_remotes_from_projects()
+    #
+    # Generates list artifact caches based on project configuration
+    #
+    # Returns:
+    #    (list of (list of ArtifactCacheSpec, Project)): Configurations each are
+    #        ready to be consumed by `self._set_remotes()`
+    #
+    # This requires that all of the projects which are to be processed in the session
+    # have already been loaded and are observable in the Context.
+    #
+    def get_remotes_from_projects(self):
+        return [
+            (_configured_remote_artifact_cache_specs(self.context, proj), proj)
+            for proj in self.context.get_projects()
+        ]
+
     # setup_remotes():
     #
     # Sets up which remotes to use
     #
     # Args:
-    #    use_config (bool): Whether to use project configuration
-    #    remote_url (str): Remote artifact cache URL
-    #    push (bool): Whether to enabe push for `remote_url`
+    #    remotes (list of (list of ArtifactCacheSpec, Project)): Configurations which are
+    #        ready to be consumed by `self._set_remotes()`
     #
     # This requires that all of the projects which are to be processed in the session
     # have already been loaded and are observable in the Context.
     #
-    def setup_remotes(self, *, use_config=False, remote_url=None, push=False):
-
+    def setup_remotes(self, *, remotes=None):
         # Ensure we do not double-initialise since this can be expensive
         assert not self._remotes_setup
         self._remotes_setup = True
 
-        # Initialize remote artifact caches. We allow the commandline to override
-        # the user config in some cases (for example `bst push --remote=...`).
-        has_remote_caches = False
-        if remote_url:
-            self._set_remotes([ArtifactCacheSpec(remote_url, push=push)])
-            has_remote_caches = True
-        if use_config:
-            for project in self.context.get_projects():
-                artifact_caches = _configured_remote_artifact_cache_specs(self.context, project)
-                if artifact_caches:  # artifact_caches is a list of ArtifactCacheSpec instances
-                    self._set_remotes(artifact_caches, project=project)
-                    has_remote_caches = True
-        if has_remote_caches:
+        if remotes:
+            for caches, project in remotes:
+                self._set_remotes(caches, project=project)
             self._initialize_remotes()
 
     # specs_from_config_node()
diff --git a/buildstream/_stream.py b/buildstream/_stream.py
index a00c4e4ca..81ea1ac3a 100644
--- a/buildstream/_stream.py
+++ b/buildstream/_stream.py
@@ -28,6 +28,7 @@ import tarfile
 import tempfile
 from contextlib import contextmanager, suppress
 
+from ._artifactcache import ArtifactCacheSpec
 from ._exceptions import StreamError, ImplError, BstError, set_last_task_error
 from ._message import Message, MessageType
 from ._scheduler import Scheduler, SchedStatus, TrackQueue, FetchQueue, BuildQueue, PullQueue, PushQueue
@@ -934,14 +935,21 @@ class Stream():
             self._pipeline.resolve_elements(track_selected)
             return [], track_selected
 
-        # ArtifactCache.setup_remotes expects all projects to be fully loaded
-        for project in self._context.get_projects():
-            project.ensure_fully_loaded()
-
+        remotes = []
+        if use_artifact_config:
+            # ArtifactCache.get_remotes_from_projects expects all projects to be
+            # fully loaded
+            for project in self._context.get_projects():
+                project.ensure_fully_loaded()
+            remotes = self._artifacts.get_remotes_from_projects()
+        elif artifact_remote_url is not None:
+            # Build the ArtifactCacheSpec instance based on `--remote`
+            remotes = [(
+                [ArtifactCacheSpec(artifact_remote_url, push=artifact_remote_can_push)],
+                None
+            )]
         # Connect to remote caches, this needs to be done before resolving element state
-        self._artifacts.setup_remotes(use_config=use_artifact_config,
-                                      remote_url=artifact_remote_url,
-                                      push=artifact_remote_can_push)
+        self._artifacts.setup_remotes(remotes=remotes)
 
         # Now move on to loading primary selection.
         #
diff --git a/tests/artifactcache/pull.py b/tests/artifactcache/pull.py
index 4c332bf36..ab8262bc8 100644
--- a/tests/artifactcache/pull.py
+++ b/tests/artifactcache/pull.py
@@ -146,7 +146,8 @@ def _test_pull(user_config_file, project_dir, artifact_dir,
     element = project.load_elements([element_name])[0]
 
     # Manually setup the CAS remote
-    cas.setup_remotes(use_config=True)
+    remotes = cas.get_remotes_from_projects()
+    cas.setup_remotes(remotes=remotes)
 
     if cas.has_push_remotes(element=element):
         # Push the element's artifact
@@ -284,7 +285,8 @@ def _test_push_tree(user_config_file, project_dir, artifact_dir, artifact_digest
     cas = artifactcache.cas
 
     # Manually setup the CAS remote
-    artifactcache.setup_remotes(use_config=True)
+    remotes = artifactcache.get_remotes_from_projects()
+    artifactcache.setup_remotes(remotes=remotes)
 
     if artifactcache.has_push_remotes():
         directory = remote_execution_pb2.Directory()
@@ -319,7 +321,8 @@ def _test_pull_tree(user_config_file, project_dir, artifact_dir, artifact_digest
     cas = context.artifactcache
 
     # Manually setup the CAS remote
-    cas.setup_remotes(use_config=True)
+    remotes = cas.get_remotes_from_projects()
+    cas.setup_remotes(remotes=remotes)
 
     if cas.has_push_remotes():
         # Pull the artifact using the Tree object
diff --git a/tests/artifactcache/push.py b/tests/artifactcache/push.py
index 116fa7865..6b25a193f 100644
--- a/tests/artifactcache/push.py
+++ b/tests/artifactcache/push.py
@@ -125,8 +125,8 @@ def _test_push(user_config_file, project_dir, artifact_dir,
     element = project.load_elements([element_name])[0]
 
     # Manually setup the CAS remote
-    cas.setup_remotes(use_config=True)
-    cas.initialize_remotes()
+    remotes = cas.get_remotes_from_projects()
+    cas.setup_remotes(remotes=remotes)
 
     if cas.has_push_remotes(element=element):
         # Push the element's artifact
@@ -185,8 +185,8 @@ def test_push_directory(cli, tmpdir, datafiles):
         assert artifactcache.contains(element, element_key)
 
         # Manually setup the CAS remote
-        artifactcache.setup_remotes(use_config=True)
-        artifactcache.initialize_remotes()
+        remotes = artifactcache.get_remotes_from_projects()
+        artifactcache.setup_remotes(remotes=remotes)
         assert artifactcache.has_push_remotes(element=element)
 
         # Recreate the CasBasedDirectory object from the cached artifact
@@ -231,8 +231,8 @@ def _test_push_directory(user_config_file, project_dir, artifact_dir, artifact_d
     cas = context.artifactcache
 
     # Manually setup the CAS remote
-    cas.setup_remotes(use_config=True)
-    cas.initialize_remotes()
+    remotes = cas.get_remotes_from_projects()
+    cas.setup_remotes(remotes=remotes)
 
     if cas.has_push_remotes():
         # Create a CasBasedDirectory from local CAS cache content
@@ -307,8 +307,8 @@ def _test_push_message(user_config_file, project_dir, artifact_dir, queue):
     cas = context.artifactcache
 
     # Manually setup the CAS remote
-    cas.setup_remotes(use_config=True)
-    cas.initialize_remotes()
+    remotes = cas.get_remotes_from_projects()
+    cas.setup_remotes(remotes=remotes)
 
     if cas.has_push_remotes():
         # Create an example message object