lordgamez commented on code in PR #2133: URL: https://github.com/apache/nifi-minifi-cpp/pull/2133#discussion_r3099560857
########## behave_framework/src/minifi_test_framework/containers/container_windows.py: ########## @@ -0,0 +1,392 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +from __future__ import annotations +import logging +import os +import tempfile +import base64 +import tarfile +import io +from typing import Union, Optional, Tuple, List, Dict, TYPE_CHECKING + +import docker +from docker.models.networks import Network +from docker.models.containers import Container + +from minifi_test_framework.containers.container_protocol import ContainerProtocol +from minifi_test_framework.containers.directory import Directory +from minifi_test_framework.containers.file import File +from minifi_test_framework.containers.host_file import HostFile + +if TYPE_CHECKING: + from minifi_test_framework.core.minifi_test_context import MinifiTestContext + + +class WindowsContainer(ContainerProtocol): + def __init__(self, image_name: str, container_name: str, network: Network, command: str | None = None, entrypoint: str | None = None): + super().__init__() + self.image_name: str = image_name + self.container_name: str = container_name + self.network: Network = network + + self.client: docker.DockerClient = docker.from_env() + self.container: Optional[Container] = None + self.files: List[File] = [] + self.dirs: List[Directory] = [] + self.host_files: List[HostFile] = [] + self.volumes: Dict = {} + self.command: str | None = command + self.entrypoint: str | None = entrypoint + self._temp_dir: Optional[tempfile.TemporaryDirectory] = None + self.ports: Optional[Dict[str, int]] = None + self.environment: List[str] = [] + + def _normalize_path(self, path: str) -> str: + clean_path = path.strip().replace("/", "\\") + if clean_path.startswith("\\"): + clean_path = clean_path[1:] + + # If it doesn't already have a drive letter, assume C: + if ":" not in clean_path: + return f"C:\\{clean_path}" + return clean_path + + def deploy(self, context: MinifiTestContext | None) -> bool: + if self._temp_dir: + self._temp_dir.cleanup() + self._temp_dir = tempfile.TemporaryDirectory() + + for directory in self.dirs: + rel_path = directory.path.strip("/\\") + temp_subdir = os.path.join(self._temp_dir.name, rel_path) + os.makedirs(temp_subdir, exist_ok=True) + + for file_name, content in directory.files.items(): + file_path = os.path.join(temp_subdir, file_name) + with open(file_path, "w", encoding="utf-8") as temp_file: + logging.info(f"writing content into {temp_file.name}") + temp_file.write(content) + + container_bind_path = self._normalize_path(directory.path) + self.volumes[temp_subdir] = { + "bind": container_bind_path, + "mode": directory.mode + } + + for host_file in self.host_files: + container_bind_path = self._normalize_path(host_file.container_path) + self.volumes[host_file.host_path] = {"bind": container_bind_path, "mode": host_file.mode} + + try: + existing_container = self.client.containers.get(self.container_name) + logging.warning(f"Found existing container '{self.container_name}'. Removing it first.") + existing_container.remove(force=True) + except docker.errors.NotFound: + pass + + try: + logging.info(f"Creating and starting container '{self.container_name}'...") + self.container = self.client.containers.create( + image=self.image_name, + name=self.container_name, + ports=self.ports, + environment=self.environment, + volumes=self.volumes, + network=self.network.name, + command=self.command, + entrypoint=self.entrypoint, + detach=True, + tty=False + ) + + self.container.start() Review Comment: Could we replace container.create + container.start with container.run here? ########## behave_framework/src/minifi_test_framework/containers/container_windows.py: ########## @@ -0,0 +1,392 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +from __future__ import annotations +import logging +import os +import tempfile +import base64 +import tarfile +import io +from typing import Union, Optional, Tuple, List, Dict, TYPE_CHECKING + +import docker +from docker.models.networks import Network +from docker.models.containers import Container + +from minifi_test_framework.containers.container_protocol import ContainerProtocol +from minifi_test_framework.containers.directory import Directory +from minifi_test_framework.containers.file import File +from minifi_test_framework.containers.host_file import HostFile + +if TYPE_CHECKING: + from minifi_test_framework.core.minifi_test_context import MinifiTestContext + + +class WindowsContainer(ContainerProtocol): + def __init__(self, image_name: str, container_name: str, network: Network, command: str | None = None, entrypoint: str | None = None): + super().__init__() + self.image_name: str = image_name + self.container_name: str = container_name + self.network: Network = network + + self.client: docker.DockerClient = docker.from_env() + self.container: Optional[Container] = None + self.files: List[File] = [] + self.dirs: List[Directory] = [] + self.host_files: List[HostFile] = [] + self.volumes: Dict = {} + self.command: str | None = command + self.entrypoint: str | None = entrypoint + self._temp_dir: Optional[tempfile.TemporaryDirectory] = None + self.ports: Optional[Dict[str, int]] = None + self.environment: List[str] = [] + + def _normalize_path(self, path: str) -> str: + clean_path = path.strip().replace("/", "\\") + if clean_path.startswith("\\"): + clean_path = clean_path[1:] + + # If it doesn't already have a drive letter, assume C: + if ":" not in clean_path: + return f"C:\\{clean_path}" + return clean_path + + def deploy(self, context: MinifiTestContext | None) -> bool: + if self._temp_dir: + self._temp_dir.cleanup() + self._temp_dir = tempfile.TemporaryDirectory() + + for directory in self.dirs: + rel_path = directory.path.strip("/\\") + temp_subdir = os.path.join(self._temp_dir.name, rel_path) + os.makedirs(temp_subdir, exist_ok=True) + + for file_name, content in directory.files.items(): + file_path = os.path.join(temp_subdir, file_name) + with open(file_path, "w", encoding="utf-8") as temp_file: + logging.info(f"writing content into {temp_file.name}") + temp_file.write(content) + + container_bind_path = self._normalize_path(directory.path) + self.volumes[temp_subdir] = { + "bind": container_bind_path, + "mode": directory.mode + } + + for host_file in self.host_files: + container_bind_path = self._normalize_path(host_file.container_path) + self.volumes[host_file.host_path] = {"bind": container_bind_path, "mode": host_file.mode} + + try: + existing_container = self.client.containers.get(self.container_name) + logging.warning(f"Found existing container '{self.container_name}'. Removing it first.") + existing_container.remove(force=True) + except docker.errors.NotFound: + pass + + try: + logging.info(f"Creating and starting container '{self.container_name}'...") + self.container = self.client.containers.create( + image=self.image_name, + name=self.container_name, + ports=self.ports, + environment=self.environment, + volumes=self.volumes, + network=self.network.name, + command=self.command, + entrypoint=self.entrypoint, + detach=True, + tty=False + ) + + self.container.start() + + for file in self.files: + self._copy_content_to_container(file.content, file.path) + + except Exception as e: + logging.error(f"Error starting container: {e}") + self.clean_up() + raise + return True + + def _copy_content_to_container(self, content: str | bytes, target_path: str): + if not self.container: + return + + win_path = self._normalize_path(target_path) + dir_name = os.path.dirname(win_path) + file_name = os.path.basename(win_path) + + self._run_powershell(f"New-Item -ItemType Directory -Force -Path '{dir_name}'") + + tar_stream = io.BytesIO() + with tarfile.open(fileobj=tar_stream, mode='w') as tar: + if isinstance(content, str): + encoded_data = content.encode('utf-8') + else: + encoded_data = content + tarinfo = tarfile.TarInfo(name=file_name) + tarinfo.size = len(encoded_data) + tar.addfile(tarinfo, io.BytesIO(encoded_data)) + + tar_stream.seek(0) + + self.container.put_archive(path=dir_name, data=tar_stream) + + def clean_up(self): + if self._temp_dir: + try: + self._temp_dir.cleanup() + self._temp_dir = None Review Comment: Shouldn't we put the `self._temp_dir = None` in the finally? Even if the cleanup fails, we should create a new directory for our next container after cleanup. ########## behave_framework/src/minifi_test_framework/containers/container_windows.py: ########## @@ -0,0 +1,392 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +from __future__ import annotations +import logging +import os +import tempfile +import base64 +import tarfile +import io +from typing import Union, Optional, Tuple, List, Dict, TYPE_CHECKING + +import docker +from docker.models.networks import Network +from docker.models.containers import Container + +from minifi_test_framework.containers.container_protocol import ContainerProtocol +from minifi_test_framework.containers.directory import Directory +from minifi_test_framework.containers.file import File +from minifi_test_framework.containers.host_file import HostFile + +if TYPE_CHECKING: + from minifi_test_framework.core.minifi_test_context import MinifiTestContext + + +class WindowsContainer(ContainerProtocol): + def __init__(self, image_name: str, container_name: str, network: Network, command: str | None = None, entrypoint: str | None = None): + super().__init__() + self.image_name: str = image_name + self.container_name: str = container_name + self.network: Network = network + + self.client: docker.DockerClient = docker.from_env() + self.container: Optional[Container] = None + self.files: List[File] = [] + self.dirs: List[Directory] = [] + self.host_files: List[HostFile] = [] + self.volumes: Dict = {} + self.command: str | None = command + self.entrypoint: str | None = entrypoint + self._temp_dir: Optional[tempfile.TemporaryDirectory] = None + self.ports: Optional[Dict[str, int]] = None Review Comment: We should be consistent, either use X | None everywhere or Optional, I would go with X | None as it is the newer syntax and needs no import. ########## docker/installed/win.Dockerfile: ########## Review Comment: The usual convention for naming Dockerfiles is `Dockerfile.<target>` so I would rename this to Dockerfile.win ########## behave_framework/src/minifi_test_framework/containers/minifi_protocol.py: ########## @@ -0,0 +1,58 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from typing import Protocol +from minifi_test_framework.minifi.flow_definition import FlowDefinition + + +class MinifiProtocol(Protocol): + flow_definition: FlowDefinition + + def set_property(self, key: str, value: str): + ... + + def set_log_property(self, key: str, value: str): + ... + + def set_deploy_timeout_seconds(self, timeout_seconds: int): + ... + + +def set_controller_socket_properties(minifi: MinifiProtocol): Review Comment: This is already part of MinifiController as a method, isn't that enough? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
