-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
Add loggingconfig rule #11132
Add loggingconfig rule #11132
Conversation
"The following configurations have been to moved from [core] to the new [logging] section. \n" | ||
"- base_log_folder \n" | ||
"- remote_logging \n" | ||
"- remote_log_conn_id \n" | ||
"- remote_base_log_folder \n" | ||
"- encrypt_s3_logs \n" | ||
"- logging_level \n" | ||
"- fab_logging_level \n" | ||
"- logging_config_class \n" | ||
"- colored_console_log \n" | ||
"- colored_log_format \n" | ||
"- colored_formatter_class \n" | ||
"- log_format \n" | ||
"- simple_log_format \n" | ||
"- task_log_prefix_template \n" | ||
"- log_filename_template \n" | ||
"- log_processor_filename_template \n" | ||
"- dag_processor_manager_log_location \n" | ||
"- task_log_reader \n" |
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.
I would suggest checking if any of this key is used by users (not empty or different than default - this can be tricky) and then print a message for each incompatibility. See #11056 for reference. In this way, the message will be more precise. What do you think?
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.
Ah I was not thinking of this cause this change is backwards compatible and not something that can break if not moved but you are right, showing only the incompatibility really helps while taking actions, I can do a comparison and return only the actively used keys but what of the default/ empty keys is it right that they still remain in the old section ?
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.
what of the default/ empty keys is it right that they still remain in the old section ?
I think it will be ok to hardcode them in rule using default values from airflow.cfg
of 1.10.x. WDYT @kaxil ?
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.
Yea default values can be hardcoded.
And also agree that showing a precise message for the used config will be better
|
||
mismatches = [] | ||
for logging_config, default in logging_configs: | ||
if not conf.has_option("logging", logging_config) and conf.has_option( |
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.
Will this work if the logging
section is not present? Have you tested it?
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.
Yep has_option
returns false
when the section is not present
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.
Look good to me 🚀
Logging configuration has been moved to new section
---------------------------------------------------
The logging configurations have been moved from [core] to the new [logging] section.
Problems:
1. task_log_prefix_template has been moved from [core] to a the new [logging] section.
bd4214b
to
195f00d
Compare
Please rebase your PR, we added few changes that should fix the CI issues. Please do rebase and try to avoid merge commits, more information: |
9d6e7f9
to
a199d65
Compare
@Sangarshanan I think that the |
8424054
to
37a63fa
Compare
The CI and PROD Docker Images for the build are prepared in a separate "Build Image" workflow, You can checks the status of those images in The workflow run |
@Sangarshanan it seems that one test is failing:
Personally I would remove the |
@turbaszek This is an interesting problem that also occurs in other cases. I think that it is also worth solving separately. If you have a configuration file from Airflow 2 and you run it on Airflow 1.10, then you can't run: ʻairflow config` |
I agree, but as you said, this should be done in separate PR |
The CI and PROD Docker Images for the build are prepared in a separate "Build Image" workflow, You can checks the status of those images in The workflow run |
All upgrade tests passed:
|
(cherry picked from commit 80dc387)
(cherry picked from commit 80dc387)
Closes: #11046
Adds LoggingConfigurationRule rule to upgrade/rules as per:
https://github.com/apache/airflow/blob/master/UPDATING.md#logging-configuration-has-been-moved-to-new-section
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.