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

cron job times are interpreted as UTC timezone #74

Closed
snrmwg opened this issue Jan 13, 2021 · 19 comments · Fixed by #76
Closed

cron job times are interpreted as UTC timezone #74

snrmwg opened this issue Jan 13, 2021 · 19 comments · Fixed by #76
Assignees
Labels
help wanted Extra attention is needed

Comments

@snrmwg
Copy link
Contributor

snrmwg commented Jan 13, 2021

When I define a job with cron interval then the resulting execution times seem to be in UTC timezone. Is it possible to use local timezone?

@niftylettuce
Copy link
Contributor

It should be using local timezone machine, is it not? Might be something in @breejs/later

@snrmwg
Copy link
Contributor Author

snrmwg commented Jan 13, 2021

It is not. Would be great if this is fixed. Any hint where one should look? Or anyone willing to look after it?

@niftylettuce
Copy link
Contributor

Maybe start by looking through legacy docs at https://bunkat.github.io/later/ - I don't have the time but I am pretty sure I had reproduced this too a while back.

@niftylettuce
Copy link
Contributor

cc @shadowgate15

@niftylettuce
Copy link
Contributor

cc @naz

@shadowgate15
Copy link
Member

Later, which is what we use on the back-end to create the schedule, creates agnostic dates. However, it can be set to local time by doing later.date.localTime(); then generate the cron expression using later. You will then be able to pass that schedule into bree. Might be a good setting to add to config or just use local time by default.

@niftylettuce
Copy link
Contributor

We should def have this as a default, and since its breaking we'll do major semver bump. We can also note it in README/changelog. Anyone care to do a PR? ❤️ 🎉

@naz
Copy link
Member

naz commented Jan 13, 2021

Don't have time to have a deep look into it right now. But would happily review a PR if needed!

@naz naz added the help wanted Extra attention is needed label Jan 13, 2021
@hovancik
Copy link

Same issue for interval: 'at 1:26 pm'

@snrmwg
Copy link
Contributor Author

snrmwg commented Jan 15, 2021

Just forked the repo and trying a PR ...

Should a later.date.localTime(); in the beginning of index.js do the job?

And I'm struggeling how to test the effect:

test('job created with cron string is using local timezone', (t) => {
  const bree = new Bree({
    root,
    jobs: [{ name: 'basic', cron: '0 18 * * *' }]
  });

  
  bree.run('basic')

  // ???

});

@shadowgate15
Copy link
Member

You will probably have to do later.date.localTime() in every file that imports later.

for the test you should be able to use bree.timeouts.basic to get the date with later utilities and then compare timezones from the date object... I think. at least that is my first thought.

@snrmwg
Copy link
Contributor Author

snrmwg commented Jan 17, 2021

Created a PR including extended unit tests. Locally all tests are green (tested with Node 12 and 15) but sadly in TravisCI the run with Node 12 is red with one failing test at a complete different location. Same test was also red in different open PR #75.

Anyone able to spot the reason and a possible fix?

@hovancik
Copy link

You should be able to set timezone on CI

@snrmwg
Copy link
Contributor Author

snrmwg commented Jan 17, 2021

Timezone is not the problem of the failing test as far as I can see. And it is also failing in other PR that does not include the local date fix.

@snrmwg
Copy link
Contributor Author

snrmwg commented Jan 19, 2021

Cannot get the 'run > job terminates after set time' test green on TravisCI. Locally it's always green. It uses closeWorkerAfterMs that is shorter than job runtime so that it should get terminated. But the received exit event receives a code of 0 instead of 1. No connection to timezone. Any help appreciated. @naz @shadowgate15

@shadowgate15
Copy link
Member

shadowgate15 commented Jan 19, 2021 via email

shadowgate15 added a commit that referenced this issue Feb 5, 2021
@nickperkinslondon
Copy link

Is this fixed?
I am just starting to use Bree, and when I say "at 6:00pm"....it actually runs at 2:00pm local time ( when it is 6:00pm in Greenwich England ).
I can't work-around this by adding 4 hours to the "interval" because it's a whole parsed language thing...
So, I am left having to ask my users to enter their desired "time-of-day" in GMT!
( embarrassing )
I think that "later" has a "localTime" option, but how do I get to that through "bree"? ( and why doesn't bree do this by default? )

@shadowgate15
Copy link
Member

It is fixed and we do use localTime. What version of Bree are you using and how are you setting interval? And is the system that Bree is run on in the same timezone as your user? localTime just means that it will use the timezone of the system that it is run in, not necessarily UTC. So if your system is set to UTC, then it will run in UTC.

@nickperkinslondon
Copy link

Thanks for your response,
Knowing that "it's not you, it's me" gave me the courage to get to the bottom of this...

The problem was that I was running my app in a Docker container, and the timezone inside the container was not right. I fixed it by adding an environment variable TZ=America/Toronto for the container. Now everything is right.

Thanks again!...awesome package!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants