Skip to content
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.

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

Don't permanently add zip DAGs to sys.path #18384

Merged
merged 1 commit into from
Sep 20, 2021

Conversation

jedcunningham
Copy link
Member

Only leave the zip on sys.path while parsing that zip file, as we
shouldn't assume parsing only happens in separate processes. For
example, resetdb is used to run migrations, some of which parse
the DAGs.

Only leave the zip on `sys.path` while parsing that zip file, as we
shouldn't assume parsing only happens in separate processes. For
example, `resetdb` is used to run migrations, some of which parse
the DAGs.
@kaxil kaxil changed the title Don't permanently add zip DAGs to sys.path Don't permanently add zip DAGs to sys.path Sep 20, 2021
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Sep 20, 2021
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@kaxil kaxil merged commit a01c08b into apache:main Sep 20, 2021
@kaxil kaxil deleted the zipfile_dont_modify_syspath branch September 20, 2021 18:18
@potiuk
Copy link
Member

potiuk commented Sep 21, 2021

Do I get correctly - that was causing the side-effect of failing KPO tests in some cases?

@jedcunningham
Copy link
Member Author

PodGenerator, yeah. Some tests in test_pod_generator.py use sys.path[0] and expect it to have cwd:

path = sys.path[0] + '/tests/kubernetes/pod_generator_base_with_secrets.yaml'

However a couple migrations parse the DAGs and this was leaving the zip in spot 0 (in fact, it would be [zip, zip, cwd, ...]). You could force the issue every time using --with-db-init. Granted, the tests probably shouldn't rely on sys.path[0] being unmodified either, but this PR at least fixed the real issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants