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

Implement timezone support + Restore default UTC interpretation #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chdanielmueller
Copy link
Contributor

Before v1.5.0 the library did always use UTC time to parse the cron syntax.
The change to cron-parser did alter this behavior.
It now uses the local timezone as default.
My proposal is to revert to the previous behavior as default to not create any breaking changes.

Another proposal is to allow the change of the tz of the cron-parser using a new option in mongodb-cron.
This would allow the user to use any timezone he desires for the parsing of the intervals.
The sleepUntil field is still saved as a UTC timestamp in any case.

closes #8

@xpepermint
Copy link
Owner

xpepermint commented Apr 25, 2019

Thank you @chdanielmueller. This is similar to what I wrote yesterday. We have to do two more things

  • The interpretation is not sufficient. When dates are modified in the database by the MongoCron process, the date must be formatted based on the configuration. For example, if I insert sleepUntil to the database using a timezone, the updated date must also be stored in this format and not the default Date() format. We can use moment-timezon package here.
  • We have to write tests to prove the functionality works. I played around with this yesterday and didn't find a nice solution just yet.

@chdanielmueller
Copy link
Contributor Author

To point 1:
The default Date() format returns a timestamp in UTC timezone. For saving to the database I agree with you to say that this is best practice.
This way we can also work around any other issues which might occur if the package works with timezones for all the logic.
Basically just use the timezone for cron parsing.
Or did I get something wrong?

To point 2:
That's why I didn't add a test in my first PR. Don't know how to write a reasonable test for this.

@xpepermint
Copy link
Owner

  1. Yes. I'll fix that first.
  2. I need to figure this out somehow :). Let me know if you come up with a reasonable code snippet :).

@chdanielmueller
Copy link
Contributor Author

@xpepermint Any timeline on this?

@xpepermint
Copy link
Owner

Hey @chdanielmueller. Sorry, no timeline. My todo is filled with other tasks right now :(.

@chdanielmueller
Copy link
Contributor Author

@xpepermint Any news?

@xpepermint
Copy link
Owner

Sorry, you'll have to use a fork until this is merged.

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

Successfully merging this pull request may close these issues.

Timezone / Summer time support
2 participants