-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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-3743] Unify different methods of working out AIRFLOW_HOME #4705
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -544,12 +544,31 @@ def parameterized_config(template): | |
|
||
conf.read(AIRFLOW_CONFIG) | ||
|
||
DEFAULT_WEBSERVER_CONFIG = _read_default_config_file('default_webserver_config.py') | ||
if conf.has_option('core', 'AIRFLOW_HOME'): | ||
ashb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
msg = ( | ||
'Specifying both AIRFLOW_HOME environment variable and airflow_home ' | ||
'in the config file is deprecated. Please use only the AIRFLOW_HOME ' | ||
'environment variable and remove the config file entry.' | ||
) | ||
if 'AIRFLOW_HOME' in os.environ: | ||
warnings.warn(msg, category=DeprecationWarning) | ||
elif conf.get('core', 'airflow_home') == AIRFLOW_HOME: | ||
warnings.warn( | ||
'Specifying airflow_home in the config file is deprecated. As you ' | ||
'have left it at the default value you should remove the setting ' | ||
'from your airflow.cfg and suffer no change in behaviour.', | ||
category=DeprecationWarning, | ||
) | ||
else: | ||
AIRFLOW_HOME = conf.get('core', 'airflow_home') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the purpose of having this line? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Double-checking on this: so your intention is that if the user is still using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I was trying to make this change as easy as possible - warn but carry on using the setting (for 99% of people it will be the same) Though the 1 case I know that this will break is the Puckel docker container. So perhaps if the two settings are different I should throw an exception. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or throw an exception once Personally I think it's better to make it as explicit as possible, and having a clean cut. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Throwing an exception in this case would be a breaking change (as this was in the default template, so every install would and up stopping working) which I am trying to avoid. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case, it’s not really necessary to have an if-else here to check whether AIRFLOW_HOME is in env? The two warning messages are not that different to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @ashb , I had a double-check on this. Let me correct myself for this:
if conf.has_option('core', 'AIRFLOW_HOME'):
warnings.warn('<warning msg>', category=DeprecationWarning,)
if 'AIRFLOW_HOME' not in os.environ:
AIRFLOW_HOME = conf.get('core', 'airflow_home') There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've changed it to: if conf.has_option('core', 'AIRFLOW_HOME'):
msg = (
'Specifying both AIRFLOW_HOME environment variable and airflow_home '
'in the config file is deprecated. Please use only the AIRFLOW_HOME '
'environment variable and remove the config file entry.'
)
if 'AIRFLOW_HOME' in os.environ:
warnings.warn(msg, category=DeprecationWarning,)
elif conf.get('core', 'airflow_home') == AIRFLOW_HOME:
warnings.warn(
'Specifying airflow_home in the config file is deprecated. As you '
'have left it at the default value you should remove the setting '
'from your airflow.cfg and suffer no change in behaviour.',
category=DeprecationWarning,
)
else:
AIRFLOW_HOME = conf.get('core', 'airflow_home')
warnings.warn(msg, category=DeprecationWarning,) I thought about adding an extra case do say which value it was |
||
warnings.warn(msg, category=DeprecationWarning) | ||
|
||
|
||
WEBSERVER_CONFIG = AIRFLOW_HOME + '/webserver_config.py' | ||
|
||
if not os.path.isfile(WEBSERVER_CONFIG): | ||
log.info('Creating new FAB webserver config file in: %s', WEBSERVER_CONFIG) | ||
DEFAULT_WEBSERVER_CONFIG = _read_default_config_file('default_webserver_config.py') | ||
with open(WEBSERVER_CONFIG, 'w') as f: | ||
f.write(DEFAULT_WEBSERVER_CONFIG) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,13 +26,13 @@ | |
import logging | ||
import os | ||
import pendulum | ||
|
||
import sys | ||
|
||
from sqlalchemy import create_engine, exc | ||
from sqlalchemy.orm import scoped_session, sessionmaker | ||
from sqlalchemy.pool import NullPool | ||
|
||
from airflow import configuration as conf | ||
from airflow.configuration import conf, AIRFLOW_HOME, WEBSERVER_CONFIG # NOQA F401 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's so we can do |
||
from airflow.logging_config import configure_logging | ||
from airflow.utils.sqlalchemy import setup_event_handlers | ||
|
||
|
@@ -67,9 +67,10 @@ | |
LOG_FORMAT = conf.get('core', 'log_format') | ||
SIMPLE_LOG_FORMAT = conf.get('core', 'simple_log_format') | ||
|
||
AIRFLOW_HOME = None | ||
SQL_ALCHEMY_CONN = None | ||
DAGS_FOLDER = None | ||
PLUGINS_FOLDER = None | ||
LOGGING_CLASS_PATH = None | ||
|
||
engine = None | ||
Session = None | ||
|
@@ -103,13 +104,18 @@ def policy(task_instance): | |
|
||
|
||
def configure_vars(): | ||
global AIRFLOW_HOME | ||
global SQL_ALCHEMY_CONN | ||
global DAGS_FOLDER | ||
AIRFLOW_HOME = os.path.expanduser(conf.get('core', 'AIRFLOW_HOME')) | ||
global PLUGINS_FOLDER | ||
SQL_ALCHEMY_CONN = conf.get('core', 'SQL_ALCHEMY_CONN') | ||
DAGS_FOLDER = os.path.expanduser(conf.get('core', 'DAGS_FOLDER')) | ||
|
||
PLUGINS_FOLDER = conf.get( | ||
'core', | ||
'plugins_folder', | ||
fallback=os.path.join(AIRFLOW_HOME, 'plugins') | ||
) | ||
|
||
|
||
def configure_orm(disable_connection_pool=False): | ||
log.debug("Setting up DB connection pool (PID %s)" % os.getpid()) | ||
|
@@ -216,21 +222,44 @@ def configure_action_logging(): | |
pass | ||
|
||
|
||
def prepare_classpath(): | ||
""" | ||
Ensures that certain subfolders of AIRFLOW_HOME are on the classpath | ||
""" | ||
|
||
if DAGS_FOLDER not in sys.path: | ||
sys.path.append(DAGS_FOLDER) | ||
|
||
# Add ./config/ for loading custom log parsers etc, or | ||
# airflow_local_settings etc. | ||
config_path = os.path.join(AIRFLOW_HOME, 'config') | ||
if config_path not in sys.path: | ||
sys.path.append(config_path) | ||
|
||
if PLUGINS_FOLDER not in sys.path: | ||
sys.path.append(PLUGINS_FOLDER) | ||
|
||
|
||
try: | ||
from airflow_local_settings import * # noqa F403 F401 | ||
log.info("Loaded airflow_local_settings.") | ||
except Exception: | ||
pass | ||
|
||
logging_class_path = configure_logging() | ||
configure_vars() | ||
configure_adapters() | ||
# The webservers import this file from models.py with the default settings. | ||
configure_orm() | ||
configure_action_logging() | ||
|
||
# Ensure we close DB connections at scheduler and gunicon worker terminations | ||
atexit.register(dispose_orm) | ||
def initialize(): | ||
configure_vars() | ||
prepare_classpath() | ||
global LOGGING_CLASS_PATH | ||
LOGGING_CLASS_PATH = configure_logging() | ||
configure_adapters() | ||
# The webservers import this file from models.py with the default settings. | ||
configure_orm() | ||
configure_action_logging() | ||
|
||
# Ensure we close DB connections at scheduler and gunicon worker terminations | ||
atexit.register(dispose_orm) | ||
|
||
|
||
# Const stuff | ||
|
||
|
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.
@ashb I'm wondering what will happen if I have both env variables
AIRFLOW_HOME
andAIRFLOW__CORE__AIRFLOW_HOME
, but set to different values?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.
To the code the second one is the same as setting it in the config file
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.
Having re-read this: this is the very problem I ran in to that caused me to issue the PR. If the are set to different values then the webserver_config.py would be looked for in one, but other things from the other.
Specifically the problem I had was that webserver_config.py was written to one, but read from the other! So
airflow create_user
etc targeted ansqlite://:memory:
DB!