Extend repeat schedule functionality #676 by UrekD · Pull Request #677 · bugy/script-server

UrekD · 2023-06-14T08:31:14Z

Implement functionality discussed in #676:

Option to define when task should end
Implemented new logic to handle the ending (arg passing, json writing, schedule handling)
Implemented new UI elements to select the end type

…f the repetable task should end

bugy · 2023-06-16T20:10:40Z

Hi @UrekD sorry for a late response. Unfortunately I'm a bit busy this week, so don't have time to review. Will try to check the pull request within the next several days.
And thanks a lot for the pull request!

bugy · 2023-06-26T19:57:13Z

                self._execution_service.add_finish_listener(cleanup, execution_id)
+
+            if job.schedule.endOption == 'after':
+                job.schedule.endArg -= 1


What do you think about not overriding the config file on every execution, but rather count how many executions could fit between now and start_datetime?
e.g. if start_datetime=19:30, max_count=5, now=22:12. Then until now there were 6 executions (19:30, 20:00, 20:30, 21:00, 21:30, 22:00) and there is no need to execute anymore? For this, we could reuse scheduling.schedule_config.ScheduleConfig.get_next_time

Sorry, I'm not really understanding what you mean here as start is in the past? I think executions shouldn't be tied to exact next execute time as this makes this option obsolete in my opinion as this can be achieve with end time. This option would be more of a guarantee execution of the job x times so even downtime wouldn't effect the job, which requires constant change on execution even in the file.

Alright, makes sense. Another proposal: may be instead of overriding the maxCount, we could save one more parameter: executionsCount (i.e. how many times the script was executed). So that we will keep the original maxCount value as a source of truth

Great idea, how about including executionsCount on all repeatable tasks to offer some statistic about how many times the job ran in general.

Sounds good to me

Implemented.

bugy · 2023-06-27T20:46:15Z

+
+        end_option = incoming_schedule_config.get('end_option')
+        prepared_schedule_config.end_option = end_option
+        if end_option == 'on':


Should we call the options max_executions and end_datetime? For me on and after are a bit confusing

Agreed, renamed em.

bugy · 2023-06-27T20:51:36Z

+def _read_datetime(incoming_schedule_config, key):
+    datetime_value = model_helper.read_datetime_from_config(key, incoming_schedule_config)
+    if datetime_value is None:
+        raise InvalidScheduleException('%1 is required', key)


Should it be '%1 is required' % key? Or f'{key} is required'

Indeed should have been f'..., fixed!

bugy · 2023-06-27T20:52:49Z

+        if end_option == 'on':
+            prepared_schedule_config.end_arg = _read_datetime(incoming_schedule_config, 'end_arg')
+        elif end_option == 'after':
+            prepared_schedule_config.end_arg = model_helper.read_int_from_config('end_arg', incoming_schedule_config)


Please verify that the value is not None

Implemented

Seen the error in my ways, I have re done it to be more in line with the previous. I think when checking for execution count it doesn't make sense to error if it's not available but just to set it to 0.

bugy · 2023-06-27T20:53:36Z

        next_datetime = schedule.get_next_time()
+
+        if schedule.end_option == 'on':
+            if date_utils.is_past(schedule.end_arg) or next_datetime > schedule.end_arg:


I think date_utils.is_past(schedule.end_arg) is redundant

Yep checked and removed.

bugy · 2023-06-27T20:54:26Z

+      endTime: endDay.toTimeString().substr(0, 5),
      id: null,
      repeatPeriod: 1,
+      executeCount: 1,


Can we call it maxExecuteCount?

…_arg, increment executions and check for max_exec

bugy

Hi @UrekD I think we are almost there :) I have only one big concern, about making this new feature optional

bugy · 2023-07-03T19:08:05Z



+def _read_executions_count(incoming_schedule_config):
+    executions_count = model_helper.read_int_from_config('executions_count', incoming_schedule_config)


You could pass default parameter to read_int_from_config

bugy · 2023-07-03T19:11:41Z

+            if job.schedule.repeatable:
+                job.schedule.executions_count += 1
+
+                file_utils.write_file(


Can we call save_job ?

bugy · 2023-07-03T19:22:04Z

+    if end_option is None:
+        raise InvalidScheduleException('end_option is required for repeatable schedule')


I think None end_option should be allowed:

existing configurations don't have it

we should make manual configuration (via json) more user-friendly, i.e. don't require unnecessary configurations

bugy · 2023-07-03T19:29:26Z

+        elif self.end_option == 'never':
+            result['end_option'] = 'never'


Should we just skip it? As in a comment above, I would suggest to completely omit end_... configuration if it's not needed.

UrekD · 2023-07-03T21:07:31Z

Hi @UrekD I think we are almost there :) I have only one big concern, about making this new feature optional

Hey @bugy, how so? I'm guessing maybe from an administrative standpoint? As the functionality shouldn't in no way hinder the functionality as it was before, if you don't select anything else. I see the point about not requiring a value for never ending schedules that was oversight on my end, which actually thought about not implementing in a current way but not for that reason :D I'll address the new stuff tomorrow, thanks!

bugy

Sorry for delay and lots of previous comments
Looks good to me now, thanks a lot for the contribution! 👍 🎉

UrekD · 2023-07-10T12:55:45Z

Sorry for delay and lots of previous comments Looks good to me now, thanks a lot for the contribution! 👍 🎉

No worries, thank you!

vnghia · 2023-12-29T06:28:01Z

Can we avoid overwriting the configuration file ? Because I am using an Infra-as-code approach and everything is generated by some kind of code and set to read-only afterward. So this failed on my server.

script-server/src/scheduling/schedule_service.py

Lines 155 to 158 in 7ea0180

    
           if job.schedule.repeatable: 
        
               job.schedule.executions_count += 1 
        
               self.save_job(job)

Maybe add a is_forever attribute to the schedule_config and we won't touch the file again if it has is_forever. I can send a PR for that.

UrekD · 2023-12-29T13:39:51Z

Can we avoid overwriting the configuration file ? Because I am using an Infra-as-code approach and everything is generated by some kind of code and set to read-only afterward. So this failed on my server.

script-server/src/scheduling/schedule_service.py

Lines 155 to 158 in 7ea0180

if job.schedule.repeatable:

job.schedule.executions_count += 1

self.save_job(job)

Maybe add a is_forever attribute to the schedule_config and we won't touch the file again if it has is_forever. I can send a PR for that.

Yeah that sounds good maybe better calling it allow_write. Personally I'd use a modified image for deployment, as with the new var implementation you will also need to handle a schedule that is set to count for it to not fire forever if it can't keep track of the count, not sure if it's worth the hassle.

vnghia · 2023-12-29T14:15:15Z

I think I only need it not to touch the config files that are persistent, i.e files with no expiration regardless time or execution count. The file with expiration can still be overwritten because usually these files are generated dynamically while the former can be generated and managed with Infra-as-code.

So we can check if the file will expire and overwrite it or just leave it there. The execution count will still be kept track inside the original file.

UrekD added 12 commits June 10, 2023 16:02

Adjust the schedule box to include new VISUAL elements for selectioni…

5722b9d

…f the repetable task should end

Parse end args to the backend

8a3c7bb

Parse values to JSON

d7dbe12

Input validation and endDate parsing

3519f37

Refactor and optimize building of JSON's to only include needed data

647779b

Prevent execute after end time

db4aa2d

Improve scheduling logic for count

5ada7fd

Decrease count only on successful execute and edit json

a6562f7

Cleaup for decrease count in json

464eca7

Schedule box fix height

e0b77cc

Fix missing DIV and reformat using formatter

e094d41

Fix comparing next ex and end date

b033c9d

bugy requested changes Jun 26, 2023

View reviewed changes

UrekD added 6 commits June 26, 2023 23:22

Revert web formatting

a5e99f2

Formatting

be3bb70

Remove redundant check

db80a76

Convert to snake_case

784e7c4

_read_datetime validation

a0bf188

Fix model snake case

170609a

bugy requested changes Jun 27, 2023

View reviewed changes

UrekD and others added 9 commits June 28, 2023 17:48

Fix exception formatiing

c85fc7b

Removed redundant check

90a7909

Validate end_option is not none

afe7f1a

vue maxExecuteCount

bd37d61

Rename on and after

4731af4

Implement execution count for repetables, validate end_option and end…

24d8f83

…_arg, increment executions and check for max_exec

Fix dict not parsing end_option for never

21f4a5c

Removed an extra , in array same functionality

d4c30ac

Merge branch 'bugy:master' into master

c8c8a2b

bugy requested changes Jul 3, 2023

View reviewed changes

UrekD and others added 4 commits July 4, 2023 09:51

Merge branch 'bugy:master' into master

3031151

Use save_job for saving count

5143f66

Removed unneeded function for read execution cpunt

3f38a06

Have no end_arg for repetable without end condition

59a7bf1

bugy approved these changes Jul 10, 2023

View reviewed changes

bugy merged commit 1940e9e into bugy:master Jul 10, 2023



		def _read_executions_count(incoming_schedule_config):
		executions_count = model_helper.read_int_from_config('executions_count', incoming_schedule_config)

		if end_option is None:
		raise InvalidScheduleException('end_option is required for repeatable schedule')

		elif self.end_option == 'never':
		result['end_option'] = 'never'

Conversation

UrekD commented Jun 14, 2023

Uh oh!

bugy commented Jun 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UrekD Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bugy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UrekD commented Jul 3, 2023

Uh oh!

bugy left a comment

Choose a reason for hiding this comment

Uh oh!

UrekD commented Jul 10, 2023

Uh oh!

vnghia commented Dec 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UrekD commented Dec 29, 2023

Uh oh!

vnghia commented Dec 29, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

UrekD Jun 26, 2023 •

edited

Loading

vnghia commented Dec 29, 2023 •

edited

Loading