Skip to content

[AIRFLOW-5480] Fix flaky impersonation test#6098

Merged
potiuk merged 1 commit intoapache:masterfrom
PolideaInternal:fix-flaky-impersonation
Sep 13, 2019
Merged

[AIRFLOW-5480] Fix flaky impersonation test#6098
potiuk merged 1 commit intoapache:masterfrom
PolideaInternal:fix-flaky-impersonation

Conversation

@mik-laj
Copy link
Member

@mik-laj mik-laj commented Sep 13, 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-5480
    • 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:

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

@feluelle
Copy link
Member

@mik-laj do you know why this is "flaky"? How does this change fix it?

@codecov-io
Copy link

Codecov Report

Merging #6098 into master will increase coverage by 70.54%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #6098       +/-   ##
==========================================
+ Coverage    9.55%   80.1%   +70.54%     
==========================================
  Files         606     606               
  Lines       34893   34893               
==========================================
+ Hits         3334   27951    +24617     
+ Misses      31559    6942    -24617
Impacted Files Coverage Δ
airflow/plugins_manager.py 86.91% <0%> (+0.93%) ⬆️
airflow/executors/dask_executor.py 2% <0%> (+2%) ⬆️
airflow/config_templates/airflow_local_settings.py 80% <0%> (+2.5%) ⬆️
airflow/kubernetes/pod_launcher.py 91.85% <0%> (+2.96%) ⬆️
airflow/exceptions.py 100% <0%> (+3.03%) ⬆️
airflow/executors/__init__.py 67.34% <0%> (+6.12%) ⬆️
airflow/utils/log/colored_log.py 93.18% <0%> (+13.63%) ⬆️
airflow/settings.py 88.4% <0%> (+15.21%) ⬆️
airflow/utils/decorators.py 90.9% <0%> (+15.9%) ⬆️
airflow/stats.py 69.23% <0%> (+16.92%) ⬆️
... and 497 more

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 6a66ece...7e1afd4. Read the comment docs.

@mik-laj
Copy link
Member Author

mik-laj commented Sep 13, 2019

@feluelle II'm not sure why it worked before, but I know why it works now. This test verifies that if we have PYTHONPATH set it will be correctly passed during interpersonation.
We set PYTHONPATH in this place: https://github.com/apache/airflow/blob/master/run-tests#L48
This allows you to search modules from a different location. Please note that airflow modules are available because it is installed on the system - pip install -e .

I would like to add quote from this file:

# AIRFLOW-1893 - Originally, impersonation tests were incomplete missing the use case when
# DAGs access custom packages usually made available through the PYTHONPATH environment
# variable. This file includes a DAG that imports a custom package made available and if
# run via the previous implementation of impersonation, will fail by not being able to
# import the custom package.
# This DAG is used to test that impersonation propagates the PYTHONPATH environment
# variable correctly.

@mik-laj mik-laj requested a review from ashb September 13, 2019 17:17
@mik-laj mik-laj changed the title [AIRFLOW-YYY] Fix flaky impersonation test [AIRFLOW-5480] Fix flaky impersonation test Sep 13, 2019
@mik-laj mik-laj requested review from Fokko, kaxil and potiuk September 13, 2019 17:18
@feluelle
Copy link
Member

Got it, thanks. 👍 Nice one! :)

@mik-laj
Copy link
Member Author

mik-laj commented Sep 13, 2019

It blocks my work a bit. I hope it will be accepted soon.

One more thing that I noticed was that it could be related to what Docker image I use. A colleague did not have this problem locally if he did not re-build the Docker image.

@potiuk potiuk merged commit 4459592 into apache:master Sep 13, 2019
alrolorojas pushed a commit to github/incubator-airflow that referenced this pull request Sep 14, 2019
kaxil pushed a commit that referenced this pull request Sep 17, 2019
potiuk pushed a commit that referenced this pull request Sep 17, 2019
potiuk pushed a commit that referenced this pull request Sep 18, 2019
ashb pushed a commit to astronomer/airflow that referenced this pull request Sep 25, 2019
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.

4 participants