Reduce verbosity in mirrors.yaml (#34210)

Ensure `spack mirror add <name> <url/path>` without further arguments translates to `<name>: <url>` key value pairs in mirrors.yaml. If --s3-* flags are provided, only store the provided ones. 

Co-authored-by: Harmen Stoppels <me@harmenstoppels.nl>

lib/spack/spack/ci.py

@@ -727,13 +727,19 @@ def generate_gitlab_ci_yaml(
         # --check-index-only, then the override mirror needs to be added to
         # the configured mirrors when bindist.update() is run, or else we
         # won't fetch its index and include in our local cache.
-        spack.mirror.add("ci_pr_mirror", remote_mirror_override, cfg.default_modify_scope())
+        spack.mirror.add(
+            spack.mirror.Mirror(remote_mirror_override, name="ci_pr_mirror"),
+            cfg.default_modify_scope(),
+        )
 
     shared_pr_mirror = None
     if spack_pipeline_type == "spack_pull_request":
         stack_name = os.environ.get("SPACK_CI_STACK_NAME", "")
         shared_pr_mirror = url_util.join(SHARED_PR_MIRROR_URL, stack_name)
-        spack.mirror.add("ci_shared_pr_mirror", shared_pr_mirror, cfg.default_modify_scope())
+        spack.mirror.add(
+            spack.mirror.Mirror(shared_pr_mirror, name="ci_shared_pr_mirror"),
+            cfg.default_modify_scope(),
+        )
 
     pipeline_artifacts_dir = artifacts_root
     if not pipeline_artifacts_dir:

lib/spack/spack/cmd/ci.py

@@ -453,9 +453,8 @@ def ci_rebuild(args):
     # mirror now so it's used when we check for a hash match already
     # built for this spec.
     if pipeline_mirror_url:
-        spack.mirror.add(
+        mirror = spack.mirror.Mirror(pipeline_mirror_url, name=spack_ci.TEMP_STORAGE_MIRROR_NAME)
-            spack_ci.TEMP_STORAGE_MIRROR_NAME, pipeline_mirror_url, cfg.default_modify_scope()
+        spack.mirror.add(mirror, cfg.default_modify_scope())
-        )
         pipeline_mirrors.append(pipeline_mirror_url)
 
     # Check configured mirrors for a built spec with a matching hash
@@ -470,7 +469,10 @@ def ci_rebuild(args):
             # could be installed from either the override mirror or any other configured
             # mirror (e.g. remote_mirror_url which is defined in the environment or
             # pipeline_mirror_url), which is also what we want.
-            spack.mirror.add("mirror_override", remote_mirror_override, cfg.default_modify_scope())
+            spack.mirror.add(
+                spack.mirror.Mirror(remote_mirror_override, name="mirror_override"),
+                cfg.default_modify_scope(),
+            )
         pipeline_mirrors.append(remote_mirror_override)
 
     if spack_pipeline_type == "spack_pull_request":

lib/spack/spack/cmd/mirror.py

@@ -145,7 +145,26 @@ def setup_parser(subparser):
 
 def mirror_add(args):
     """Add a mirror to Spack."""
-    spack.mirror.add(args.name, args.url, args.scope, args)
+    if (
+        args.s3_access_key_id
+        or args.s3_access_key_secret
+        or args.s3_access_token
+        or args.s3_profile
+        or args.s3_endpoint_url
+    ):
+        connection = {"url": args.url}
+        if args.s3_access_key_id and args.s3_access_key_secret:
+            connection["access_pair"] = (args.s3_access_key_id, args.s3_access_key_secret)
+        if args.s3_access_token:
+            connection["access_token"] = args.s3_access_token
+        if args.s3_profile:
+            connection["profile"] = args.s3_profile
+        if args.s3_endpoint_url:
+            connection["endpoint_url"] = args.s3_endpoint_url
+        mirror = spack.mirror.Mirror(fetch_url=connection, push_url=connection, name=args.name)
+    else:
+        mirror = spack.mirror.Mirror(args.url, name=args.name)
+    spack.mirror.add(mirror, args.scope)
 
 
 def mirror_remove(args):

lib/spack/spack/mirror.py

@@ -119,6 +119,10 @@ def from_url(url: str):
         return Mirror(fetch_url=url)
 
     def to_dict(self):
+        # Keep it a key-value pair <name>: <url> when possible.
+        if isinstance(self._fetch_url, str) and self._push_url is None:
+            return self._fetch_url
+
         if self._push_url is None:
             return syaml_dict([("fetch", self._fetch_url), ("push", self._fetch_url)])
         else:
@@ -556,30 +560,17 @@ def mirror_cache_and_stats(path, skip_unstable_versions=False):
     return mirror_cache, mirror_stats
 
 
-def add(name, url, scope, args={}):
+def add(mirror: Mirror, scope=None):
     """Add a named mirror in the given scope"""
     mirrors = spack.config.get("mirrors", scope=scope)
     if not mirrors:
         mirrors = syaml_dict()
 
-    if name in mirrors:
+    if mirror.name in mirrors:
-        tty.die("Mirror with name %s already exists." % name)
+        tty.die("Mirror with name {} already exists.".format(mirror.name))
 
     items = [(n, u) for n, u in mirrors.items()]
-    mirror_data = url
+    items.insert(0, (mirror.name, mirror.to_dict()))
-    key_values = ["s3_access_key_id", "s3_access_token", "s3_profile"]
-    # On creation, assume connection data is set for both
-    if any(value for value in key_values if value in args):
-        url_dict = {
-            "url": url,
-            "access_pair": (args.s3_access_key_id, args.s3_access_key_secret),
-            "access_token": args.s3_access_token,
-            "profile": args.s3_profile,
-            "endpoint_url": args.s3_endpoint_url,
-        }
-        mirror_data = {"fetch": url_dict, "push": url_dict}
-
-    items.insert(0, (name, mirror_data))
     mirrors = syaml_dict(items)
     spack.config.set("mirrors", mirrors, scope=scope)