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 initializer which sets the TZ environment variable #1356

Merged
merged 1 commit into from Mar 16, 2016

Conversation

dsander
Copy link
Collaborator

@dsander dsander commented Mar 15, 2016

This properly propagates the configured TIMEZONE to the system TZ variable which is used by libv8 in the JavascriptAgent to access the current time.
#1355

This properly propagates the configured TIMEZONE to the system TZ variable which is used by libv8 in the JavascriptAgent
to access the current time.
@@ -0,0 +1,2 @@
# Set the timezone for the JavascriptAgent (libv8 only relies on the TZ variable)
ENV['TZ'] = Time.zone.tzinfo.canonical_identifier
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe ||=?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, shouldn't the TIMEZONE .env setting be the only source for the timezone Huginn uses?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess? I know many systems also set TZ, so it might be nice to use if
present? I'm not sure.
On Wed, Mar 16, 2016 at 1:29 AM Dominik Sander notifications@github.com
wrote:

In config/initializers/timezone.rb
#1356 (comment):

@@ -0,0 +1,2 @@
+# Set the timezone for the JavascriptAgent (libv8 only relies on the TZ variable)
+ENV['TZ'] = Time.zone.tzinfo.canonical_identifier

Not sure, shouldn't the TIMEZONE .env setting be the only source for the
timezone Huginn uses?


You are receiving this because you commented.

Reply to this email directly or view it on GitHub
https://github.com/cantino/huginn/pull/1356/files/c48ff63e7a96260540f9399201cbd7a23d65c9fa#r56296774

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought was to use the TIMEZONE .env variable as truth for the timezone the user selects. If we would callback to the system wide TZ when it's present, the only way to have a consistent timezone setting would be to add TZ to .env as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I support that, let's leave this as you have it. LGTM!
On Wed, Mar 16, 2016 at 9:40 AM Dominik Sander notifications@github.com
wrote:

In config/initializers/timezone.rb
#1356 (comment):

@@ -0,0 +1,2 @@
+# Set the timezone for the JavascriptAgent (libv8 only relies on the TZ variable)
+ENV['TZ'] = Time.zone.tzinfo.canonical_identifier

My thought was to use the TIMEZONE .env variable as truth for the
timezone the user selects. If we would callback to the system wide TZ
when it's present, the only way to have a consistent timezone setting would
be to add TZ to .env as well.


You are receiving this because you commented.

Reply to this email directly or view it on GitHub
https://github.com/cantino/huginn/pull/1356/files/c48ff63e7a96260540f9399201cbd7a23d65c9fa#r56368208

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, great 👍

dsander added a commit that referenced this pull request Mar 16, 2016
Add initializer which sets the TZ environment variable
@dsander dsander merged commit e5fcb24 into huginn:master Mar 16, 2016
@dsander dsander deleted the fix-javascriptagent-timezone branch March 16, 2016 21:48
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.

None yet

2 participants