Skip to content
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

Introducing destroy_after config property and new model #69

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

ron-shinall
Copy link
Contributor

@ron-shinall ron-shinall commented Nov 4, 2024

This PR adds a new destroy_after property, and also introduces the SolidErrors::Cleaner model to handle the work.

Motivation:
It would be nice if Solid Errors could be "self cleaning" by periodically destroying old resolved records.

@fractaledmind let me know your thoughts on this.

  1. I've introduced a new model to handle the work, and I've picked an arbitrary number of 100 as the occurrence id interval to execute the deletions (execute the deletions after every 100 occurrence inserts)
  2. It is currently scoped to only delete resolved errors & occurrences associated with resolved errors
  3. I've used .delete_all to keep it fast
  4. I've used a transaction - probably not be necessary, but it is nice that it keeps it all together
  5. Maybe a different model name should be used? SolidErrors::Maintainer, SolidErrors::Destroyer, etc.?
  6. In the process of designing the tests, I also did the following:
    6.1 Identified several improvements to the test configuration & test dummy app
    6.2 Simplified the database structure in the test dummy app & removed the databases from the repo
    6.3 Renamed the existing test to conform to common Rails conventions (solid_errors_test.rb rather than test_solid_errors.rb)

@fractaledmind
Copy link
Owner

Just finishing up with RubyConf, will give this a review shortly

@ron-shinall
Copy link
Contributor Author

ron-shinall commented Nov 17, 2024

I've been thinking about the scope for destroying records. It's currently scoped only to resolved Errors. I believe it makes more sense to either:

  1. Remove the scope and treat it like a "sharp knife"...if that config value is set, unconditionally destroy all matching records
  2. Convert it to a hash and allow the user to specify the scope, perhaps something like this:
config.solid_errors.destroy_after = { duration: 30.days, only: :resolved }
config.solid_errors.destroy_after = { duration: 30.days, only: :unresolved } # not sure why this would be used
config.solid_errors.destroy_after = { duration: 30.days } # all records older than duration

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