Skip to content

Conversation

@22quinn
Copy link
Contributor

@22quinn 22quinn commented Jul 6, 2020

Fixes #9609


Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Unit tests coverage for changes (not needed for documentation changes)
  • Target Github ISSUE in description if exists
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

Can you please rebase on the master branch to remove conflicts?

@22quinn 22quinn force-pushed the time-sensor-timezone branch from b754a8b to b88cd95 Compare July 18, 2020 03:18
@22quinn
Copy link
Contributor Author

22quinn commented Jul 18, 2020

Can you please rebase on the master branch to remove conflicts?

Done. Thanks for the review.

Do you mind also taking a look at #9697? It offers idempotence for target_time.

@kaxil kaxil merged commit b34ba87 into apache:master Jul 18, 2020
@22quinn 22quinn deleted the time-sensor-timezone branch July 19, 2020 03:36
@freget
Copy link
Contributor

freget commented Jul 19, 2020

Cool, thank you!

@wangze999999
Copy link

wangze999999 commented Jul 19, 2020 via email

@JeffryMAC
Copy link

I have two questions:

  1. When this is released?
  2. If someone is writing timezone aware dag. it might be very confusing when the TimeSensor works with default setting that can't be change. I think to make this change complete maybe it should take first the Timezone from Dag if given otherwise the default from the airflow settings.

@22quinn
Copy link
Contributor Author

22quinn commented Jul 19, 2020

2. If someone is writing timezone aware dag. it might be very confusing when the TimeSensor works with default setting that can't be change. I think to make this change complete maybe it should take first the Timezone from Dag if given otherwise the default from the airflow settings.

Good point. I was not aware of the existence of timezone-aware dags. Just created another PR to achieve this: #9882

@kaxil kaxil added this to the Airflow 1.10.13 milestone Aug 26, 2020
kaxil pushed a commit that referenced this pull request Nov 20, 2020
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
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.

TimeSensor triggers immediately when used over midnight (UTC)

5 participants