Skip to content

Conversation

@ashb
Copy link
Member

@ashb ashb commented Nov 17, 2020

By doing it this way we can add new features to upgrade_check, such as
the adding a flag to make changes automatically, without having to
release a new version of Airflow.

I will next work on packaging up the existing checks in to a packageable format.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
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.

@ashb ashb added this to the Airflow 1.10.13 milestone Nov 17, 2020
@ashb ashb requested review from kaxil, mik-laj and turbaszek November 17, 2020 12:37
@github-actions
Copy link

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

By doing it this way we can add new features to upgrade_check, such as
the adding a flag to make changes automatically, without having to
release a new version of Airflow.
@ashb ashb force-pushed the upgrade-check-from-external-dist branch from f6ff9fa to d9f4b72 Compare November 17, 2020 13:23
@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@ashb
Copy link
Member Author

ashb commented Nov 17, 2020

@potiuk Test failed with "Local airflow CI image not available" which is not an error I've seen before.

Possible we've got a bug in CI harness on v1-10-stable PRs now. Do you have time to look, or if not any ideas where I should start looking at?

@potiuk
Copy link
Member

potiuk commented Nov 17, 2020

looking!

@potiuk
Copy link
Member

potiuk commented Nov 17, 2020

Nope that 's not it. This message is perfectly fine there. It's a real flake8 error but I must have lost a fix not showing problems on pre-commits failing. I will make a PR shortly but in the meantime just run flake8 locally.

@potiuk
Copy link
Member

potiuk commented Nov 17, 2020

Nope. I have not lost the messages - they are shown all right.

I believe this is caused by https://github.com/apache/airflow/pull/12397/files#diff-2dfb149ecb4c1fe07a9c14f488a64326a8255a369d85fee621c4ecbd092a21acR2279 -> for some reason upgrade_check() is called when flake is executed likely and it causes system.exit()

@potiuk
Copy link
Member

potiuk commented Nov 17, 2020

I also made #12402 chage to hide those messages by default. They are not useful for the user.

@ashb
Copy link
Member Author

ashb commented Nov 17, 2020

Nope. I have not lost the messages - they are shown all right.

I believe this is caused by https://github.com/apache/airflow/pull/12397/files#diff-2dfb149ecb4c1fe07a9c14f488a64326a8255a369d85fee621c4ecbd092a21acR2279 -> for some reason upgrade_check() is called when flake is executed likely and it causes system.exit()

😱 Flake is running code? THat's horrifying. I'll take a look.

Ah no, it's that code, but not the exit.

airflow/upgrade/checker.py:68:5: F821 undefined name 'args'
airflow/upgrade/checker.py:69:5: F821 undefined name 'args'
airflow/upgrade/checker.py:69:15: F821 undefined name 'args'

That's the error. But why didn't pre-commit/flake8 show it.

@potiuk
Copy link
Member

potiuk commented Nov 17, 2020

or maybe simplu failing hard on sys.exit("STRING") !!!!

@potiuk
Copy link
Member

potiuk commented Nov 17, 2020

Hmm. will take a look and try to reproduce

@ashb
Copy link
Member Author

ashb commented Nov 17, 2020

I'm just rebuilding the image locally to elimiate that.

@ashb
Copy link
Member Author

ashb commented Nov 17, 2020

(It's at times like this I wish pre-commit would have an option to output more debugging!)

@ashb
Copy link
Member Author

ashb commented Nov 17, 2020

Hmmmm, now I've rebuilt it, pre-commit locally is showing me the error.... CURIOUS

@potiuk
Copy link
Member

potiuk commented Nov 17, 2020

Yep. Same for me. strange :(

@ashb ashb merged commit fb90c75 into apache:v1-10-stable Nov 18, 2020
@ashb ashb deleted the upgrade-check-from-external-dist branch November 18, 2020 13:23
kaxil pushed a commit that referenced this pull request Nov 18, 2020
By doing it this way we can add new features to upgrade_check, such as
the adding a flag to make changes automatically, without having to
release a new version of Airflow.

(cherry picked from commit fb90c75)
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
By doing it this way we can add new features to upgrade_check, such as
the adding a flag to make changes automatically, without having to
release a new version of Airflow.

(cherry picked from commit fb90c75)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants