Allow cron.present to change a timespec from non-special to special #60998
Conversation
asomers
force-pushed
the
cron-antispecial
branch
2 times, most recently
from
369c7e1 to
e52d0f0
Compare
|
The test failure looks unrelated to this PR. |
|
ping @twangboy would you be able to review this PR sometime? |
asomers
force-pushed
the
cron-antispecial
branch
from
e52d0f0 to
7de0e04
Compare
|
rebased @twangboy |
|
@asomers Looks good. Can you please add a changelog to this PR? Thanks! |
asomers
force-pushed
the
cron-antispecial
branch
from
7de0e04 to
733fb57
Compare
|
Rebased and changelog added, @twangboy @garethgreenaway |
|
re-run all |
salt/states/cron.py
Outdated
| @@ -355,6 +355,9 @@ def present( | |||
| return ret | |||
|
|
|||
| if special is None: | |||
| antidata = __salt__["cron.rm_special"]( | |||
There was a problem hiding this comment.
I would prefer the logic be in the set_job and set_special functions. Those functions do some logic to see what is currently set in the crontab and see if there is a difference. We should be able to add the logic in those functions to determine if there needs to be an update instead of removing it first.
There was a problem hiding this comment.
Ok, I can change it. However, I just discovered that all of the mock expectations in tests/unit/modules/test_cron.py are broken, so I'll have to fix that entire file first. Sigh.
There was a problem hiding this comment.
@Ch3LL I made your requested change. And it turned out that only one of the existing modules/cron tests needed fixing.
Previously when changing a cron.present job from using a normal time specification to special or vice-versa, it would duplicate the entry in the crontab.
The original assertion used "mock_method.call_args.assert_called_with", which does nothing at all. Correct usage is either "mock_method.call_args == ..." or "mock_method.assert_called_with(...)"
asomers
force-pushed
the
cron-antispecial
branch
from
d84dd96 to
051af4e
Compare
This allows the cron.set_job and cron.set_special commands to change cron entries from special to non-special or vise-versa. It also removes all changes to states/cron from this PR's original commit, while retaining the behavioral changes to the cron.present state
asomers
force-pushed
the
cron-antispecial
branch
from
051af4e to
54ac70f
Compare
What does this PR do?
Previously when changing a cron.present job from using a normal time
specification to special or vice-versa, it would duplicate the entry in
the crontab.
What issues does this PR fix or reference?
Fixes: #60997
Previous Behavior
cron.present would duplicate cron jobs when changing timespec from non-special to special
New Behavior
cron.present will delete the old non-special cronjob when creating a special one with the same identifier
Merge requirements satisfied?
Commits signed with GPG?
Yes