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

run-on-startup optional param for all jobs #184

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

Conversation

jtbry
Copy link

@jtbry jtbry commented Apr 9, 2022

This pull request is in relation to Issue #79. I also have a use case for a data polling service that I would like to run once when the ofelia docker service starts up and then schedule from there. I stumbled across the issue and saw it open for help-wanted.

I added the RunOnStartup option to the BareJob struct to allow for it to apply to all job types.
Also added appropriate testing for RunOnStartup to the scheduler test suite
Also added documentation regarding the run-on-startup param to the jobs documentation

@jtbry jtbry mentioned this pull request Jul 8, 2022
@ec-ecss
Copy link

ec-ecss commented Jul 8, 2022

@jtbry I've got a litte question with your PR
The execution via run-on-startup is actually blocking every other ofelia execution
As long as the command is not terminated, there is no other job registration
So I can't lauch, for exemple, a script that never ends (such a file listener in a specific folder)
My workaround is to create a intermediary script with "&" that call the effective script (the "&" is not correctly parsed in the ofelia command line attribute )
Do you think it's a bug or a feature ?

@jtbry
Copy link
Author

jtbry commented Jul 8, 2022

@CaptainKant it is most likely a bug. The on startup jobs are started as parsed, in my testing I did not see a noticeable delay or anticipate a long blocking scenario. I’ll look at how other jobs are ran in a non blocking manner and see if it can be applied

@ec-ecss
Copy link

ec-ecss commented Jul 8, 2022

@CaptainKant it is most likely a bug. The on startup jobs are started as parsed, in my testing I did not see a noticeable delay or anticipate a long blocking scenario. I’ll look at how other jobs are ran in a non blocking manner and see if it can be applied

Ok. Just to let you know, I just noticed my workaround cause high cpu usage of the main docker-desktop process, for a strange reason.

@jtbry
Copy link
Author

jtbry commented Jul 8, 2022

@CaptainKant it is most likely a bug. The on startup jobs are started as parsed, in my testing I did not see a noticeable delay or anticipate a long blocking scenario. I’ll look at how other jobs are ran in a non blocking manner and see if it can be applied

Ok. Just to let you know, I just noticed my workaround cause high cpu usage of the main docker-desktop process, for a strange reason.

This should be fixed in the most recent commit. The startup job now runs in a goroutine to prevent blocking. You should be able to use it normally now without your workaround.

@escoand
Copy link

escoand commented Oct 4, 2022

When will this be released as a new docker image?

@jtbry
Copy link
Author

jtbry commented Oct 4, 2022

When will this be released as a new docker image?

You can use this image including this functionality jtbry/ofelia:latest. I have published this image on my own from my fork while waiting for merge.

@ghostlexly
Copy link

I need this so bad 👍

siccous added a commit to BTLzdravtech/ofelia that referenced this pull request Nov 7, 2022
PR mcuadros#184 run-on-startup optional param for all jobs
@al-tr
Copy link

al-tr commented Mar 12, 2023

if you use docker compose, don't forget to add

...
  ofelia:
    depends_on: 
      - service-names
      - you-want-to
      - run-jobs-in
...

otherwise ofelia is very likely to start first and hit an unprepared container, and you'll have to manually restart it to rertigger the jobs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants