Add type checking for build_workflow (#1342)

* Add type checking for build_workflow

Signed-off-by: Yilin Zhang <[email protected]>

* Revert "Merge remote-tracking branch 'upstream/main' into type_checking_build_workflow"

This reverts commit 24b3083, reversing
changes made to 3c82ae0.

Signed-off-by: Yilin Zhang <[email protected]>

* Revert "Merge remote-tracking branch 'upstream/main' into type_checking_build_workflow"

This reverts commit 24b3083, reversing
changes made to 3c82ae0.

Signed-off-by: Yilin Zhang <[email protected]>

.github/workflows/python-tests.yml

@@ -31,7 +31,7 @@ jobs:
           pipenv run flake8 .
       - name: Run Type Checker
         run: |
-          pipenv run mypy src/checkout_workflow tests/tests_checkout_workflow src/run_assemble.py tests/test_run_assemble.py src/assemble_workflow tests/tests_assemble_workflow src/manifests tests/tests_manifests
+          pipenv run mypy src/build_workflow tests/tests_build_workflow src/checkout_workflow tests/tests_checkout_workflow src/run_assemble.py tests/test_run_assemble.py src/assemble_workflow tests/tests_assemble_workflow src/manifests tests/tests_manifests
       - name: Run Tests with Coverage
         run: |
           pipenv run coverage run -m pytest --cov=./src --cov-report=xml

.pre-commit-config.yaml

@@ -18,7 +18,7 @@ repos:
   - id: mypy
     stages: [commit]
     name: mypy
-    entry: bash -c 'pipenv run mypy src/checkout_workflow tests/tests_checkout_workflow src/run_assemble.py tests/test_run_assemble.py src/assemble_workflow tests/tests_assemble_workflow src/manifests tests/tests_manifests'
+    entry: bash -c 'pipenv run mypy src/build_workflow tests/tests_build_workflow src/checkout_workflow tests/tests_checkout_workflow src/run_assemble.py tests/test_run_assemble.py src/assemble_workflow tests/tests_assemble_workflow src/manifests tests/tests_manifests'
     language: system
   - id: pytest
     stages: [commit]

src/build_workflow/build_args.py

@@ -7,6 +7,7 @@
 import argparse
 import logging
 import sys
+from typing import IO
 
 
 class BuildArgs:
@@ -16,14 +17,14 @@ class BuildArgs:
         "arm64",
     ]
 
-    manifest: str
+    manifest: IO
     snapshot: bool
     component: str
     keep: bool
     platform: str
     architecture: str
 
-    def __init__(self):
+    def __init__(self) -> None:
         parser = argparse.ArgumentParser(description="Build an OpenSearch Bundle")
         parser.add_argument("manifest", type=argparse.FileType("r"), help="Manifest file.")
         parser.add_argument(
@@ -71,7 +72,7 @@ def __init__(self):
         self.architecture = args.architecture
         self.script_path = sys.argv[0].replace("/src/run_build.py", "/build.sh")
 
-    def component_command(self, name):
+    def component_command(self, name: str) -> str:
         return " ".join(
             filter(
                 None,

src/build_workflow/build_artifact_check.py

@@ -7,16 +7,18 @@
 import os
 from abc import ABC, abstractmethod
 
+from build_workflow.build_target import BuildTarget
+
 
 class BuildArtifactCheck(ABC):
     class BuildArtifactInvalidError(Exception):
-        def __init__(self, path, message):
+        def __init__(self, path: str, message: str) -> None:
             self.path = path
             super().__init__(f"Artifact {os.path.basename(path)} is invalid. {message}")
 
-    def __init__(self, target):
+    def __init__(self, target: BuildTarget) -> None:
         self.target = target
 
     @abstractmethod
-    def check(self, path):
+    def check(self, path: str) -> None:
         pass

src/build_workflow/build_artifact_checks.py

@@ -4,13 +4,16 @@
 # this file be licensed under the Apache-2.0 license or a
 # compatible open source license.
 
+from typing import Any, Dict
+
+from build_workflow.build_target import BuildTarget
 from build_workflow.opensearch.build_artifact_check_maven import BuildArtifactOpenSearchCheckMaven
 from build_workflow.opensearch.build_artifact_check_plugin import BuildArtifactOpenSearchCheckPlugin
 from build_workflow.opensearch_dashboards.build_artifact_check_plugin import BuildArtifactOpenSearchDashboardsCheckPlugin
 
 
 class BuildArtifactChecks:
-    TYPES = {
+    TYPES: Dict[str, Dict[str, Any]] = {
         "OpenSearch": {
             "plugins": BuildArtifactOpenSearchCheckPlugin,
             "maven": BuildArtifactOpenSearchCheckMaven,
@@ -19,21 +22,21 @@ class BuildArtifactChecks:
     }
 
     @classmethod
-    def from_name_and_type(cls, name, type):
+    def from_name_and_type(cls, name: str, type: str) -> Any:
         checks = cls.TYPES.get(name, None)
         if not checks:
             raise ValueError(f"Unsupported bundle: {name}")
         return checks.get(type, None)
 
     @classmethod
-    def create(cls, target, artifact_type):
+    def create(cls, target: BuildTarget, artifact_type: str) -> Any:
         klass = cls.from_name_and_type(target.name, artifact_type)
         if not klass:
             return None
         return klass(target)
 
     @classmethod
-    def check(cls, target, artifact_type, path):
+    def check(cls, target: BuildTarget, artifact_type: str, path: str) -> None:
         instance = cls.create(target, artifact_type)
         if instance:
             instance.check(path)

src/build_workflow/build_recorder.py

@@ -7,18 +7,21 @@
 import logging
 import os
 import shutil
+from typing import Any, Dict
 
 from build_workflow.build_artifact_checks import BuildArtifactChecks
+from build_workflow.build_target import BuildTarget
+from git.git_repository import GitRepository
 from manifests.build_manifest import BuildManifest
 
 
 class BuildRecorder:
-    def __init__(self, target):
+    def __init__(self, target: BuildTarget):
         self.build_manifest = self.BuildManifestBuilder(target)
         self.target = target
         self.name = target.name
 
-    def record_component(self, component_name, git_repo):
+    def record_component(self, component_name: str, git_repo: GitRepository) -> None:
         self.build_manifest.append_component(
             component_name,
             self.target.component_version,
@@ -27,7 +30,7 @@ def record_component(self, component_name, git_repo):
             git_repo.sha,
         )
 
-    def record_artifact(self, component_name, artifact_type, artifact_path, artifact_file):
+    def record_artifact(self, component_name: str, artifact_type: str, artifact_path: str, artifact_file: str) -> None:
         logging.info(f"Recording {artifact_type} artifact for {component_name}: {artifact_path} (from {artifact_file})")
         # Ensure the target directory exists
         dest_file = os.path.join(self.target.output_dir, artifact_path)
@@ -40,27 +43,27 @@ def record_artifact(self, component_name, artifact_type, artifact_path, artifact
         # Notify the recorder
         self.build_manifest.append_artifact(component_name, artifact_type, artifact_path)
 
-    def get_manifest(self):
+    def get_manifest(self) -> BuildManifest:
         return self.build_manifest.to_manifest()
 
-    def write_manifest(self):
+    def write_manifest(self) -> None:
         manifest_path = os.path.join(self.target.output_dir, "manifest.yml")
         self.get_manifest().to_file(manifest_path)
         logging.info(f"Created build manifest {manifest_path}")
 
     class BuildManifestBuilder:
-        def __init__(self, target):
-            self.data = {}
+        def __init__(self, target: BuildTarget):
+            self.data: Dict[str, Any] = {}
             self.data["build"] = {}
             self.data["build"]["id"] = target.build_id
             self.data["build"]["name"] = target.name
             self.data["build"]["version"] = target.opensearch_version
             self.data["build"]["platform"] = target.platform
             self.data["build"]["architecture"] = target.architecture
             self.data["schema-version"] = "1.2"
-            self.components_hash = {}
+            self.components_hash: Dict[str, Dict[str, Any]] = {}
 
-        def append_component(self, name, version, repository_url, ref, commit_id):
+        def append_component(self, name: str, version: str, repository_url: str, ref: str, commit_id: str) -> None:
             component = {
                 "name": name,
                 "repository": repository_url,
@@ -71,14 +74,14 @@ def append_component(self, name, version, repository_url, ref, commit_id):
             }
             self.components_hash[name] = component
 
-        def append_artifact(self, component, type, path):
+        def append_artifact(self, component: str, type: str, path: str) -> None:
             artifacts = self.components_hash[component]["artifacts"]
             list = artifacts.get(type, [])
             if len(list) == 0:
                 artifacts[type] = list
             list.append(path)
 
-        def to_manifest(self):
+        def to_manifest(self) -> 'BuildManifest':
             # The build manifest expects `components` to be a list, not a hash, so we need to munge things a bit
             components = self.components_hash.values()
             if len(components):

src/build_workflow/build_target.py

@@ -6,6 +6,7 @@
 
 import os
 import uuid
+from typing import List
 
 from system.os import current_architecture, current_platform
 
@@ -21,14 +22,14 @@ class BuildTarget:
 
     def __init__(
         self,
-        version,
-        patches=[],
-        platform=None,
-        architecture=None,
-        name=None,
-        snapshot=True,
-        build_id=None,
-        output_dir="artifacts"
+        version: str,
+        patches: List[str] = [],
+        platform: str = None,
+        architecture: str = None,
+        name: str = None,
+        snapshot: bool = True,
+        build_id: str = None,
+        output_dir: str = "artifacts",
     ):
         self.build_id = os.getenv("BUILD_NUMBER") or build_id or uuid.uuid4().hex
         self.name = name
@@ -40,32 +41,32 @@ def __init__(
         self.output_dir = output_dir
 
     @property
-    def opensearch_version(self):
+    def opensearch_version(self) -> str:
         return self.version + "-SNAPSHOT" if self.snapshot else self.version
 
     @property
-    def compatible_opensearch_versions(self):
+    def compatible_opensearch_versions(self) -> List[str]:
         return (
             [self.version + "-SNAPSHOT" if self.snapshot else self.version]
             + self.patches
             + list(map(lambda version: version + "-SNAPSHOT", self.patches))
         )
 
     @property
-    def component_version(self):
+    def component_version(self) -> str:
         # BUG: the 4th digit is dictated by the component, it's not .0, this will break for 1.1.0.1
         return self.version + ".0-SNAPSHOT" if self.snapshot else f"{self.version}.0"
 
     @property
-    def compatible_component_versions(self):
+    def compatible_component_versions(self) -> List[str]:
         return (
             [self.version + ".0-SNAPSHOT" if self.snapshot else f"{self.version}.0"]
             + list(map(lambda version: version + ".0", self.patches))
             + list(map(lambda version: version + ".0-SNAPSHOT", self.patches))
         )
 
     @property
-    def compatible_versions(self):
+    def compatible_versions(self) -> List[str]:
         versions = [self.version]
         versions.extend(self.patches)
         return versions

src/build_workflow/builder.py

@@ -5,22 +5,26 @@
 # compatible open source license.
 
 from abc import ABC, abstractmethod
+from typing import Any
+
+from build_workflow.build_recorder import BuildRecorder
+from build_workflow.build_target import BuildTarget
 
 
 class Builder(ABC):
-    def __init__(self, component, target):
+    def __init__(self, component: Any, target: BuildTarget) -> None:
         self.output_path = "builds"
         self.component = component
         self.target = target
 
     @abstractmethod
-    def checkout(self):
+    def checkout(self, work_dir: str) -> None:
         pass
 
     @abstractmethod
-    def build(self, build_recorder):
+    def build(self, build_recorder: BuildRecorder) -> None:
         pass
 
     @abstractmethod
-    def export_artifacts(self, build_recorder):
+    def export_artifacts(self, build_recorder: BuildRecorder) -> None:
         pass

src/build_workflow/builder_from_dist.py

@@ -7,30 +7,33 @@
 import logging
 import os
 import urllib.request
+from typing import Any
 
 import manifests.distribution
+from build_workflow.build_recorder import BuildRecorder
 from build_workflow.builder import Builder
+from git.git_repository import GitRepository
 from manifests.build_manifest import BuildManifest
 
 
 class BuilderFromDist(Builder):
-    class ManifestGitRepository:
-        def __init__(self, manifest):
+    class ManifestGitRepository(GitRepository):
+        def __init__(self, manifest: Any) -> None:
             self.url = manifest.repository
             self.ref = manifest.ref
             self.sha = manifest.commit_id
 
-    def checkout(self, work_dir):
+    def checkout(self, work_dir: str) -> None:
         self.__download_build_manifest()
 
-    def build(self, build_recorder):
+    def build(self, build_recorder: 'BuildRecorder') -> None:
         pass
 
     @property
-    def target_name(self):
+    def target_name(self) -> str:
         return self.target.name.lower().replace(' ', '-')
 
-    def export_artifacts(self, build_recorder):
+    def export_artifacts(self, build_recorder: 'BuildRecorder') -> None:
         os.makedirs(self.output_path, exist_ok=True)
         component_manifest = self.build_manifest.components[self.component.name]
         logging.info(f"Downloading {component_manifest.name} {component_manifest.version} ({component_manifest.commit_id}) ...")
@@ -48,7 +51,7 @@ def export_artifacts(self, build_recorder):
                     urllib.request.urlretrieve(artifact_url, artifact_dest)
                     build_recorder.record_artifact(self.component.name, artifact_type, artifact, artifact_dest)
 
-    def __download_build_manifest(self):
+    def __download_build_manifest(self) -> None:
         self.distribution_url = manifests.distribution.find_build_root(self.component.dist, self.target.platform, self.target.architecture, self.target_name)
         manifest_url = f"{self.distribution_url}/manifest.yml"
         logging.info(f"Downloading {manifest_url} ...")

src/build_workflow/builder_from_source.py

@@ -6,6 +6,7 @@
 
 import os
 
+from build_workflow.build_recorder import BuildRecorder
 from build_workflow.builder import Builder
 from git.git_repository import GitRepository
 from paths.script_finder import ScriptFinder
@@ -18,15 +19,15 @@
 
 
 class BuilderFromSource(Builder):
-    def checkout(self, work_dir):
+    def checkout(self, work_dir: str) -> None:
         self.git_repo = GitRepository(
             self.component.repository,
             self.component.ref,
             os.path.join(work_dir, self.component.name),
             self.component.working_directory,
         )
 
-    def build(self, build_recorder):
+    def build(self, build_recorder: BuildRecorder) -> None:
         build_script = ScriptFinder.find_build_script(self.target.name, self.component.name, self.git_repo.working_directory)
 
         build_command = " ".join(
@@ -49,7 +50,7 @@ def build(self, build_recorder):
         self.git_repo.execute(build_command)
         build_recorder.record_component(self.component.name, self.git_repo)
 
-    def export_artifacts(self, build_recorder):
+    def export_artifacts(self, build_recorder: BuildRecorder) -> None:
         artifacts_path = os.path.join(self.git_repo.working_directory, self.output_path)
         for artifact_type in ["maven", "dist", "plugins", "libs", "core-plugins"]:
             for dir, _, files in os.walk(os.path.join(artifacts_path, artifact_type)):