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

Add a new cron function #5780

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Add a new cron function #5780

wants to merge 9 commits into from

Conversation

chain312
Copy link
Contributor

Problem description

image
When I used core.st2.CronTimer to create a scheduled task, I found that I could not specify the start time and end time

Solution

As can be seen from the source code, the scheduled task is implemented by aspscheduler and the parameters of the scheduled task are transparently transmitted. aspscheduler has this function. In use, parameters can be specified as start_date and end_date. You only need to change CRON_PARAMETERS_SCHEMA to add these two parameters

@pull-request-size pull-request-size bot added the size/XS PR that changes 0-9 lines. Quick fix/merge. label Oct 16, 2022
@CLAassistant
Copy link

CLAassistant commented Oct 16, 2022

CLA assistant check
All committers have signed the CLA.

@chain312
Copy link
Contributor Author

set the milestone

@amanda11
Copy link
Contributor

Thanks for looking into this enhancement, and working out how it can be achieved. It's great to get new contributions.

The CronTrigger is really supposed to be mimicing the functionality of a cron job, and with a cron job you don't really get start and end times. However, as you mention https://apscheduler.readthedocs.io/en/3.x/modules/triggers/cron.html does support start and end dates.

It would be useful to do a post on the StackStorm development slack channels to see what people think of this enhancement request, as I'm not sure if its best to start a new timer type rather than extend the cron timer. We can extend the crontimer as you have mentioned, but that's not something that is in cron itself, and ties ourself to using apscheduler. So, I'd like to get the view of some others in the TSC about whether this is the right place to put it. But, as this functionality is not available anywhere else it would be good functionality to extend StackStorm, and this would be the simplest way of adding it.

If we do go the route you've coded, then the only addition I think this needs is some unit-tests added. There are a variety of tests on CronTimer with invalid formats etc in ./st2api/tests/unit/controllers/v1/test_rules.py, so it would be good to get these extended with some valid and invalid examples that use the start_date and end_date parameters.

@amanda11 amanda11 added this to the 3.9.0 milestone Oct 17, 2022
@rush-skills
Copy link
Member

Thanks for looking into this enhancement, and working out how it can be achieved. It's great to get new contributions.

The CronTrigger is really supposed to be mimicing the functionality of a cron job, and with a cron job you don't really get start and end times. However, as you mention https://apscheduler.readthedocs.io/en/3.x/modules/triggers/cron.html does support start and end dates.

It would be useful to do a post on the StackStorm development slack channels to see what people think of this enhancement request, as I'm not sure if its best to start a new timer type rather than extend the cron timer. We can extend the crontimer as you have mentioned, but that's not something that is in cron itself, and ties ourself to using apscheduler.

I agree with @amanda11 here. It might make sense for it to be a new timer type (core.st2.ScheduledCronTimer?), so that
the new functionality can be kept separate and minimize changes, if any, for other users.

With that said, I would love to have this included and can try to get this rolling for the next release.

@chain312
Copy link
Contributor Author

Thanks for looking into this enhancement, and working out how it can be achieved. It's great to get new contributions.
The CronTrigger is really supposed to be mimicing the functionality of a cron job, and with a cron job you don't really get start and end times. However, as you mention https://apscheduler.readthedocs.io/en/3.x/modules/triggers/cron.html does support start and end dates.
It would be useful to do a post on the StackStorm development slack channels to see what people think of this enhancement request, as I'm not sure if its best to start a new timer type rather than extend the cron timer. We can extend the crontimer as you have mentioned, but that's not something that is in cron itself, and ties ourself to using apscheduler.

I agree with @amanda11 here. It might make sense for it to be a new timer type (core.st2.ScheduledCronTimer?), so that the new functionality can be kept separate and minimize changes, if any, for other users.

With that said, I would love to have this included and can try to get this rolling for the next release.

@rush-skills I posted a discussion of this feature in my slack channel, but no one talked to me about it. This feature can be used to start a scheduled task at a certain time of day. For example, I started a vulnerability scan task after 12 o 'clock on January 12. I also think we could add a new type of trigger.

@chain312
Copy link
Contributor Author

Thanks for looking into this enhancement, and working out how it can be achieved. It's great to get new contributions.
The CronTrigger is really supposed to be mimicing the functionality of a cron job, and with a cron job you don't really get start and end times. However, as you mention https://apscheduler.readthedocs.io/en/3.x/modules/triggers/cron.html does support start and end dates.
It would be useful to do a post on the StackStorm development slack channels to see what people think of this enhancement request, as I'm not sure if its best to start a new timer type rather than extend the cron timer. We can extend the crontimer as you have mentioned, but that's not something that is in cron itself, and ties ourself to using apscheduler.

I agree with @amanda11 here. It might make sense for it to be a new timer type (core.st2.ScheduledCronTimer?), so that the new functionality can be kept separate and minimize changes, if any, for other users.

With that said, I would love to have this included and can try to get this rolling for the next release.

@rush-skills I saw the problem and thought we could create a trigger that exposed all the aspscheduler parametersst2 cron timer was missed

@rush-skills
Copy link
Member

@rush-skills I saw the problem and thought we could create a trigger that exposed all the aspscheduler parametersst2 cron timer was missed

What is the default behaviour when the start_date or end_date is not specified? Does it work like the old version and thus retains backward compatibility? In that case, I would be happy to merge this.

But I think some tests should be added if possible to check the above. @amanda11 Do you know if there is a way to test this?

@amanda11
Copy link
Contributor

amanda11 commented Feb 7, 2023

st2common/unit/test_trigger_services.py is where there are some unit-tests for the crontimer, so if keeping as the same trigger and to prove backwards compatiblity. We should have some tests in there with and without the start/end time. It uses test rules that are setup in: st2tests/st2tests/fixtures/generic/rules, so those could be expanded.

I couldn't find any integration tests that check times actually go off when you expect...

@chain312
Copy link
Contributor Author

st2common/unit/test_trigger_services.py is where there are some unit-tests for the crontimer, so if keeping as the same trigger and to prove backwards compatiblity. We should have some tests in there with and without the start/end time. It uses test rules that are setup in: st2tests/st2tests/fixtures/generic/rules, so those could be expanded.

I couldn't find any integration tests that check times actually go off when you expect...
@rush-skills
I'll write test cases later when I have time

@pull-request-size pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/XS PR that changes 0-9 lines. Quick fix/merge. labels Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M PR that changes 30-99 lines. Good size to review. status:under discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants