Skip to content

[AIRFLOW-6864] Make airflow/jobs pylint compatible #7484

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.

Sign up for GitHub

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

Jump to bottom
Merged
turbaszek merged 1 commit into from
Feb 25, 2020
Merged

[AIRFLOW-6864] Make airflow/jobs pylint compatible #7484

turbaszek merged 1 commit into from
Feb 25, 2020

Conversation

@turbaszek
Copy link
Member

@turbaszek turbaszek commented Feb 21, 2020 •
edited
Loading

This PR makes airflow/jobs pylint compatible and removes the cyclic dependency created by importing BackfillJob to access BackfillJob.ID_PREFIX

Issue link: AIRFLOW-6864

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

  • Description above provides context of the change
  • Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
  • Unit tests coverage for changes (not needed for documentation changes)
  • 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.

* For document-only changes commit message can start with [AIRFLOW-XXXX].

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.

@boring-cyborg boring-cyborg bot added area:CLI area:Scheduler including HA (high availability) scheduler area:webserver Webserver related Issues labels Feb 21, 2020
@turbaszek turbaszek requested a review from mik-laj February 21, 2020 12:46
@turbaszek
Copy link
Member Author

turbaszek commented Feb 21, 2020

Depends on #7482

turbaszek
turbaszek commented Feb 21, 2020
View reviewed changes
@turbaszek turbaszek requested review from kaxil and potiuk February 21, 2020 12:49
@turbaszek turbaszek removed area:CLI area:webserver Webserver related Issues labels Feb 21, 2020
@turbaszek turbaszek force-pushed the AIRFLOW-6864-pylint-airflow-jobs branch from fadbf43 to e536fab Compare February 21, 2020 13:44
@turbaszek turbaszek force-pushed the AIRFLOW-6864-pylint-airflow-jobs branch 4 times, most recently from cc5ca91 to 84bf211 Compare February 22, 2020 12:41
@boring-cyborg boring-cyborg bot added area:dev-tools area:webserver Webserver related Issues labels Feb 22, 2020
turbaszek
turbaszek commented Feb 22, 2020
View reviewed changes
@turbaszek turbaszek force-pushed the AIRFLOW-6864-pylint-airflow-jobs branch from 84bf211 to 6230baf Compare February 22, 2020 12:58
@turbaszek turbaszek closed this Feb 22, 2020
@turbaszek turbaszek reopened this Feb 22, 2020
@turbaszek turbaszek force-pushed the AIRFLOW-6864-pylint-airflow-jobs branch from 6230baf to c0582aa Compare February 22, 2020 13:58
@potiuk potiuk mentioned this pull request Feb 22, 2020
Closed
6 tasks
ashb
ashb reviewed Feb 22, 2020
View reviewed changes
ashb
ashb reviewed Feb 22, 2020
View reviewed changes
ashb
ashb reviewed Feb 22, 2020
View reviewed changes
ashb
ashb reviewed Feb 22, 2020
View reviewed changes
ashb
ashb reviewed Feb 22, 2020
View reviewed changes
@mik-laj
Copy link
Member

mik-laj commented Feb 23, 2020

We currently have "airlfow/utils/state.py" which contains constants, so we can also create a new file that contains other enums/types/constants.

mik-laj
mik-laj reviewed Feb 23, 2020
View reviewed changes
airflow/jobs/backfill_job.py Outdated
Copy link
Member

@mik-laj mik-laj Feb 23, 2020

Choose a reason for hiding this comment

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

Do we need these constants? Maybe it would be better to use the DagRunType directly?

Copy link
Member Author

@turbaszek turbaszek Feb 23, 2020

Choose a reason for hiding this comment

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

I am happy to do this in other PR.

Copy link
Member Author

@turbaszek turbaszek Feb 23, 2020

Choose a reason for hiding this comment

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

I will leave it for other PR. It's not strictly related to this PR.

mik-laj
mik-laj reviewed Feb 23, 2020
View reviewed changes
@ashb
Copy link
Member

ashb commented Feb 23, 2020

Good spot Kamil!

mik-laj
mik-laj reviewed Feb 23, 2020
View reviewed changes
@turbaszek turbaszek force-pushed the AIRFLOW-6864-pylint-airflow-jobs branch from 9360697 to 9b4c356 Compare February 24, 2020 15:02
ashb
ashb reviewed Feb 24, 2020
View reviewed changes
@turbaszek turbaszek changed the title [AIRFLOW-6864][depends on 6863] Make airflow/jobs pylint compatible [AIRFLOW-6864] Make airflow/jobs pylint compatible Feb 24, 2020
@turbaszek turbaszek requested review from ashb, kaxil and mik-laj February 24, 2020 20:32
@kaxil
Copy link
Member

kaxil commented Feb 24, 2020

There are merge conflicts

@turbaszek turbaszek force-pushed the AIRFLOW-6864-pylint-airflow-jobs branch 2 times, most recently from ca317e3 to 0027102 Compare February 24, 2020 23:05
@turbaszek
Copy link
Member Author

turbaszek commented Feb 25, 2020

@kaxil @potiuk @mik-laj @ashb are we good to go?

@ashb ashb mentioned this pull request Feb 25, 2020
Merged
6 tasks
@ashb
Copy link
Member

ashb commented Feb 25, 2020

@nuclearpinguin See https://github.com/apache/airflow/pull/7527/files#r383812108 -- this is my only outstanding query about this PR.

@turbaszek
Copy link
Member Author

turbaszek commented Feb 25, 2020

@nuclearpinguin See https://github.com/apache/airflow/pull/7527/files#r383812108 -- this is my only outstanding query about this PR.

This PR runs pylint over scheduler_job.py the one you mention does not. I suppose once I rebase I will have to fix it in more places. @ashb

@turbaszek
[AIRFLOW-6864] Make airfow/jobs pylint compatible
19ae57d 
fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible

fixup! fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible

fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible

fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible

fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible

fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible

fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible

fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible

fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible
@potiuk potiuk mentioned this pull request Feb 25, 2020
Closed
6 tasks
@turbaszek turbaszek force-pushed the AIRFLOW-6864-pylint-airflow-jobs branch from 0027102 to 19ae57d Compare February 25, 2020 15:29
@turbaszek
Copy link
Member Author

turbaszek commented Feb 25, 2020 •
edited
Loading 

************* Module airflow.jobs.scheduler_job
airflow/jobs/scheduler_job.py:1031:16: E1101: Method 'state' has no 'is_' member (no-member)

@ashb

I assume that the difference is that here DM.dag_id.is_(None) we are referencing dag_id attribute of sqla model. And here models.DagRun.state.is_(None) reference a method of DagRun:

    @declared_attr
    def state(self):
        return synonym('_state',
                       descriptor=property(self.get_state, self.set_state))

@ashb
Copy link
Member

ashb commented Feb 25, 2020

Ah, because we have ignored that file (or mostly ignored it) it's not yet showing up in Pylint. SQLA DelcarativeModels does funny things with the classes/instances that pylint can't detect.

I guess it's unavoidable, and we will have to ignore those few lines.

I think pylint-dev/pylint#3334 is the same issue we have (open, no feedback).

@turbaszek turbaszek merged commit 3111406 into apache:master Feb 25, 2020
galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020
@turbaszek @galuszkak
[AIRFLOW-6864] Make airfow/jobs pylint compatible (apache#7484)
d6d0244 
fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible

fixup! fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible

fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible

fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible

fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible

fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible

fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible

fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible

fixup! [AIRFLOW-6864] Make airfow/jobs pylint compatible
@turbaszek turbaszek deleted the AIRFLOW-6864-pylint-airflow-jobs branch March 14, 2020 23:33
@kaxil kaxil mentioned this pull request May 21, 2020
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashb ashb ashb approved these changes

@kaxil kaxil kaxil approved these changes

@potiuk potiuk Awaiting requested review from potiuk

@mik-laj mik-laj Awaiting requested review from mik-laj

Assignees

No one assigned

Labels

area:CLI area:dev-tools area:Scheduler including HA (high availability) scheduler area:webserver Webserver related Issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@turbaszek @codecov-io @mik-laj @ashb @kaxil @potiuk