Refactor tagging: create functions, better logs and names, textwrap.dedent (#2239)

Author: mathbunnyru

tagging/apps/apply_tags.py

@@ -15,9 +15,6 @@
 
 
 def apply_tags(config: Config) -> None:
-    """
-    Tags <config.full_image()> with the tags reported by all taggers for this image
-    """
     LOGGER.info(f"Tagging image: {config.image}")
 
     file_prefix = get_file_prefix_for_platform(config.platform, config.variant)
@@ -28,6 +25,8 @@ def apply_tags(config: Config) -> None:
         LOGGER.info(f"Applying tag: {tag}")
         docker["tag", config.full_image(), tag] & plumbum.FG
 
+    LOGGER.info(f"All tags applied to image: {config.image}")
+
 
 if __name__ == "__main__":
     logging.basicConfig(level=logging.INFO)

tagging/apps/merge_tags.py

@@ -15,24 +15,31 @@
 LOGGER = logging.getLogger(__name__)
 
 
-def merge_tags(config: Config) -> None:
-    """
-    Merge tags for x86_64 and aarch64 images when possible.
-    """
-    LOGGER.info(f"Merging tags for image: {config.image}")
-
-    all_tags: set[str] = set()
+def read_tags_from_files(config: Config) -> set[str]:
+    LOGGER.info(f"Read tags from file(s) for image: {config.image}")
 
+    tags: set[str] = set()
     for platform in ALL_PLATFORMS:
+        LOGGER.info(f"Reading tags for platform: {platform}")
+
         file_prefix = get_file_prefix_for_platform(platform, config.variant)
         filename = f"{file_prefix}-{config.image}.txt"
-        file_path = config.tags_dir / filename
-        if file_path.exists():
-            tags = file_path.read_text().splitlines()
-            all_tags.update(tag.replace(platform + "-", "") for tag in tags)
+        path = config.tags_dir / filename
+        if path.exists():
+            LOGGER.info(f"Tag file: {path} found")
+            lines = path.read_text().splitlines()
+            tags.update(tag.replace(platform + "-", "") for tag in lines)
+        else:
+            LOGGER.info(f"Tag file: {path} doesn't exist")
+
+    LOGGER.info(f"Tags read for image: {config.image}")
+    return tags
 
-    LOGGER.info(f"Got tags: {all_tags}")
 
+def merge_tags(config: Config) -> None:
+    LOGGER.info(f"Merging tags for image: {config.image}")
+
+    all_tags = read_tags_from_files(config)
     for tag in all_tags:
         LOGGER.info(f"Trying to merge tag: {tag}")
         existing_images = []
@@ -51,8 +58,11 @@ def merge_tags(config: Config) -> None:
         LOGGER.info(f"Found images: {existing_images}")
         docker["manifest", "create", tag][existing_images] & plumbum.FG
         docker["manifest", "push", tag] & plumbum.FG
+
         LOGGER.info(f"Successfully merged and pushed tag: {tag}")
 
+    LOGGER.info(f"All tags merged for image: {config.image}")
+
 
 if __name__ == "__main__":
     logging.basicConfig(level=logging.INFO)

tagging/apps/write_manifest.py

@@ -7,11 +7,9 @@
 from docker.models.containers import Container
 
 from tagging.apps.common_cli_arguments import common_arguments_parser
-from tagging.hierarchy.get_taggers_and_manifests import (
-    get_taggers_and_manifests,
-)
+from tagging.hierarchy.get_manifests import get_manifests
+from tagging.hierarchy.get_taggers import get_taggers
 from tagging.manifests.build_info import BuildInfo
-from tagging.manifests.manifest_interface import ManifestInterface
 from tagging.utils.config import Config
 from tagging.utils.docker_runner import DockerRunner
 from tagging.utils.get_prefix import get_file_prefix, get_tag_prefix
@@ -24,10 +22,12 @@
 MARKDOWN_LINE_BREAK = "<br />"
 
 
-def write_build_history_line(
-    config: Config, filename: str, all_tags: list[str]
-) -> None:
-    LOGGER.info("Appending build history line")
+def get_build_history_line(config: Config, filename: str, container: Container) -> str:
+    LOGGER.info(f"Calculating build history line for image: {config.image}")
+
+    taggers = get_taggers(config.image)
+    tags_prefix = get_tag_prefix(config.variant)
+    all_tags = [tags_prefix + "-" + tagger.tag_value(container) for tagger in taggers]
 
     date_column = f"`{BUILD_TIMESTAMP}`"
     image_column = MARKDOWN_LINE_BREAK.join(
@@ -42,19 +42,28 @@ def write_build_history_line(
         ]
     )
     build_history_line = f"| {date_column} | {image_column} | {links_column} |"
-    config.hist_lines_dir.mkdir(parents=True, exist_ok=True)
-    file = config.hist_lines_dir / f"{filename}.txt"
-    file.write_text(build_history_line)
-    LOGGER.info(f"Build history line written to: {file}")
-
-
-def write_manifest_file(
-    config: Config,
-    filename: str,
-    commit_hash_tag: str,
-    manifests: list[ManifestInterface],
-    container: Container,
+
+    LOGGER.info(f"Build history line calculated for image: {config.image}")
+    return build_history_line
+
+
+def write_build_history_line(
+    config: Config, filename: str, container: Container
 ) -> None:
+    LOGGER.info(f"Writing tags for image: {config.image}")
+
+    path = config.hist_lines_dir / f"{filename}.txt"
+    path.parent.mkdir(parents=True, exist_ok=True)
+    build_history_line = get_build_history_line(config, filename, container)
+    path.write_text(build_history_line)
+
+    LOGGER.info(f"Build history line written to: {path}")
+
+
+def get_manifest(config: Config, commit_hash_tag: str, container: Container) -> str:
+    LOGGER.info(f"Calculating manifest file for image: {config.image}")
+
+    manifests = get_manifests(config.image)
     manifest_names = [manifest.__class__.__name__ for manifest in manifests]
     LOGGER.info(f"Using manifests: {manifest_names}")
 
@@ -65,27 +74,35 @@ def write_manifest_file(
     ]
     markdown_content = "\n\n".join(markdown_pieces) + "\n"
 
-    config.manifests_dir.mkdir(parents=True, exist_ok=True)
-    file = config.manifests_dir / f"{filename}.md"
-    file.write_text(markdown_content)
-    LOGGER.info(f"Manifest file written to: {file}")
+    LOGGER.info(f"Manifest file calculated for image: {config.image}")
+    return markdown_content
+
+
+def write_manifest(
+    config: Config, filename: str, commit_hash_tag: str, container: Container
+) -> None:
+    LOGGER.info(f"Writing manifest file for image: {config.image}")
 
+    path = config.manifests_dir / f"{filename}.md"
+    path.parent.mkdir(parents=True, exist_ok=True)
+    manifest = get_manifest(config, commit_hash_tag, container)
+    path.write_text(manifest)
 
-def write_manifest(config: Config) -> None:
-    LOGGER.info(f"Creating manifests for image: {config.image}")
-    taggers, manifests = get_taggers_and_manifests(config.image)
+    LOGGER.info(f"Manifest file wrtitten to: {path}")
+
+
+def write_all(config: Config) -> None:
+    LOGGER.info(f"Writing all files for image: {config.image}")
 
     file_prefix = get_file_prefix(config.variant)
     commit_hash_tag = GitHelper.commit_hash_tag()
     filename = f"{file_prefix}-{config.image}-{commit_hash_tag}"
 
     with DockerRunner(config.full_image()) as container:
-        tags_prefix = get_tag_prefix(config.variant)
-        all_tags = [
-            tags_prefix + "-" + tagger.tag_value(container) for tagger in taggers
-        ]
-        write_build_history_line(config, filename, all_tags)
-        write_manifest_file(config, filename, commit_hash_tag, manifests, container)
+        write_build_history_line(config, filename, container)
+        write_manifest(config, filename, commit_hash_tag, container)
+
+    LOGGER.info(f"All files written for image: {config.image}")
 
 
 if __name__ == "__main__":
@@ -101,4 +118,4 @@ def write_manifest(config: Config) -> None:
         manifests_dir=True,
         repository=True,
     )
-    write_manifest(config)
+    write_all(config)

tagging/apps/write_tags_file.py

@@ -4,26 +4,18 @@
 import logging
 
 from tagging.apps.common_cli_arguments import common_arguments_parser
-from tagging.hierarchy.get_taggers_and_manifests import (
-    get_taggers_and_manifests,
-)
+from tagging.hierarchy.get_taggers import get_taggers
 from tagging.utils.config import Config
 from tagging.utils.docker_runner import DockerRunner
 from tagging.utils.get_prefix import get_file_prefix, get_tag_prefix
 
 LOGGER = logging.getLogger(__name__)
 
 
-def write_tags_file(config: Config) -> None:
-    """
-    Writes tags file for the image {config.full_image()}
-    """
-    LOGGER.info(f"Tagging image: {config.image}")
-    taggers, _ = get_taggers_and_manifests(config.image)
-
-    file_prefix = get_file_prefix(config.variant)
-    filename = f"{file_prefix}-{config.image}.txt"
+def get_tags(config: Config) -> list[str]:
+    LOGGER.info(f"Calculating tags for image: {config.image}")
 
+    taggers = get_taggers(config.image)
     tags_prefix = get_tag_prefix(config.variant)
     tags = [f"{config.full_image()}:{tags_prefix}-latest"]
     with DockerRunner(config.full_image()) as container:
@@ -34,10 +26,22 @@ def write_tags_file(config: Config) -> None:
                 f"Calculated tag, tagger_name: {tagger_name} tag_value: {tag_value}"
             )
             tags.append(f"{config.full_image()}:{tags_prefix}-{tag_value}")
-    config.tags_dir.mkdir(parents=True, exist_ok=True)
-    file = config.tags_dir / filename
-    file.write_text("\n".join(tags))
-    LOGGER.info(f"Tags file written to: {file}")
+
+    LOGGER.info(f"Tags calculated for image: {config.image}")
+    return tags
+
+
+def write_tags_file(config: Config) -> None:
+    LOGGER.info(f"Writing tags for image: {config.image}")
+
+    file_prefix = get_file_prefix(config.variant)
+    filename = f"{file_prefix}-{config.image}.txt"
+    path = config.tags_dir / filename
+    path.parent.mkdir(parents=True, exist_ok=True)
+    tags = get_tags(config)
+    path.write_text("\n".join(tags))
+
+    LOGGER.info(f"Tags wrtitten to: {path}")
 
 
 if __name__ == "__main__":

tagging/hierarchy/get_manifests.py

@@ -0,0 +1,12 @@
+# Copyright (c) Jupyter Development Team.
+# Distributed under the terms of the Modified BSD License.
+from tagging.hierarchy.images_hierarchy import ALL_IMAGES
+from tagging.manifests.manifest_interface import ManifestInterface
+
+
+def get_manifests(image: str | None) -> list[ManifestInterface]:
+    if image is None:
+        return []
+    image_description = ALL_IMAGES[image]
+    parent_manifests = get_manifests(image_description.parent_image)
+    return parent_manifests + image_description.manifests

tagging/hierarchy/get_taggers.py

@@ -0,0 +1,12 @@
+# Copyright (c) Jupyter Development Team.
+# Distributed under the terms of the Modified BSD License.
+from tagging.hierarchy.images_hierarchy import ALL_IMAGES
+from tagging.taggers.tagger_interface import TaggerInterface
+
+
+def get_taggers(image: str | None) -> list[TaggerInterface]:
+    if image is None:
+        return []
+    image_description = ALL_IMAGES[image]
+    parent_taggers = get_taggers(image_description.parent_image)
+    return parent_taggers + image_description.taggers

tagging/hierarchy/get_taggers_and_manifests.py

@@ -1,5 +1,7 @@
 # Copyright (c) Jupyter Development Team.
 # Distributed under the terms of the Modified BSD License.
+import textwrap
+
 from docker.models.containers import Container
 
 from tagging.utils.docker_runner import DockerRunner
@@ -14,9 +16,11 @@ def quoted_output(container: Container, cmd: str) -> str:
 
     assert cmd_output, f"Command `{cmd}` returned empty output"
 
-    return f"""\
-`{cmd}`:
+    return textwrap.dedent(
+        f"""\
+        `{cmd}`:
 
-```text
-{cmd_output}
-```"""
+        ```text
+        {cmd_output}
+        ```"""
+    )