-
Notifications
You must be signed in to change notification settings - Fork 32
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 Workflow module to Wazuh-qa repository #4990
Add Workflow module to Wazuh-qa repository #4990
Conversation
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.
LGTM
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.
Review Notes
LGTM
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.
GJ, some changes
deployability/modules/workflow_engine/examples/dtt1-agents-poc.yaml
Outdated
Show resolved
Hide resolved
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.
LGTM
deployability/modules/setup.py
Outdated
script_path = os.path.dirname(__file__) | ||
rel_path = "../version.json" | ||
abs_file_path = os.path.join(script_path, rel_path) | ||
f = open(abs_file_path) |
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.
Always use a with
statement to open files, ensuring an appropriate context handling
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.
Done in c9e7911
from setuptools import setup, find_packages | ||
import os | ||
|
||
def get_files_from_directory(directory): |
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 may lead to including unwanted files (like temporary or binary files) in the distribution package. It's generally better to specify only the types of files we need explicitly.
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.
Done in c9e7911
deployability/modules/setup.py
Outdated
paths = [] | ||
for (path, directories, filenames) in os.walk(directory): | ||
for filename in filenames: | ||
paths.append(os.path.join('..', path, filename)) |
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.
We are assuming the package's structure relative to where the setup script is run, which can vary and not work as expected while packaging. We need to consider using a more predictable method to specify package data instead of ../
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.
Done in c9e7911
deployability/modules/setup.py
Outdated
|
||
def get_version(): | ||
script_path = os.path.dirname(__file__) | ||
rel_path = "../version.json" |
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.
We always need to add appropriate exception handling, if the version.json
file doesn't exist or contains invalid JSON it will lead to a runtime error without proper handling.
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.
Done in c9e7911
|
||
class ThreadIDFilter(logging.Filter): | ||
""" | ||
A filter that uppercases the name of the log record. |
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 seems wrong based on the function below
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.
Done in c9e7911
""" | ||
A filter that uppercases the name of the log record. | ||
""" | ||
def filter(self, record: str) -> bool: |
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.
The str
type hint is incorrect, it should be logging.LogRecord
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.
Done in c9e7911
bool: True if the record should be logged, False otherwise. | ||
""" | ||
record.thread_id = threading.get_native_id() | ||
return record |
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 returning a bool but the record itself
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.
Done in c9e7911
|
||
def _load_config() -> None: | ||
""" | ||
Loads the logging configuration from 'config.yaml' file. |
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.
Add error handling around the file reading and YAML parsing process. If the config.yaml file is missing, corrupt, or contains invalid YAML, the application should handle this gracefully, possibly falling back to a default logging configuration.
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.
Done in c9e7911
@@ -0,0 +1,3 @@ | |||
# Copyright (C) 2015, Wazuh Inc. |
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.
What is this empty file?
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 file is not used for the Workflow, it is dragged in case it is required at some point, but at the moment it is not necessary and was eliminated in 39e5f11.
Description
The objective of the PR is to incorporate the DTT Workflow module into the QA repository.
Related to: #4905
Testing performed
All test in: #4910