[AIRFLOW-1985] Impersonation fixes for using run_as_user#2991
Conversation
|
@aoen @yrqls21 PTAL |
|
Please rebase against master to trigger a new build. The CI should be fixed. |
edgarRd
force-pushed
the
erod-impersonation-fix
branch
from
3b8a4cf to
55994f7
Compare
|
@Fokko Done. Thanks! |
Codecov Report
@@ Coverage Diff @@
## master #2991 +/- ##
==========================================
+ Coverage 72.96% 73.14% +0.18%
==========================================
Files 176 177 +1
Lines 12375 12394 +19
==========================================
+ Hits 9029 9066 +37
+ Misses 3346 3328 -18
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Nit format like your other TODOs:
TODO(edgarRd): AIRFLOW-1444:
There was a problem hiding this comment.
Nit: /Adding cfg_path as a way to/cfg_path is needed to
There was a problem hiding this comment.
It's a bit unclear what this is referring to, and the line of code sited could change. I think it's fine to just remove this comment
There was a problem hiding this comment.
I believe so, otherwise cfg_path used further down in call executor.queue_task_instance(...) would not be defined if the flow does not go through the following if statement.
There was a problem hiding this comment.
Let's provide more context (no pun intended) around what kind of context this is referring to (i.e. what should run in the same context as what)
There was a problem hiding this comment.
Example would be helpful here, e.g. PythonOperator where airflow is imported
There was a problem hiding this comment.
What cleans this up? If it's not cleaned up, seems like a good place for a context manager.
There was a problem hiding this comment.
I tried using a context manager, but this file is actually removed by the receiving new process so triggered by the task runner: https://github.com/apache/incubator-airflow/blob/master/airflow/bin/cli.py#L395 so, the context does not work for this case.
This is usually a bad pattern because the run command in the cli (a new process) is removing a file it does not maintain, making it harder for the caller (the parent process) to clean up the files the it actually creates / maintains. Fixing that design is out of scope of this PR.
There was a problem hiding this comment.
@edgarRd can you at least create a JIRA for that issue?
There was a problem hiding this comment.
edgarRd
force-pushed
the
erod-impersonation-fix
branch
from
55994f7 to
e96dfd2
Compare
* [AIRFLOW-1985] Changes to propagate config in subdags * [AIRFLOW-1985] Make sure tasks running have the config Making sure all tasks have the proper configuration by copying it into a temporary file. * [AIRFLOW-1985] Airflow home and python path to BashOperator BashOperators should have the context of the AIRFLOW_HOME and PYTHONPATH as other tasks have.
edgarRd
force-pushed
the
erod-impersonation-fix
branch
from
e96dfd2 to
38eb7d1
Compare
|
LGTM |
Changes to propagate config in subdags, and make sure tasks running have the config. Making sure all tasks have the proper configuration by copying it into a temporary file. BashOperators should have the context of the AIRFLOW_HOME and PYTHONPATH as other tasks have. Closes apache#2991 from edgarRd/erod-impersonation-fix
5 participants
JIRA
Description
[AIRFLOW-1985] Changes to propagate config in subdags
[AIRFLOW-1985] Make sure tasks running have the config
Making sure all tasks have the proper configuration by copying it into a
temporary file.
BashOperators should have the context of the AIRFLOW_HOME and PYTHONPATH
as other tasks have.
Make sure you have checked all steps below.
Tests
Impersonation unit tests included. Integration tests were also done in a staging environment.
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":
Passes
git diff upstream/master -u -- "*.py" | flake8 --diff