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 2022/02/12 18:31:06 UTC
[GitHub] [airflow] xyu opened a new pull request #21539: Filter out default configs when overrides exist.
xyu opened a new pull request #21539:
URL: https://github.com/apache/airflow/pull/21539
When sending configs to Airflow workers we materialize a temp config file. In #18772 a feature was added so that `_cmd` generated secrets are not written to the files in some cases instead favoring maintaining the raw `_cmd` settings. Unfortunately during materializing of the configs via `as_dict()` Airflow defaults are generated and materialized as well including defaults for the non `_cmd` versions of some settings. And due to Airflow setting precedence stating bare versions of settings winning over `_cmd` versions it results in `_cmd` settings being discarded:
https://airflow.apache.org/docs/apache-airflow/stable/howto/set-config.html
This change checks `_cmd`, env, and secrets when materializing configs via `as_dict()` so that if the bare versions of the values is exactly the same as Airflow defaults and we have "hidden" / special versions of these configs that are trying to be set we remove the bare versions so that the correct version can be used.
closes: #20092
related: #18772 #4050
<!--
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/main/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/main/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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #21539: Filter out default configs when overrides exist.
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #21539:
URL: https://github.com/apache/airflow/pull/21539#issuecomment-1066233262
After looking agian - I think what is also missing here is a bit "user" description in the https://airflow.apache.org/docs/apache-airflow/stable/howto/set-config.html
I think we might have a lot of questions from users about the behaviour of config values as this is a "user" thing. I doubt our users will look in detail in "docstring" - they will mostly look at the documentation to understand how configuration precedence works, and especially - there should be a documentation explaining that it has changed in 2.3.0.
Would it be possible tha tyou add such "user" facing documentation @xyu ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #21539: Filter out default configs when overrides exist.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21539:
URL: https://github.com/apache/airflow/pull/21539#issuecomment-1053739629
And we need another committer chiming in.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #21539: Filter out default configs when overrides exist.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21539:
URL: https://github.com/apache/airflow/pull/21539#issuecomment-1068297336
Random environmental problems. Merging.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xyu commented on pull request #21539: Filter out default configs when overrides exist.
Posted by GitBox <gi...@apache.org>.
xyu commented on pull request #21539:
URL: https://github.com/apache/airflow/pull/21539#issuecomment-1067245016
> there should be a documentation explaining that it has changed in 2.3.0.
I'm not quite sure what you mean @potiuk, as you pointed out in the original issue (https://github.com/apache/airflow/issues/20092#issuecomment-997431778) the precedence for configuration options is bare setting over `_cmd` or secret versions. This PR filters out the bare configs if they are set to Airflow’s built in defaults which then allows the user set `_cmd` overrides to work again. So this is really fixing a bug not adding or changing config behavior. (I mean I guess it is changing the config behavior but it's changing the behavior to an unbroken as documented state.)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #21539: Filter out default configs when overrides exist.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21539:
URL: https://github.com/apache/airflow/pull/21539#issuecomment-1067278507
> Oh I see as in for a changelog? Or do you mean add another "note" box to [this page](https://airflow.apache.org/docs/apache-airflow/stable/howto/set-config.html)?
I think would be nice to mentiont it on this page that it was working differently before 2.3.0
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xyu commented on pull request #21539: Filter out default configs when overrides exist.
Posted by GitBox <gi...@apache.org>.
xyu commented on pull request #21539:
URL: https://github.com/apache/airflow/pull/21539#issuecomment-1067274225
Oh I see as in for a changelog? Or do you mean add another "note" box to [this page](https://airflow.apache.org/docs/apache-airflow/stable/howto/set-config.html)?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #21539: Filter out default configs when overrides exist.
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #21539:
URL: https://github.com/apache/airflow/pull/21539#issuecomment-1068297614
Awesome work, congrats on your first merged pull request!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #21539: Filter out default configs when overrides exist.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21539:
URL: https://github.com/apache/airflow/pull/21539#issuecomment-1066233262
After looking agian - I think what is also missing here is a bit "user" description in the https://airflow.apache.org/docs/apache-airflow/stable/howto/set-config.html
I think we might have a lot of questions from users about the behaviour of config values as this is a "user" thing. I doubt our users will look in detail in "docstring" - they will mostly look at the documentation to understand how configuration precedence works, and especially - there should be a documentation explaining that it has changed in 2.3.0.
Would it be possible tha tyou add such "user" facing documentation @xyu
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #21539: Filter out default configs when overrides exist.
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21539:
URL: https://github.com/apache/airflow/pull/21539#issuecomment-1067264979
> I'm not quite sure what you mean @potiuk
I mean that we should mention it was NOT working as expected before.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xyu commented on pull request #21539: Filter out default configs when overrides exist.
Posted by GitBox <gi...@apache.org>.
xyu commented on pull request #21539:
URL: https://github.com/apache/airflow/pull/21539#issuecomment-1058789950
@potiuk / @uranusjr can you take a look at the docstrings I've added to see if they make sense? Also if one of you can trigger the full test run after the latest merge that would be awesome and let me know if there's anything else I should patch up with this PR to get it ready.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xyu commented on pull request #21539: Filter out default configs when overrides exist.
Posted by GitBox <gi...@apache.org>.
xyu commented on pull request #21539:
URL: https://github.com/apache/airflow/pull/21539#issuecomment-1045396949
This should be a more general solution for #21604 as well
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #21539: Filter out default configs when overrides exist.
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #21539:
URL: https://github.com/apache/airflow/pull/21539
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xyu commented on pull request #21539: Filter out default configs when overrides exist.
Posted by GitBox <gi...@apache.org>.
xyu commented on pull request #21539:
URL: https://github.com/apache/airflow/pull/21539#issuecomment-1046356234
Merged trunk in because #21664 was needed to fix broken tests.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #21539: Filter out default configs when overrides exist.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #21539:
URL: https://github.com/apache/airflow/pull/21539#issuecomment-1052465080
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.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xyu edited a comment on pull request #21539: Filter out default configs when overrides exist.
Posted by GitBox <gi...@apache.org>.
xyu edited a comment on pull request #21539:
URL: https://github.com/apache/airflow/pull/21539#issuecomment-1046356234
Merged trunk in because #21664 from @potiuk was needed to fix broken tests.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #21539: Filter out default configs when overrides exist.
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #21539:
URL: https://github.com/apache/airflow/pull/21539#issuecomment-1054788420
It’d be nice if a docstring could be added to explain what `filter_by_source` does and why it is needed.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xyu edited a comment on pull request #21539: Filter out default configs when overrides exist.
Posted by GitBox <gi...@apache.org>.
xyu edited a comment on pull request #21539:
URL: https://github.com/apache/airflow/pull/21539#issuecomment-1067245016
> there should be a documentation explaining that it has changed in 2.3.0.
I'm not quite sure what you mean @potiuk, as you pointed out in the original issue (https://github.com/apache/airflow/issues/20092#issuecomment-997431778) the precedence for configuration options is bare setting over `_cmd` or secret versions. This PR filters out the bare configs if they are set to Airflow’s built in defaults which then allows the user set `_cmd` overrides to work again. So this is really fixing a bug not adding or changing config behavior. (I mean I guess it is changing the config behavior but it's changing the behavior to an unbroken as documented state.)
EDIT: Just to clarify with Airflow >= 2.2.1 when `as_dict()` was called if a "sensitive_config_values" had the bare value (e.g. `sql_alchemy_conn`) unset but a cmd or secret version of that set (e.g. `sql_alchemy_conn_cmd`) the dict produced by the `as_dict()` function would contain the Airflow default for the bare setting set. This in turn causes anything downstream, e.g. Airflow workers to ignore the cmd or secret version of that config and always use Airflow defaults.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org