Skip to content

Conversation

@BasPH
Copy link
Contributor

@BasPH BasPH commented Aug 5, 2019

Make sure you have checked all steps below.

Jira

  • My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
    • https://issues.apache.org/jira/browse/AIRFLOW-4192
    • In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
    • In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
    • In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

  • Here are some details about my PR, including screenshots of any UI changes:

After #5010 ending up in an endless discussion, I split up the PR to decide about individual task context variables. This PR removes end_date and latest_date.

They were added in the early days of Airflow (45d14c1) and in the mailing list discussion @artwr was the only one to comment on it:

"both were a blend of things which were used by a backfill framework used internally at Airbnb. Latest date was used if you needed to join to a dimension for which you only wanted the latest version of the attributes in you backfill. end_date was used for time ranges where several days were processed together in a range to save on compute. I don't see an issue with removing them."

I think we can safely remove these variables without any issues.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Both context variables are untested, so no tests were changed.

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

  • Passes flake8

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

I've never used these

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Should not we add it to UPDATING.md ? Seems like even if almost no-one uses it, it's a breaking change.

@BasPH
Copy link
Contributor Author

BasPH commented Aug 6, 2019

I'll add them to UPDATING.md. Shall I also mention the removal of tables in this PR? Otherwise I'll make a new PR just for adding that one line to UPDATING.md.

@potiuk
Copy link
Member

potiuk commented Aug 7, 2019

What do you mean by removal of tables :)? Related PR ?

@BasPH
Copy link
Contributor Author

BasPH commented Aug 7, 2019

This PR: #5723

@potiuk
Copy link
Member

potiuk commented Aug 7, 2019

Yeah. Sure. That one is already merged, but in general it's better to keep UPDATING.md in the same commit as the change itself - easier to find where it came from (via git annotate for example).

@BasPH
Copy link
Contributor Author

BasPH commented Aug 7, 2019

Yea my bad, should definitely keep 1 change per PR.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

No worries :)

@potiuk potiuk merged commit c2227fc into apache:master Aug 7, 2019
@BasPH BasPH deleted the bash-remove-enddate-latestdate-4192 branch August 7, 2019 16:28
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.

3 participants