cron - implement all features for special period #38448
Conversation
|
This is going to need a lot of testing before considering merging I believe... I have a feeling that something isn't working correctly. I haven't managed to find a case that breaks it yet, but be warned. :) |
|
@swalladge OK. Keep us posted. :] |
|
Update: I've started work on a crontab module from scratch, and still deciding what the api should be exactly. The current cron module has some options and such that don't make sense and could be simpler. How open would the saltstack team be to a new crontab module with a new api? I'm also trying to find advice on how it should behave in cases. (For example how to handle/match jobs where the time is only partially specified, or what to do with existing jobs, etc.) Keen to hear any feedback or ideas, or even who I could contact about it. :) |
|
@swalladge I can't say I'm all that excited about changing the API for something like the crontab module without an extremely compelling reason. Could you expound on your reasoning for this? |
|
I am going to back up @cachedout here, we should not change the cron module api, I am open to a new module, but we should keep the api |
|
Replacing it completely is not something I'm excited to do. |
|
I should agree with @terminalmage that while I am open to a new module that preserves the api, I would not like to see a complete rewrite. |
|
Go Go Jenkins! |
1 similar comment
|
Go Go Jenkins! |
|
@swalladge Apologies for the extreme delay here. This has some automated tests for the cron state that fail as a result of these changes. Could you take a look? https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-ubuntu14-n/8666/ |
|
Bump @swalladge. Were you able to check the test failures? |
|
@cachedout sorry I haven't really had the time to look into this. I think the reason tests are failing is because the special time entries now have the identifier comment above them (where they didn't before). |
|
@rallytime Do you have a minute to peek at this? |
|
@cachedout Probably not for a couple of days, but I will add it to the list. :) |
|
Go Go Jenkins! |
|
@swalladge Would you be willing to rebase this against the current HEAD of develop? I want to be sure that the failures we're seeing here aren't residual failures from the state of the develop branch back in December, or tests that we need to address in regards to this change. That way I can assess the state of your changes more thoroughly to make sure no test failures sneak in that I thought were not related. After that, I'd be happy to take a look at the cron test failures, and anything else that might be related so we can get this in. :) |
As reported in #38425, previously when the 'special' keyword was used in states, or 'cron.set_special' module used. This commit merges code for set_job and set_special behind the scenes, thus implementing all features that were previously available with setting a job with minute, hour, etc. when 'special' is used. The api remains unchanged.
|
Done. The tests I can see failing for |
|
@swalladge After reviewing these test failures, I think they are actually pointing out a legitimate bug with this change. I don't see anywhere in this change where the Note that this change will cause some other problems with the tests because there are places where the expected default has been changed. For example, the |
|
By the way, I'd forgotten about this... I don't use the crontab module any more because I don't trust it, but rather the files modules to put cron files under Someone please feel free to close this pr or merge any changes that may be useful. :) |
|
Thanks for updating this @swalladge - we can always re-open this if you or someone else has time to come back to this. |
What does this PR do?
As reported in #38425, previously when the 'special' keyword was used in
states, or 'cron.set_special' module used. This commit merges code for set_job
and set_special behind the scenes, thus implementing all features that were
previously available with setting a job with minute, hour, etc. when 'special'
is used.
The api remains unchanged.
What issues does this PR fix or reference?
ref #38425
Previous Behavior
Use of 'special' keyword disabled use of identifier lines and other useful features that were available when not using 'special'.
New Behavior
Features implemented for when using 'special' keyboard as well.
Tests written?
Please let me know if/what tests should be written.