-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add run_isolated
and pass_docker_extra_args
to docker hook
#3952
Changes from 66 commits
865c83a
636c67c
12c7b3a
b6c0074
52cbc16
811d7d8
df905e0
bdb623a
a001cd0
48ba8fb
1fec4d7
145a4d4
df7265d
03c1a9a
51c3fc2
8e662f9
8fadcdd
8b0cfee
8e5c718
4c3ac03
4c071bc
07768c9
aa0774e
dc84978
e4393b5
ac72ac2
bb343af
dfb4bfd
66696b7
0ab2404
5271826
1fdcdab
d75a7bf
a86f967
c5f1c95
dce24a8
912263a
99680ac
01cea5f
b69788a
c94d1f3
ce31d04
60fbc5c
0a374c9
fa1f2a6
5b09049
0e2e4ae
9b883dc
cd54a5b
b845feb
0bb77df
8c40fe8
5e3d3e1
7848edf
fd54b1b
bbed5c8
2ab5ab6
f07d146
5b4b71a
972af9e
d1dec05
8874f77
04269f2
b4a5fbd
fa6181c
411b849
16d52bd
e43fd9d
5012410
53d24e2
7d40490
922f9f1
5a3c098
142ba15
548cfe9
31b6082
43c383c
dc3c3ee
a05d9c1
cc9edf5
a8c43cd
5617450
b103059
599cda9
0943a45
9713aac
6370e15
2694167
d4d298b
7a24e3d
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 |
---|---|---|
|
@@ -4,16 +4,23 @@ | |
import subprocess | ||
import time | ||
from collections import defaultdict | ||
from concurrent.futures import ThreadPoolExecutor | ||
from copy import deepcopy | ||
from functools import lru_cache | ||
from pathlib import Path | ||
from typing import Dict, Iterable, List, Optional, Set, Tuple | ||
|
||
from docker.errors import DockerException | ||
from packaging.version import Version | ||
|
||
from demisto_sdk.commands.common.constants import TYPE_PWSH, TYPE_PYTHON | ||
from demisto_sdk.commands.common.constants import ( | ||
TYPE_PWSH, | ||
TYPE_PYTHON, | ||
) | ||
from demisto_sdk.commands.common.content_constant_paths import CONTENT_PATH, PYTHONPATH | ||
from demisto_sdk.commands.common.cpu_count import cpu_count | ||
from demisto_sdk.commands.common.docker_helper import ( | ||
DockerBase, | ||
docker_login, | ||
get_docker, | ||
init_global_docker_client, | ||
|
@@ -153,15 +160,15 @@ def get_environment_flag(env: dict) -> str: | |
def _split_by_objects( | ||
files_with_objects: List[Tuple[Path, IntegrationScript]], | ||
config_arg: Optional[Tuple], | ||
split_by_obj: bool = False, | ||
run_isolated: bool = False, | ||
) -> Dict[Optional[IntegrationScript], Set[Tuple[Path, IntegrationScript]]]: | ||
""" | ||
Will group files into groups that share the same configuration file. | ||
If there is no config file, they get set to the NO_CONFIG_VALUE group | ||
Args: | ||
files: the files to split | ||
config_arg: a tuple, argument_name, file_name | ||
split_by_obj: a boolean. If true it will split all the objects into separate hooks. | ||
run_isolated: a boolean. If true it will split all the objects into separate hooks. | ||
|
||
Returns: | ||
a dict where the keys are the names of the folder of the config and the value is a set of files for that config | ||
|
@@ -171,8 +178,9 @@ def _split_by_objects( | |
] = defaultdict(set) | ||
|
||
for file, obj in files_with_objects: | ||
if split_by_obj or (config_arg and (obj.path.parent / config_arg[1]).exists()): | ||
if run_isolated or (config_arg and (obj.path.parent / config_arg[1]).exists()): | ||
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. extract |
||
object_to_files[obj].add((file, obj)) | ||
|
||
else: | ||
object_to_files[NO_SPLIT].add((file, obj)) | ||
|
||
|
@@ -184,6 +192,38 @@ class DockerHook(Hook): | |
This class will make common manipulations on commands that need to run in docker | ||
""" | ||
|
||
def clean_args_from_hook(self, hooks: List[Dict]): | ||
"""This clean unsupported args from the generated hooks | ||
|
||
Args: | ||
hooks (List[Dict]): The hooks generated | ||
""" | ||
for hook in hooks: | ||
hook.pop("docker_image", None) | ||
hook.pop("config_file_arg", None) | ||
hook.pop("copy_files", None) | ||
hook.pop("run_isolated", None) | ||
hook.pop("pass_docker_extra_args", None) | ||
|
||
def process_image( | ||
self, | ||
image: str, | ||
files_with_objects: List[Tuple[Path, IntegrationScript]], | ||
config_arg: Optional[Tuple], | ||
run_isolated: bool, | ||
) -> List[Dict]: | ||
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. add docstring |
||
object_to_files = _split_by_objects( | ||
files_with_objects, | ||
config_arg, | ||
run_isolated, | ||
) | ||
image_is_powershell = any(obj.is_powershell for _, obj in files_with_objects) | ||
ilaner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
dev_image = devtest_image(image, image_is_powershell, self.context.dry_run) | ||
hooks = self.generate_hooks(dev_image, image, object_to_files, config_arg) | ||
logger.debug(f"Generated {len(hooks)} hooks for image {image}") | ||
return hooks | ||
|
||
def prepare_hook( | ||
self, | ||
): | ||
|
@@ -222,39 +262,39 @@ def prepare_hook( | |
for file in copy_files: | ||
source: Path = CONTENT_PATH / file | ||
target = obj.path.parent / Path(file).name | ||
if source != target and source.exists(): | ||
if source != target and source.exists() and not target.exists(): | ||
shutil.copy( | ||
CONTENT_PATH / file, obj.path.parent / Path(file).name | ||
) | ||
split_by_obj = self._get_property("split_by_object", False) | ||
run_isolated = self._get_property("run_isolated", False) | ||
config_arg = self._get_config_file_arg() | ||
start_time = time.time() | ||
logger.debug(f"{len(tag_to_files_objs)} images were collected from files") | ||
logger.debug(f'collected images: {" ".join(tag_to_files_objs.keys())}') | ||
for image, files_with_objects in sorted( | ||
tag_to_files_objs.items(), key=lambda item: item[0] | ||
): | ||
object_to_files = _split_by_objects( | ||
files_with_objects, config_arg, split_by_obj | ||
) | ||
image_is_powershell = any( | ||
obj.is_powershell for _, obj in files_with_objects | ||
) | ||
|
||
dev_image = devtest_image(image, image_is_powershell, self.context.dry_run) | ||
hooks = self.get_new_hooks( | ||
dev_image, | ||
image, | ||
object_to_files, | ||
config_arg, | ||
) | ||
with ThreadPoolExecutor(max_workers=cpu_count()) as executor: | ||
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. arent we not pulling here? Whats the point of the threadpool? 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. we make requests to dockerhub api to check if the image is availible. |
||
results = [] | ||
for image, files_objs in sorted( | ||
tag_to_files_objs.items(), key=lambda item: item[0] | ||
): | ||
results.append( | ||
executor.submit( | ||
self.process_image, | ||
image, | ||
files_objs, | ||
config_arg, | ||
run_isolated, | ||
) | ||
) | ||
for result in results: | ||
hooks = result.result() | ||
self.hooks.extend(hooks) | ||
|
||
end_time = time.time() | ||
logger.debug( | ||
f"DockerHook - prepared images in {round(end_time - start_time, 2)} seconds" | ||
) | ||
|
||
def get_new_hooks( | ||
def generate_hooks( | ||
self, | ||
dev_image, | ||
image, | ||
|
@@ -270,6 +310,7 @@ def get_new_hooks( | |
image: name of the base image (for naming) | ||
object_to_files_with_objects: A dict where the key is the object (or None) and value is the set of files to run together. | ||
config_arg: The config arg to set where relevant. This will be appended to the end of "args" | ||
|
||
Returns: | ||
All the hooks to be appended for this image | ||
""" | ||
|
@@ -278,24 +319,35 @@ def get_new_hooks( | |
new_hook["name"] = f"{new_hook.get('name')}-{image}" | ||
new_hook["language"] = "docker_image" | ||
env = new_hook.pop("env", {}) | ||
docker_version = DockerBase.version() | ||
quiet = True | ||
# quiet mode is silently pull the image, and it is supported only above 19.03 | ||
ilaner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if docker_version < Version("19.03"): | ||
quiet = False | ||
docker_extra_args = self._get_property("pass_docker_extra_args", "") | ||
new_hook[ | ||
"entry" | ||
] = f'--entrypoint {new_hook.get("entry")} {get_environment_flag(env)} --quiet {dev_image}' | ||
] = f'--entrypoint {new_hook.get("entry")} {docker_extra_args} {get_environment_flag(env)} {"--quiet" if quiet else ""} -u 4000:4000 {dev_image}' | ||
ret_hooks = [] | ||
for ( | ||
integration_script, | ||
files_with_objects, | ||
) in object_to_files_with_objects.items(): | ||
change_working_directory = False | ||
files = {file for file, _ in files_with_objects} | ||
hook = deepcopy(new_hook) | ||
if integration_script is not None: | ||
change_working_directory = ( | ||
True # isolate container, so run in the same directory | ||
) | ||
if config_arg: | ||
args = deepcopy(self._get_property("args", [])) | ||
args.extend( | ||
[ | ||
config_arg[0], | ||
str( | ||
( | ||
Path("/src") | ||
/ ( | ||
integration_script.path.parent / config_arg[1] | ||
).relative_to(CONTENT_PATH) | ||
), | ||
|
@@ -308,17 +360,22 @@ def get_new_hooks( | |
hook[ | ||
"name" | ||
] = f"{hook['name']}-{integration_script.object_id}" # for uniqueness | ||
# change the working directory to the integration script, as it runs in an isolated container | ||
hook[ | ||
"entry" | ||
] = f"-w {Path('/src') / integration_script.path.parent.relative_to(CONTENT_PATH)} {hook['entry']}" | ||
|
||
if self._set_files_on_hook( | ||
hook, files, should_filter=False | ||
hook, | ||
files, | ||
should_filter=False, | ||
use_args=change_working_directory, | ||
base_path=Path("/src"), | ||
): # no need to filter again, we have only filtered files | ||
# disable multiprocessing on hook | ||
hook["require_serial"] = True | ||
ret_hooks.append(hook) | ||
for hook in ret_hooks: | ||
hook.pop("docker_image", None) | ||
hook.pop("config_file_arg", None) | ||
hook.pop("copy_files", None) | ||
hook.pop("split_by_object", None) | ||
self.clean_args_from_hook(ret_hooks) | ||
return ret_hooks | ||
|
||
def _get_config_file_arg(self) -> Optional[Tuple]: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
|
||
from packaging.version import Version | ||
|
||
from demisto_sdk.commands.common.content_constant_paths import CONTENT_PATH | ||
from demisto_sdk.commands.common.logger import logger | ||
from demisto_sdk.commands.pre_commit.hooks.utils import get_property | ||
from demisto_sdk.commands.pre_commit.pre_commit_context import PreCommitContext | ||
|
@@ -44,21 +45,33 @@ def exclude_irrelevant_files(self): | |
self._exclude_hooks_by_support_level() | ||
|
||
def _set_files_on_hook( | ||
self, hook: dict, files: Iterable[Path], should_filter: bool = True | ||
self, | ||
hook: dict, | ||
files: Iterable[Path], | ||
should_filter: bool = True, | ||
use_args: bool = False, | ||
base_path: Path = CONTENT_PATH, | ||
ilaner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) -> int: | ||
""" | ||
|
||
Args: | ||
hook: mutates the hook with files returned from filter_files_matching_hook_config | ||
files: the list of files to set on the hook | ||
|
||
use_args: if True, the files will be added to the args of the hook, and pass_filenames will be set to False | ||
Returns: the number of files that ultimately are set on the hook. Use this to decide if to run the hook at all | ||
|
||
""" | ||
files_to_run_on_hook = set(files) | ||
if should_filter: | ||
files_to_run_on_hook = self.filter_files_matching_hook_config(files) | ||
hook["files"] = join_files(files_to_run_on_hook) | ||
if use_args: | ||
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. whats this do? 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. because we run the script not in content working directory we will have to provide the filepaths in the |
||
if "args" not in hook: | ||
hook["args"] = [] | ||
hook["args"].extend( | ||
(str(base_path / file) for file in files_to_run_on_hook) | ||
) | ||
hook["pass_filenames"] = False | ||
return len(files_to_run_on_hook) | ||
|
||
def filter_files_matching_hook_config( | ||
|
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.
why did you remove it completely?