[Airflow-1606] airflow.models.DAG.sync_to_db is no longer a static method

[ashb]

Dear Airflow maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the 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-1606

Description

This will need #2602 merged first (to make dag.logger available again)

Tests

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

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
  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"

Dear Airflow maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the 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-1606

Description

airflow.models.DAG.sync_to_db is declared as a static method, but it takes as it's first argument a DAG object (i.e. the same class that the method is delcared on). This is a bit odd

This will need #2602 (AIRFLOW-1605) merged first (to make dag.logger available again)

Tests

• My PR adds the following unit tests OR does not need testing for this extremely good reason: initdb is already tested.

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
  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"

[bolkedebruin]

LGTM. Please rebase, squash and use imperative for commit message. After successful ci I'll merge.

[ashb]

Squashed. Happy with the commit message?

[codecov-io]

Codecov Report

Merging #2605 into master will increase coverage by <.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #2605      +/-   ##
==========================================
+ Coverage   70.71%   70.71%   +<.01%     
==========================================
  Files         150      150              
  Lines       11564    11565       +1     
==========================================
+ Hits         8177     8178       +1     
  Misses       3387     3387

Impacted Files	Coverage Δ
airflow/utils/db.py	84.42% <100%> (-0.26%)	⬇️
airflow/models.py	86.98% <81.81%> (-0.03%)	⬇️
airflow/executors/dask_executor.py	81.39% <0%> (+2.32%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 956699f...16b1eca. Read the comment docs.

[Fokko]

I think you've missed one call:
https://github.com/apache/incubator-airflow/blob/master/airflow/jobs.py#L1717

[ashb]

Argh, well spotted! Edit, updated that one now.

[bolkedebruin]

Happy with the message. If you fix that last call ;-) then I'll merge.

[ashb]

Done.

[bolkedebruin]

BTW: as tests did not catch that missed refactor should we make it so (i.e. fail if it uses the old call)?

[ashb]

It turns out that (at least in Py3) this change is backwards compatible:

class Foo:

    def foo(self):
        print(self)


class Bar:
    @staticmethod
    def bar(x):
        print(x)


foo = Foo()
bar = Bar()

Foo.foo(foo)
foo.foo()
Bar.bar(bar)

And the output is:

<__main__.Foo object at 0x10f125dd8>
<__main__.Foo object at 0x10f125dd8>
<__main__.Bar object at 0x10f125e10>

So since the arg order was the same this missed one wasn't an actual problem. Which also means I'm not sure how we might test for it.

[bolkedebruin]

K, just assuming you refactored properly then :P

[bolkedebruin]

Oef, commit message wasn't entirely clean. We will survive.

[Fokko]

That was merged too soon, I think the amend of @ashb is missing:
https://github.com/apache/incubator-airflow/blob/master/airflow/jobs.py#L1717

[bolkedebruin]

Mmm yes it seems so. Anyone for a hot fix?

[ashb]

D'oh yes, missed the force push after final rebase :( A new PR against the same Jira ticket?

[ashb]

Ah someone is on it already 👍