-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#256: Fixed type hints #260
Changes from 11 commits
8b21c2b
24d58e3
0dadb19
24385b2
61e8d3b
45c4c0e
29659b2
aea7c29
15b5d74
5a4ef15
d66750d
bf7deed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,48 +1,65 @@ | ||
# pylint: disable=not-an-iterable | ||
from typing import Dict, Generator, Optional, Set, Tuple | ||
|
||
import luigi | ||
from exasol_integration_test_docker_environment.lib.base.abstract_task_future import ( | ||
AbstractTaskFuture, | ||
) | ||
from exasol_integration_test_docker_environment.lib.base.base_task import BaseTask | ||
from exasol_integration_test_docker_environment.lib.base.flavor_task import ( | ||
FlavorsBaseTask, | ||
) | ||
from exasol_integration_test_docker_environment.lib.base.json_pickle_target import ( | ||
JsonPickleTarget, | ||
) | ||
from exasol_integration_test_docker_environment.lib.base.pickle_target import ( | ||
PickleTarget, | ||
from exasol_integration_test_docker_environment.lib.docker.images.image_info import ( | ||
ImageInfo, | ||
) | ||
from luigi import Config | ||
from luigi.format import Nop | ||
|
||
from exasol.slc.internal.tasks.build.docker_flavor_build_base import ( | ||
DockerFlavorBuildBase, | ||
) | ||
|
||
|
||
class DockerBuildParameter(Config): | ||
goals = luigi.ListParameter() | ||
shortcut_build = luigi.BoolParameter(True) | ||
goals: Tuple[str, ...] = luigi.ListParameter() # type: ignore | ||
shortcut_build: bool = luigi.BoolParameter(True) # type: ignore | ||
|
||
|
||
class DockerBuild(FlavorsBaseTask, DockerBuildParameter): | ||
|
||
def __init__(self, *args, **kwargs): | ||
self._images_futures = None | ||
def __init__(self, *args, **kwargs) -> None: | ||
self._images_futures: Optional[AbstractTaskFuture] = None | ||
super().__init__(*args, **kwargs) | ||
|
||
def register_required(self): | ||
def register_required(self) -> None: | ||
tasks = self.create_tasks_for_flavors_with_common_params(DockerFlavorBuild) | ||
self._images_futures = self.register_dependencies(tasks) | ||
|
||
def run_task(self): | ||
image_info = self.get_values_from_futures(self._images_futures) | ||
self.return_object(image_info) | ||
def run_task(self) -> None: | ||
image_infos: Dict[str, Dict[str, ImageInfo]] = self.get_values_from_futures( | ||
self._images_futures | ||
) | ||
assert isinstance(image_infos, dict) | ||
assert all(isinstance(x, str) for x in image_infos.keys()) | ||
assert all(isinstance(x, dict) for x in image_infos.values()) | ||
assert all(isinstance(y, str) for x in image_infos.values() for y in x.keys()) | ||
assert all( | ||
isinstance(y, ImageInfo) for x in image_infos.values() for y in x.values() | ||
) | ||
Comment on lines
+41
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to be sure that the expected type hints for the deserialized objects are really correct. Haven't found a better way to check the type of a |
||
self.return_object(image_infos) | ||
|
||
|
||
class DockerFlavorBuild(DockerFlavorBuildBase, DockerBuildParameter): | ||
|
||
def get_goals(self): | ||
return self.goals | ||
def get_goals(self) -> Set[str]: | ||
return set(self.goals) | ||
|
||
def run_task(self): | ||
def run_task(self) -> Generator[BaseTask, None, None]: | ||
build_tasks = self.create_build_tasks(self.shortcut_build) | ||
image_info_futures = yield from self.run_dependencies(build_tasks) | ||
image_infos = self.get_values_from_futures(image_info_futures) | ||
image_infos: Dict[str, ImageInfo] = self.get_values_from_futures( | ||
image_info_futures | ||
) | ||
assert isinstance(image_infos, dict) | ||
assert all(isinstance(x, str) for x in image_infos.keys()) | ||
assert all(isinstance(x, ImageInfo) for x in image_infos.values()) | ||
self.return_object(image_infos) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,7 +26,7 @@ class DockerFlavorAnalyzeImageTask(DockerAnalyzeImageTask, FlavorBaseTask): | |
# In this case DockerPullOrBuildFlavorImageTask could create a DockerPullOrBuildImageTask | ||
# if this would have parameters instead of abstract methods | ||
|
||
def __init__(self, *args, **kwargs): | ||
def __init__(self, *args, **kwargs) -> None: | ||
self.build_step = ( # pylint: disable=assignment-from-no-return | ||
self.get_build_step() | ||
) | ||
|
@@ -42,7 +42,7 @@ def is_rebuild_requested(self) -> bool: | |
or len(config.force_rebuild_from) == 0 | ||
) | ||
|
||
def get_build_step(self) -> str: # type: ignore | ||
def get_build_step(self) -> Optional[str]: | ||
""" | ||
Called by the constructor to get the name of build step. | ||
Sub classes need to implement this method. | ||
|
@@ -83,15 +83,15 @@ def get_source_repository_name(self) -> str: | |
def get_target_repository_name(self) -> str: | ||
return target_docker_repository_config().repository_name # type: ignore | ||
|
||
def get_source_image_tag(self): | ||
def get_source_image_tag(self) -> str: | ||
if source_docker_repository_config().tag_prefix != "": | ||
return ( | ||
f"{source_docker_repository_config().tag_prefix}_{self.get_image_tag()}" | ||
) | ||
else: | ||
return f"{self.get_image_tag()}" | ||
|
||
def get_target_image_tag(self): | ||
def get_target_image_tag(self) -> str: | ||
if target_docker_repository_config().tag_prefix != "": | ||
return ( | ||
f"{target_docker_repository_config().tag_prefix}_{self.get_image_tag()}" | ||
|
@@ -105,7 +105,8 @@ def get_image_tag(self) -> str: | |
|
||
def get_mapping_of_build_files_and_directories(self) -> Dict[str, str]: | ||
build_step_path = self.get_build_step_path() | ||
result = {self.build_step: str(build_step_path)} | ||
assert self.build_step is not None | ||
result: Dict[str, str] = {self.build_step: str(build_step_path)} | ||
result.update(self.additional_build_directories_mapping) | ||
if language_definition := self.get_language_definition(): | ||
lang_def_path = self.get_path_in_flavor_path() / language_definition | ||
|
@@ -120,13 +121,17 @@ def get_mapping_of_build_files_and_directories(self) -> Dict[str, str]: | |
return result | ||
|
||
def get_build_step_path(self) -> Path: | ||
return self.get_path_in_flavor_path() / self.get_build_step() | ||
if build_step_path := self.get_build_step(): | ||
return self.get_path_in_flavor_path() / build_step_path | ||
else: | ||
return self.get_path_in_flavor_path() | ||
|
||
def get_path_in_flavor_path(self) -> Path: | ||
flavor_path: str = self.flavor_path # type: ignore | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't ITDE specify a type hint for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The type hint in ITDE needs to be fixed. |
||
if path_in_flavor := self.get_path_in_flavor(): | ||
return Path(self.flavor_path) / path_in_flavor # type: ignore | ||
return Path(flavor_path) / path_in_flavor | ||
else: | ||
return Path(self.flavor_path) # type: ignore | ||
return Path(flavor_path) | ||
|
||
def get_dockerfile(self) -> str: | ||
return str(self.get_build_step_path().joinpath("Dockerfile")) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,9 @@ | ||
from typing import Generator, List | ||
|
||
import docker.models.images | ||
import luigi | ||
from docker import DockerClient | ||
from exasol_integration_test_docker_environment.lib.base.base_task import BaseTask | ||
from exasol_integration_test_docker_environment.lib.base.docker_base_task import ( | ||
DockerBaseTask, | ||
) | ||
|
@@ -14,12 +19,9 @@ | |
|
||
|
||
class CleanImageTask(DockerBaseTask): | ||
image_id = luigi.Parameter() | ||
|
||
def __init__(self, *args, **kwargs): | ||
super().__init__(*args, **kwargs) | ||
image_id: str = luigi.Parameter() # type: ignore | ||
|
||
def run_task(self): | ||
def run_task(self) -> Generator[BaseTask, None, None]: | ||
self.logger.info("Try to remove dependent images of %s" % self.image_id) | ||
yield from self.run_dependencies( | ||
self.get_clean_image_tasks_for_dependent_images() | ||
|
@@ -38,7 +40,7 @@ def run_task(self): | |
) | ||
) | ||
|
||
def get_clean_image_tasks_for_dependent_images(self): | ||
def get_clean_image_tasks_for_dependent_images(self) -> List["CleanImageTask"]: | ||
with self._get_docker_client() as docker_client: | ||
image_ids = [ | ||
str(possible_child).replace("sha256:", "") | ||
|
@@ -50,7 +52,7 @@ def get_clean_image_tasks_for_dependent_images(self): | |
for image_id in image_ids | ||
] | ||
|
||
def is_child_image(self, possible_child, docker_client): | ||
def is_child_image(self, possible_child, docker_client) -> bool: | ||
try: | ||
inspect = docker_client.api.inspect_image( | ||
image=str(possible_child).replace("sha256:", "") | ||
|
@@ -61,9 +63,9 @@ def is_child_image(self, possible_child, docker_client): | |
|
||
|
||
class CleanImagesStartingWith(DockerBaseTask): | ||
starts_with_pattern = luigi.Parameter() | ||
starts_with_pattern: str = luigi.Parameter() # type: ignore | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a utility function would be useful, e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I think this is not possible, because the object needs to be luigi.Parameter() here; only after the deserialization in the constructor it "becomes" a str |
||
|
||
def register_required(self): | ||
def register_required(self) -> None: | ||
with self._get_docker_client() as docker_client: | ||
image_ids = [ | ||
str(image.id).replace("sha256:", "") | ||
|
@@ -76,7 +78,9 @@ def register_required(self): | |
] | ||
) | ||
|
||
def find_images_to_clean(self, docker_client): | ||
def find_images_to_clean( | ||
self, docker_client: DockerClient | ||
) -> List[docker.models.images.Image]: | ||
self.logger.info( | ||
"Going to remove all images starting with %s" % self.starts_with_pattern | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not nice from luigi. Even though the class is called
ListParameter
, the serialized object is a tuple. Needed to change this in several places...