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

PR #17199 changed trigger.now from local to UTC breaking automations #20110

Closed
pnbruckner opened this issue Jan 14, 2019 · 12 comments · Fixed by #21544
Closed

PR #17199 changed trigger.now from local to UTC breaking automations #20110

pnbruckner opened this issue Jan 14, 2019 · 12 comments · Fixed by #21544

Comments

@pnbruckner
Copy link
Contributor

Home Assistant release with the issue:
0.84.6

Last working Home Assistant release (if known):
0.80

Operating environment (Hass.io/Docker/Windows/etc.):
Raspbian 9.6 (stretch) on pi

Component/platform:
https://www.home-assistant.io/docs/automation/templating/#time

Description of problem:
For a time trigger, trigger.now used to be expressed in local time. Since 0.81 (PR #17199) it is now in UTC. This breaks automations that use trigger.now.hour.

Problem-relevant configuration.yaml entries:

automation:
  - alias: Period of Day
    trigger:
      - platform: time
        at: '05:45:00'
      - platform: time
        at: '22:00:00'
    action:
      service_template: >
        {% if trigger.now.hour == 5 %}
          script.pod_morning
        {% else %}
          script.pod_night
        {% endif %}

Traceback (if applicable):
none

Additional information:
Previous code:
https://github.com/home-assistant/home-assistant/blob/3abdf217bb0f41d6f8164321e689bbc551b3ca6a/homeassistant/helpers/event.py#L344-L355

You can see that now passed to async_run_job would be converted to local time.

New code:
https://github.com/home-assistant/home-assistant/blob/f22557098074c8d0c581ff47b18eb0e9ca5012f8/homeassistant/helpers/event.py#L360-L374

You can see that now passed to async_run_job is not converted to local time anymore.

@pnbruckner
Copy link
Contributor Author

Any word on whether or not this will be fixed?

@pnbruckner
Copy link
Contributor Author

@OttoWinter, since your PR changed the behavior of trigger.now, what are your thoughts on this?

@OttoWinter
Copy link
Member

@OttoWinter
Copy link
Member

OttoWinter commented Feb 3, 2019

Ah you mean the trigger data. Yes then I guess this is true. (though nothing ever documented it being in local time, so changing to UTC is within the documented behavior)

@pnbruckner
Copy link
Contributor Author

Did you see my example above? Time triggers are local time, so it makes sense that trigger.now.hour is also in local time. And it was. So although technically undocumented, it does change long standing behavior, which I believe was the right way for it to work, and it breaks existing automations. Can you restore the previous behavior by converting trigger.now back to local time (I believe in this call: hass.async_run_job(action, event.data[ATTR_NOW]))?

@pnbruckner
Copy link
Contributor Author

This not only affects trigger.now.hour, but also trigger.now.weekday(), etc. E.g.:

{% set dt_utc = strptime(
     '2019-02-20 03:00:00+0000', '%Y-%m-%d %H:%M:%S%z') %}
{% set dt_loc = strptime(
     '2019-02-19 21:00:00-0600', '%Y-%m-%d %H:%M:%S%z') %}
{{ dt_utc.timestamp() }}
{{ dt_loc.timestamp() }}
{{ dt_utc.weekday() }}
{{ dt_loc.weekday() }}

Results in:

1550631600.0
1550631600.0
2
1

So you can see that two datetimes that represent the same moment in time (i.e., the same timestamp), where one is in UTC, and the other is in local time, will not result in the same weekday.

So if trigger.now is in UTC, then any automation that uses trigger.now.weekday() (or many of the other time functions, such as day(), month(), year(), etc.) might also break.

@pnbruckner
Copy link
Contributor Author

@OttoWinter, not sure if you've seen my latest two comments. Based on this additional information, do you still think it should not be changed?

@miwagner1
Copy link

miwagner1 commented Feb 21, 2019

I have spent days trying to figure out why my good morning scripts were broken and I never ever would have guessed that the time is in UTC. I believe that many more people are going to be making the same mistake and getting frustrated assuming that the time is supposed to be local time like it is with everything else. What about daylight savings changes? This would screw up my good morning workflow.

@emontnemery
Copy link
Contributor

@pnbruckner can you test the fix in #21544?

@pnbruckner
Copy link
Contributor Author

@emontnemery I will, first thing tomorrow. But now that the time trigger has been split, don't you need to make the same change in automation/time_pattern.py? In any case, thanks for working on this!

@emontnemery
Copy link
Contributor

now that the time trigger has been split, don't you need to make the same change in automation/time_pattern.py

You're right of course. #21544 should now cover time pattern as well (and implementation is a bit different as requested by balloob).

@pnbruckner
Copy link
Contributor Author

and implementation is a bit different as requested by balloob

That makes sense, because that is where the problem was introduced (as I described above.) 😃

I tested both a time and time_pattern trigger, before and after the change. Before trigger is UTC, after it's local. I would say that fixes it. Thanks!!

@ghost ghost removed the in progress label Mar 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants