You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/01/06 02:31:28 UTC
[GitHub] [airflow] mik-laj opened a new pull request #13501: Warn about precedence of env var when getting variables
mik-laj opened a new pull request #13501:
URL: https://github.com/apache/airflow/pull/13501
This behavior may not be obvious to everyone, so I display a message when this happens. Hope this will improve DX a little.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #13501: Warn about precedence of env var when getting variables
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13501:
URL: https://github.com/apache/airflow/pull/13501#discussion_r552841054
##########
File path: airflow/models/variable.py
##########
@@ -143,6 +147,14 @@ def set(cls, key: str, value: Any, serialize_json: bool = False, session: Sessio
:param serialize_json: Serialize the value to a JSON string
:param session: SQL Alchemy Sessions
"""
+ env_var_name = "AIRFLOW_VAR_" + key.upper()
Review comment:
Yep. I realize this is more complex. So it should be part of separate PR. And rather than querying the backend, I'd rather add somewhere an info like "origin" of a variable, so when it is read, we'd now it comes from secret backend rather than db or something like that - so that we do not increase number of queries to the DB. This should pretty simple to implement and fairly robust (I believe in all cases the variable should be read before it is written to - we might want to check all places, but it should be the case already).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj merged pull request #13501: Warn about precedence of env var when getting variables
Posted by GitBox <gi...@apache.org>.
mik-laj merged pull request #13501:
URL: https://github.com/apache/airflow/pull/13501
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13501: Warn about precedence of env var when getting variables
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13501:
URL: https://github.com/apache/airflow/pull/13501#issuecomment-757149624
[The Workflow run](https://github.com/apache/airflow/actions/runs/474019545) is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #13501: Warn about precedence of env var when getting variables
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13501:
URL: https://github.com/apache/airflow/pull/13501#discussion_r552517719
##########
File path: airflow/models/variable.py
##########
@@ -143,6 +147,14 @@ def set(cls, key: str, value: Any, serialize_json: bool = False, session: Sessio
:param serialize_json: Serialize the value to a JSON string
:param session: SQL Alchemy Sessions
"""
+ env_var_name = "AIRFLOW_VAR_" + key.upper()
Review comment:
Should we also do this in case we have secret backend?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #13501: Warn about precedence of env var when getting variables
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13501:
URL: https://github.com/apache/airflow/pull/13501#discussion_r552532697
##########
File path: airflow/models/variable.py
##########
@@ -143,6 +147,14 @@ def set(cls, key: str, value: Any, serialize_json: bool = False, session: Sessio
:param serialize_json: Serialize the value to a JSON string
:param session: SQL Alchemy Sessions
"""
+ env_var_name = "AIRFLOW_VAR_" + key.upper()
Review comment:
This is a bit more complicated case as this will require sending an additional request to the secret backend. The new request can be problematic in some environments as access to data in the secret is precisely monitored and audited. So even queries to check if there is an item in the backend will mean that the security department may be concerned. Especially since we don't have an API that allows us to check the existence of a key without downloading a secret.
In this change, I wanted to improve the experience of novice developers who are just starting to use Airflow and may not fully understand all the limitations. Such users suspect that they don't use the secret backend, so I don't think the lack of backend secret support is a big problem.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13501: Warn about precedence of env var when getting variables
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13501:
URL: https://github.com/apache/airflow/pull/13501#issuecomment-757149624
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13501: Warn about precedence of env var when getting variables
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13501:
URL: https://github.com/apache/airflow/pull/13501#issuecomment-757399382
[The Workflow run](https://github.com/apache/airflow/actions/runs/474915213) 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*.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #13501: Warn about precedence of env var when getting variables
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13501:
URL: https://github.com/apache/airflow/pull/13501#discussion_r553052730
##########
File path: airflow/models/variable.py
##########
@@ -143,6 +147,14 @@ def set(cls, key: str, value: Any, serialize_json: bool = False, session: Sessio
:param serialize_json: Serialize the value to a JSON string
:param session: SQL Alchemy Sessions
"""
+ env_var_name = "AIRFLOW_VAR_" + key.upper()
+ if env_var_name in os.environ:
+ log.warning(
+ "You have the environment variable %s defined, which takes precedence over reading "
+ "from the database. The value will be saved, but to read it you have to delete "
+ "the environment variable.",
+ env_var_name,
+ )
Review comment:
Wait, this means we get a warning when we knowingly use Env Variables to get Variables -- that should not be the case. I am happy for it to be at debug level but not warn
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #13501: Warn about precedence of env var when getting variables
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13501:
URL: https://github.com/apache/airflow/pull/13501#issuecomment-755273400
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 master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org