Skip to content
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

[AIRFLOW-6271] Printing log files read during load_test_config #6842

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Dec 18, 2019

Airflow should show where the configuration is read from. Especially for new
user it might be confusing as it is not logged anywhere and there are several
places the configuration might be read from. 

This is printed for the main config file, but not when load_test_config is used.

Make sure you have checked all steps below.

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

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":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Airflow should show where the configuration is read from. Especially for new
user it might be confusing as it is not logged anywhere and there are several
places the configuration might be read from. 

This is printed for the main config file, but not when load_test_config is used.
@potiuk potiuk requested a review from mik-laj December 18, 2019 19:03
@codecov-io
Copy link

codecov-io commented Dec 18, 2019

Codecov Report

Merging #6842 into master will decrease coverage by 0.28%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6842      +/-   ##
==========================================
- Coverage   84.49%   84.21%   -0.29%     
==========================================
  Files         680      680              
  Lines       38392    38395       +3     
==========================================
- Hits        32441    32335     -106     
- Misses       5951     6060     +109
Impacted Files Coverage Δ
airflow/configuration.py 93.3% <100%> (+0.07%) ⬆️
airflow/operators/mysql_operator.py 100% <0%> (ø) ⬆️
airflow/operators/mysql_to_hive.py 100% <0%> (ø) ⬆️
airflow/kubernetes/volume_mount.py 44.44% <0%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0%> (-47.06%) ⬇️
airflow/kubernetes/pod_launcher.py 45.25% <0%> (-46.72%) ⬇️
airflow/kubernetes/refresh_config.py 50.98% <0%> (-23.53%) ⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py 78.75% <0%> (-20%) ⬇️
airflow/utils/sqlalchemy.py 96.61% <0%> (ø) ⬆️
airflow/hooks/hive_hooks.py 77.6% <0%> (ø) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 136dead...f005737. Read the comment docs.

@potiuk potiuk requested review from ashb and kaxil December 19, 2019 08:39
@@ -430,10 +430,13 @@ def load_test_config(self):
Note: this is not reversible.
"""
# override any custom settings with defaults
log.info("Overriding settings with defaults from %s", DEFAULT_CONFIG_FILE_PATH)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these should be debug -- it's a bit too noisy and not useful to see this all the time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nm, this is load_test_config only :)

@potiuk potiuk merged commit 81de282 into apache:master Dec 19, 2019
kaxil pushed a commit that referenced this pull request Dec 19, 2019
Airflow should show where the configuration is read from. Especially for new
user it might be confusing as it is not logged anywhere and there are several
places the configuration might be read from. 

This is printed for the main config file, but not when load_test_config is used.

(cherry picked from commit 81de282)
kaxil pushed a commit that referenced this pull request Dec 20, 2019
Airflow should show where the configuration is read from. Especially for new
user it might be confusing as it is not logged anywhere and there are several
places the configuration might be read from. 

This is printed for the main config file, but not when load_test_config is used.

(cherry picked from commit 81de282)
KKcorps pushed a commit to KKcorps/airflow that referenced this pull request Dec 21, 2019
…e#6842)

Airflow should show where the configuration is read from. Especially for new
user it might be confusing as it is not logged anywhere and there are several
places the configuration might be read from. 

This is printed for the main config file, but not when load_test_config is used.
galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020
…e#6842)

Airflow should show where the configuration is read from. Especially for new
user it might be confusing as it is not logged anywhere and there are several
places the configuration might be read from. 

This is printed for the main config file, but not when load_test_config is used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants