-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
Store config description in Airflow configuration object #32669
Store config description in Airflow configuration object #32669
Conversation
This is the most important part extracted from #32604 (I will shortly rebase #32604) on top of this one. This one DOES NOT contain any provider-related changes. Those will come as finall installment of the provider config contributed changes and is a crucial refactor to be able to add provider-contributed configurations. I rewrote a little what I have done initially after I run more thorough testing - I came back (and got friends with) using ConfigParser as "default" configuration storage (because of interpolation). I also think that having The gist of this change remains the same as described in the original PR - make I also included some comments from the review in #32604 by @jedcunningham @ferruzzi and @o-nikolas (some of them still will need to be addressed separately and I resolved all those that are applied). |
8b2a8fc
to
49a47db
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did one pass at the PR and have few nits. Nothing major
df0d88a
to
e12ea1f
Compare
Applied first round of comments. |
e12ea1f
to
4699c1c
Compare
b24d5d4
to
d71812a
Compare
Applied second round of comments from @jedcunningham. For those who did not try it yet, the nice part of this change (after @jedcunningham comment) is that we can start promoting the "good" pattern of only changing the configuration variables that users actually want to change. Part of the change is to make the ![]() But this command is now a bit of "swiss-army-knife" of configuration: ![]() Especially the We've been promoting this kind of behaviour as a good pattern (only set those values that you want to be changed from defaults) and with such generated config file it is, I think, perfect way of doing it, because the user can see all the descriptions and examples right in the place the values can be edited, but the values are actually commented out, so it requires deliberate effort to change them - and users will easily see which of the values they actually changed. ![]() |
6e930dc
to
b7e990e
Compare
b7e990e
to
0fbef49
Compare
Green now. Looking for some more reviews and finally merging to continue with config move to providers and executors move :) |
132bf9a
to
698ca1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked at most of the PR, except mainly the changes in configuration.py
30c88e4
to
9f00983
Compare
We would like to use the config.yml approach as our main source of truth for airflow configuration. So far it has been split amongst multiple files: * confg.yml -> descroption * default_airflow.cfg -> JINJA template to keep cofiguration, examples and description (used to generate airflow.cfg when airflow starts) * default_test.cfg -> storing test-only configuration used in some test cases - used to generate unittest.cfg * scripts/in_container/airflow_ci.cfg where dCI-specific configuration overwrote the unittest.cfg This change consolidates it all into unified appraoch, where all configuration information is retrieved from .yml files stored in config_templates. No more additional template files processed by JINJA, no more special CI versions of it, no more unittestdb.cfg file where such configuration would be generated, no more unittestdb to be used separately for tests. * The default_*.cfg files were not real configuration files, becuase they were really JINJA templates and it got peoeple confused when copying the files. This change leaves the file empty with the comment that instructs the user how they can get the default configuration. * The default_airflow.cfg is gone and instead, we have a way to show defaults via `airflow config list --defaults` command * Unittest config is generated on-the-flight using defaults stored in confg_templates. constituing a single place where they need to be changed for the tests to use them * internally, description of the configuration is stored in AirflowConfigurationParser and used by the parser to generate the default configuration when needed. * we got rid of `{{{{` in templated config defaults by marking the templates with ``is_template`` and getting rid of processing those entries with regular formatting when generating the default values. This only concerns defaults from config.yml. Reading those configuration entries from file is unaffected. This change aims to be 100% backwards compatible with the previous implementation when it comes to functionality, even if internals changed. It also does not add provider-specific changes that are coming separately. The only changes visible to the user are: * generated airflow.cfg is slightly more readable and displays names of variables that can be used to override each configuration (which is very useful for copy&pasting) * user are advised, instead of copying the default_airflow.cfg to use `airflow config list --defaults` to generate production config. This configuration has all the entries commented out, so that they can selectively uncomment and change the values they want. This is now promoted as "best practice" in the documentation.
9f00983
to
9191b33
Compare
Random infra-related stability problems. Merging finally :) |
We would like to use the config.yml approach as our main source of truth for airflow configuration. So far it has been split amongst multiple files: * confg.yml -> descroption * default_airflow.cfg -> JINJA template to keep cofiguration, examples and description (used to generate airflow.cfg when airflow starts) * default_test.cfg -> storing test-only configuration used in some test cases - used to generate unittest.cfg * scripts/in_container/airflow_ci.cfg where dCI-specific configuration overwrote the unittest.cfg This change consolidates it all into unified appraoch, where all configuration information is retrieved from .yml files stored in config_templates. No more additional template files processed by JINJA, no more special CI versions of it, no more unittestdb.cfg file where such configuration would be generated, no more unittestdb to be used separately for tests. * The default_*.cfg files were not real configuration files, becuase they were really JINJA templates and it got peoeple confused when copying the files. This change leaves the file empty with the comment that instructs the user how they can get the default configuration. * The default_airflow.cfg is gone and instead, we have a way to show defaults via `airflow config list --defaults` command * Unittest config is generated on-the-flight using defaults stored in confg_templates. constituing a single place where they need to be changed for the tests to use them * internally, description of the configuration is stored in AirflowConfigurationParser and used by the parser to generate the default configuration when needed. * we got rid of `{{{{` in templated config defaults by marking the templates with ``is_template`` and getting rid of processing those entries with regular formatting when generating the default values. This only concerns defaults from config.yml. Reading those configuration entries from file is unaffected. This change aims to be 100% backwards compatible with the previous implementation when it comes to functionality, even if internals changed. It also does not add provider-specific changes that are coming separately. The only changes visible to the user are: * generated airflow.cfg is slightly more readable and displays names of variables that can be used to override each configuration (which is very useful for copy&pasting) * user are advised, instead of copying the default_airflow.cfg to use `airflow config list --defaults` to generate production config. This configuration has all the entries commented out, so that they can selectively uncomment and change the values they want. This is now promoted as "best practice" in the documentation.
We would like to use the config.yml approach as our main source of truth
for airflow configuration. So far it has been split amongst multiple
files:
and description (used to generate airflow.cfg when airflow starts)
test cases - used to generate unittest.cfg
overwrote the unittest.cfg
This change consolidates it all into unified appraoch, where all
configuration information is retrieved from .yml files stored in
config_templates. No more additional template files processed by
JINJA, no more special CI versions of it, no more unittestdb.cfg file
where such configuration would be generated, no more unittestdb to
be used separately for tests.
they were really JINJA templates and it got peoeple confused when
copying the files. This change leaves the file empty with the
comment that instructs the user how they can get the default
configuration.
show defaults via
airflow config list --defaults
commandin confg_templates. constituing a single place where they need
to be changed for the tests to use them
AirflowConfigurationParser and used by the parser to generate
the default configuration when needed.
{{{{
in templated config defaults by markingthe templates with
is_template
and getting rid of processingthose entries with regular formatting when generating the default
values. This only concerns defaults from config.yml. Reading
those configuration entries from file is unaffected.
This change aims to be 100% backwards compatible with the previous
implementation when it comes to functionality, even if internals
changed. It also does not add provider-specific changes that are
coming separately.
The only changes visible to the user are:
generated airflow.cfg is slightly more
readable and displays names of variables that can be used to override
each configuration (which is very useful for copy&pasting)
user are advised, instead of copying the default_airflow.cfg to use
airflow config list --defaults
to generate production config. Thisconfiguration has all the entries commented out, so that they can
selectively uncomment and change the values they want. This is now
promoted as "best practice" in the documentation.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.