-
-
Notifications
You must be signed in to change notification settings - Fork 83
Optionally skip housekeeping cron; auto-remove for NetBox v4.4+ #206
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: develop
Are you sure you want to change the base?
Conversation
|
@tyler-8 Please have a look into this and let me know if you need anything else |
lae
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.
Looking over this, a separate task should not be used.
Also, the same treatment should apply to git deployments that contain the upstream change. Specifically, netbox-community/netbox#19815. See install_via_git.yml for examples.
In fact, if my understanding after looking through that PR is correct, we don't exactly need a role variable to managing the cronjob? It should just exist or not exist depending on what NetBox version is installed, so the version comparison would be enough...though probably in the state attribute instead.
It looks like the new task is only for users currently using this playbook and upgrading to v4.4 - it cleans up the previously installed cron file. Otherwise, as I've tested myself, the housekeeping cron file will persist unless you manually delete it, so I think that's a reasonable addition. However, I do agree that a new configurable variable isn't necessary. We should do a version comparison or look for the presence of the relevant commit ID and register the variable to determine if action is needed just like already happens here:
|
tyler-8
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.
Instead of introducing netbox_housekeeping_cron_enabled, we should instead extend the already existing task that sets _netbox_git_contains_housekeeping to include checking for the commit ID that now removes the houskeeping feature.
However, the cleanup task is a good idea I think.
Right, but I'm suggesting that cleanup doesn't need a new task. Setting the |
I see what you mean now and agreed - dynamically adjusting the |
|
@PrakharJain1509 can we get an update on this? |
netbox-community/netbox@f3a8b9c >commit doesn't exist 🤔 |
|
I don't know if GitHub intends to fix this, but I will just mention that my previous comment/review is a response to Jain and is the most recent comment and the timestamp for Jain's was moved an hour ahead for some reason. (note the commit and comment/gha notification timestamps are basically an hour apart, give a minute for writing the comment)
which means it's time for our annual mention of https://www.youtube.com/watch?v=-5wpm-gesOY |


PR Description
NetBox v4.4 replaces the legacy housekeeping script with NetBox-managed scheduled jobs. This PR ensures we don’t create a stale cron entry on v4.4+ and allows operators to disable the cron explicitly.
Changes:
netbox_housekeeping_cron_enabled(default:true) to control whether the legacy housekeeping cron is managed.netbox_stable_version >= 3.0.0and< 4.4.0; for git installs, rely on the existing commit presence check.Files edited:
tasks/deploy_netbox.yml: Add version-aware guard, toggle, and cleanup task for the housekeeping cron.defaults/main.yml: Introducenetbox_housekeeping_cron_enabledvariable.Backwards compatibility:
false.Upgrade notes:
netbox_housekeeping_cron_enabled: falseTesting:
Example override:
group_vars:netbox_housekeeping_cron_enabled: falseFor PR Optionally skip housekeeping cron setup task #205