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

Fix disabling of interval cleanups #768

Merged

Conversation

zarqman
Copy link
Contributor

@zarqman zarqman commented Dec 11, 2022

This PR allows cleanup_interval_jobs and cleanup_interval_seconds to each be disabled by setting to nil or false. Since v3.0.0 (see #545), both would fallback to the default values.

Since disabling these worked in v2.x and CleanupTracker still allows nil, it seems this was an accidental regression. Tests added accordingly.

The ENV variable equivalents may also be disabled by setting to an empty string:
env GOOD_JOB_CLEANUP_INTERVAL_JOBS= good_job ...
This was already true, but is now tested.

The primary use-case is to allow one cleanup interval to be used, with the other disabled (that's the case here).

If both are set to nil, then auto cleanup is disabled. In light of the recent change to always preserve cron jobs (#767), that could be bad. However, setting both to nil does allow intentionally running cleanup externally, which may be a useful configuration for some. Accordingly, no attempt has been made to prevent both being disabled.

Finally, tagged on an update to ensure macOS on Intel can use precompiled versions of gems like nokogiri. I don't know if you prefer having such housekeeping tasks as separate PRs or not. Happy to split off if needed.

Then allows `bundle install` to use precompiled versions of gems like
nokogiri, which installs much faster.
Copy link
Owner

@bensheldon bensheldon left a comment

Choose a reason for hiding this comment

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

@zarqman thank you for finding this! Yep, this looks correct.

I do somewhat regret allowing nil to be a proper configuration value. I think ideally it would be false (for Ruby) or -1 (for ENV). But it is what is is for now.

Thank you again! 🙏🏻

@bensheldon bensheldon added the bug Something isn't working label Dec 12, 2022
@bensheldon bensheldon changed the title Allow interval cleanups to be disabled Fix disabling of interval cleanups Dec 12, 2022
@bensheldon bensheldon merged commit e37f519 into bensheldon:main Dec 12, 2022
@zarqman
Copy link
Contributor Author

zarqman commented Dec 12, 2022

@bensheldon, funny you mention it... I started with just false as well, and then also realized the legacy nature of nil. Arguably 0 could disable as well.

I suppose it could be added to the v4 changes list for when the time comes? (Or if you'd like me to prepare such a PR now and leave it sitting for v4, I'd be happy to do that while it's all fresh in my mind.)

And, thanks for the quick merge!

@zarqman zarqman deleted the fix-disabling-of-interval-cleanups branch December 12, 2022 19:46
@bensheldon
Copy link
Owner

@zarqman if you wanted to add a deprecation notice, that would be ideal for me remembering to change it in v4.

I think the CLI should use -1 to disable it, and Ruby config should use false, then the configuration object would coerce all that to either false or a positive integer.

Tell me I'm wrong here. But I think 0 would be a valid value to imply "run a clean up cycle after every execution".

@zarqman
Copy link
Contributor Author

zarqman commented Dec 19, 2022

In cleanup_interval_jobs, 0 is currently equivalent to 1.
In cleanup_interval_seconds, 0 is currently equivalent to setting cleanup_interval_jobs = 1, which is, well, odd.

For the sake of clear, intuitive behavior, I think 0 and -1 should both disable. If 1 is wanted, it should be set expressly. Setting SOME_ENV=0 is also a pretty common way to indicate disabled.

PR coming shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants