Skip to content

Conversation

@jaegeral
Copy link
Collaborator

@jaegeral jaegeral commented Nov 12, 2025

This pull request enhances the system's deletion workflows by adding new end-to-end tests and refining existing logic. It specifically addresses the proper handling of soft-deleted sketches during hard deletion and ensures that search indexes for failed timelines are correctly archived upon deletion.

@jaegeral
Copy link
Collaborator Author

jaegeral commented Dec 2, 2025

/gemini summary

@gemini-code-assist
Copy link
Contributor

Summary of Changes

This pull request enhances the system's deletion workflows by adding new end-to-end tests and refining existing logic. It specifically addresses the proper handling of soft-deleted sketches during hard deletion and ensures that search indexes for failed timelines are correctly archived upon deletion. These changes aim to improve the robustness and reliability of data cleanup operations.

Highlights

  • New Test for Hard Deleting Soft-Deleted Sketches: A new end-to-end test has been added to verify the functionality of hard-deleting a sketch that was previously soft-deleted. This ensures proper cleanup and state transitions for sketches.
  • New Test for Deleting Failed Timelines: An end-to-end test was introduced to confirm that when a timeline with a 'fail' status is deleted, its associated search index is correctly archived.
  • Improved Timeline Deletion Logic: The timeline deletion logic has been updated to correctly handle timelines that are in a 'fail' status, ensuring their search indexes are considered for closure alongside 'archived' timelines.
Changelog
  • end_to_end_tests/client_test.py
    • Added test_hard_delete_soft_deleted_sketch to validate the hard deletion of sketches that have been soft-deleted.
  • end_to_end_tests/timeline_deletion_test.py
    • New file created to house end-to-end tests for timeline deletion.
    • Implemented test_delete_failed_timeline to verify that search indexes for failed timelines are archived after deletion.
  • timesketch/api/v1/resources/timeline.py
    • Modified the delete method to include 'fail' status in the condition for closing a search index, alongside 'archived' status, during timeline deletion.
Activity
  • jaegeral requested a Gemini summary.

@jaegeral
Copy link
Collaborator Author

jaegeral commented Dec 2, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 important fixes for timeline deletion workflows, particularly for timelines in a 'fail' state. The core logic change in timesketch/api/v1/resources/timeline.py correctly allows an index to be archived even if sibling timelines have a 'fail' or 'deleted' status. The addition of the end_to_end_tests/timeline_deletion_test.py file with comprehensive tests for these scenarios is a great enhancement and significantly improves confidence in the fix. My review includes suggestions to improve the new test code for better maintainability and adherence to the project's style guide, such as removing redundant code, organizing imports, and eliminating hardcoded values.

jaegeral and others added 4 commits December 2, 2025 18:51
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@jaegeral jaegeral requested a review from jkppr December 2, 2025 17:56
@jaegeral jaegeral marked this pull request as ready for review December 2, 2025 17:56
@jaegeral jaegeral self-assigned this Dec 5, 2025
Copy link
Collaborator

@jkppr jkppr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, small comment below


if timeline_.id != timeline_id:
# There are more than a single timeline using this index_name,
# we can't close it (unless this timeline is archived).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update the comment accordingly to reflect the tates that will prevent index closure?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants