-
Notifications
You must be signed in to change notification settings - Fork 636
chg: [importer] reuse existing sketch if name is given #3626
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
base: master
Are you sure you want to change the base?
Conversation
|
/gemini review |
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.
Code Review
This pull request introduces a useful feature to reuse existing sketches by name. The implementation is straightforward. My review includes a couple of suggestions to align the new code with the project's style guide, specifically regarding type hints, docstring format, and the preferred formatting for log messages.
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.
Code Review
This pull request modifies the Timesketch importer to reuse an existing sketch if a name is provided, instead of always creating a new one. The implementation introduces a new helper function get_sketch_by_name and updates the main logic to use it. The changes are logical and address the issue described. I've identified a critical bug in the new helper function that could lead to a runtime error due to a type mismatch, and a minor style issue regarding docstring formatting. My review includes suggestions to fix these issues.
jkppr
left a comment
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.
Thanks for adding this feature. Please take a look at the review comments below. Let's make sure this is added in the right place as a robust feature.
| handler.setFormatter(logger_formatter) | ||
|
|
||
|
|
||
| def get_sketch_by_name( |
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.
Architectural comment
So looking up a sketch by name is a generic operation that would be useful for other integrations as well, not just the importer. (like the existing get_sketch)
Instead of keeping this logic isolated in timesketch_importer.py, could you move this method into the core TimesketchApi class in api_client/python/timesketch_api_client/client.py here?
This would ensure the method can be used in other tools as well and keeps the "business logic" of how we handle sketches in one place. It also let's the importer focus on what it can do best: Import Data.
You can add it right next to the existing get_sketch(self, sketch_id) method. It should handle all the duplicate name issues etc. that we discussed in this PR there as well.
Then, in timesketch_importer.py, you can simply call my_sketch = ts_client.get_sketch_by_name(sketch_name).
|
Thank you very much for the thinking and lots of provided input. |
…s/timesketch into feature_sketch_by_name
Added one function to get the
get_sketch_by_name()plus a minor change try/except to try getting the existing sketch.Checks
Closing issues
closes #3625