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 2020/10/22 11:47:23 UTC
[GitHub] [airflow] michalmisiewicz opened a new pull request #11745: Unify user session lifetime configuration
michalmisiewicz opened a new pull request #11745:
URL: https://github.com/apache/airflow/pull/11745
In current version of Airflow user session lifetime can be configured by `session_lifetime_days` and `force_log_out_after` options. In practise only `session_lifetime_days` have impact on session lifetime, but it is limited to values in day. In this PR I have simplified user session lifetime configuration.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zacharya19 commented on a change in pull request #11745: Unify user session lifetime configuration
Posted by GitBox <gi...@apache.org>.
zacharya19 commented on a change in pull request #11745:
URL: https://github.com/apache/airflow/pull/11745#discussion_r510341791
##########
File path: tests/www/test_app.py
##########
@@ -215,3 +217,27 @@ def test_should_set_sqlalchemy_engine_options(self):
'max_overflow': 5
}
self.assertEqual(app.config['SQLALCHEMY_ENGINE_OPTIONS'], engine_params)
+
+ @conf_vars({
+ ('webserver', 'session_lifetime_minutes'): '3600',
+ })
+ @mock.patch("airflow.www.app.app", None)
+ def test_should_set_permanent_session_timeout(self):
+ app = application.cached_app(testing=True)
+ self.assertEqual(app.config['PERMANENT_SESSION_LIFETIME'], timedelta(minutes=3600))
+
+ @conf_vars({
+ ('webserver', 'session_lifetime_days'): '30',
+ ('webserver', 'force_log_out_after'): '30',
+ })
+ @mock.patch("airflow.www.app.app", None)
+ def test_should_emit_deprecation_warnings(self):
+ with pytest.warns(DeprecationWarning) as warns:
+ application.cached_app(testing=True)
+ app_path = str(Path(__file__).parents[2].joinpath('airflow', 'www', 'app.py'))
+ warns = [w for w in warns if w.filename == app_path]
+ self.assertEqual(len(warns), 2)
+ self.assertEqual(warns[0].message.args[0],
Review comment:
My main issue with this function is that if we tomorrow add new deprecation warnings it can break this test, while it shouldn't.
Why not just check in general that those messages exists in warns? Why filter by filename and be specific on the index number?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] michalmisiewicz commented on a change in pull request #11745: Unify user session lifetime configuration
Posted by GitBox <gi...@apache.org>.
michalmisiewicz commented on a change in pull request #11745:
URL: https://github.com/apache/airflow/pull/11745#discussion_r510391076
##########
File path: airflow/www/app.py
##########
@@ -66,8 +66,16 @@ def create_app(config=None, testing=False, app_name="Airflow"):
flask_app = Flask(__name__)
flask_app.secret_key = conf.get('webserver', 'SECRET_KEY')
- session_lifetime_days = conf.getint('webserver', 'SESSION_LIFETIME_DAYS', fallback=30)
- flask_app.config['PERMANENT_SESSION_LIFETIME'] = timedelta(days=session_lifetime_days)
+ if conf.has_option('webserver', 'SESSION_LIFETIME_DAYS'):
+ warnings.warn('SESSION_LIFETIME_DAYS option is deprecated. Please use `SESSION_LIFETIME_MINUTES`',
+ DeprecationWarning)
+
+ if conf.has_option('webserver', 'FORCE_LOG_OUT_AFTER'):
+ warnings.warn('FORCE_LOG_OUT_AFTER option is deprecated. Please use `SESSION_LIFETIME_MINUTES`',
Review comment:
I believe that we should warn user about removed options, as this is breaking change. Warning can be removed in next Airflow release.
I can changed message to `option is removed. Please use ...`
Please let me know if I should remove this warning.
##########
File path: airflow/www/app.py
##########
@@ -66,8 +66,16 @@ def create_app(config=None, testing=False, app_name="Airflow"):
flask_app = Flask(__name__)
flask_app.secret_key = conf.get('webserver', 'SECRET_KEY')
- session_lifetime_days = conf.getint('webserver', 'SESSION_LIFETIME_DAYS', fallback=30)
- flask_app.config['PERMANENT_SESSION_LIFETIME'] = timedelta(days=session_lifetime_days)
+ if conf.has_option('webserver', 'SESSION_LIFETIME_DAYS'):
+ warnings.warn('SESSION_LIFETIME_DAYS option is deprecated. Please use `SESSION_LIFETIME_MINUTES`',
+ DeprecationWarning)
+
+ if conf.has_option('webserver', 'FORCE_LOG_OUT_AFTER'):
+ warnings.warn('FORCE_LOG_OUT_AFTER option is deprecated. Please use `SESSION_LIFETIME_MINUTES`',
Review comment:
I believe that we should warn user about removed options, as this is breaking change. Warnings can be removed in next Airflow release.
I can changed message to `option is removed. Please use ...`
Please let me know if I should remove this warning.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] michalmisiewicz commented on pull request #11745: Unify user session lifetime configuration
Posted by GitBox <gi...@apache.org>.
michalmisiewicz commented on pull request #11745:
URL: https://github.com/apache/airflow/pull/11745#issuecomment-719468987
@mik-laj @turbaszek can we merge it ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] michalmisiewicz commented on a change in pull request #11745: Unify user session lifetime configuration
Posted by GitBox <gi...@apache.org>.
michalmisiewicz commented on a change in pull request #11745:
URL: https://github.com/apache/airflow/pull/11745#discussion_r510391076
##########
File path: airflow/www/app.py
##########
@@ -66,8 +66,16 @@ def create_app(config=None, testing=False, app_name="Airflow"):
flask_app = Flask(__name__)
flask_app.secret_key = conf.get('webserver', 'SECRET_KEY')
- session_lifetime_days = conf.getint('webserver', 'SESSION_LIFETIME_DAYS', fallback=30)
- flask_app.config['PERMANENT_SESSION_LIFETIME'] = timedelta(days=session_lifetime_days)
+ if conf.has_option('webserver', 'SESSION_LIFETIME_DAYS'):
+ warnings.warn('SESSION_LIFETIME_DAYS option is deprecated. Please use `SESSION_LIFETIME_MINUTES`',
+ DeprecationWarning)
+
+ if conf.has_option('webserver', 'FORCE_LOG_OUT_AFTER'):
+ warnings.warn('FORCE_LOG_OUT_AFTER option is deprecated. Please use `SESSION_LIFETIME_MINUTES`',
Review comment:
I believe that we should warn user about removed options. Warning can be removed in next Airflow release.
I can changed message to `option is removed. Please use ...`
Please let me know if I should remove this warning.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] michalmisiewicz commented on a change in pull request #11745: Unify user session lifetime configuration
Posted by GitBox <gi...@apache.org>.
michalmisiewicz commented on a change in pull request #11745:
URL: https://github.com/apache/airflow/pull/11745#discussion_r510391076
##########
File path: airflow/www/app.py
##########
@@ -66,8 +66,16 @@ def create_app(config=None, testing=False, app_name="Airflow"):
flask_app = Flask(__name__)
flask_app.secret_key = conf.get('webserver', 'SECRET_KEY')
- session_lifetime_days = conf.getint('webserver', 'SESSION_LIFETIME_DAYS', fallback=30)
- flask_app.config['PERMANENT_SESSION_LIFETIME'] = timedelta(days=session_lifetime_days)
+ if conf.has_option('webserver', 'SESSION_LIFETIME_DAYS'):
+ warnings.warn('SESSION_LIFETIME_DAYS option is deprecated. Please use `SESSION_LIFETIME_MINUTES`',
+ DeprecationWarning)
+
+ if conf.has_option('webserver', 'FORCE_LOG_OUT_AFTER'):
+ warnings.warn('FORCE_LOG_OUT_AFTER option is deprecated. Please use `SESSION_LIFETIME_MINUTES`',
Review comment:
I believe that we should warn user about removed options, as this is breaking change. Warnings can be removed in next Airflow release. I can changed message to `option is removed. Please use ...`
Please let me know if I should remove this warning.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] michalmisiewicz commented on a change in pull request #11745: Unify user session lifetime configuration
Posted by GitBox <gi...@apache.org>.
michalmisiewicz commented on a change in pull request #11745:
URL: https://github.com/apache/airflow/pull/11745#discussion_r511972115
##########
File path: airflow/www/app.py
##########
@@ -66,8 +66,16 @@ def create_app(config=None, testing=False, app_name="Airflow"):
flask_app = Flask(__name__)
flask_app.secret_key = conf.get('webserver', 'SECRET_KEY')
- session_lifetime_days = conf.getint('webserver', 'SESSION_LIFETIME_DAYS', fallback=30)
- flask_app.config['PERMANENT_SESSION_LIFETIME'] = timedelta(days=session_lifetime_days)
+ if conf.has_option('webserver', 'SESSION_LIFETIME_DAYS'):
Review comment:
@mik-laj are we ready to go ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] tooptoop4 commented on a change in pull request #11745: Unify user session lifetime configuration
Posted by GitBox <gi...@apache.org>.
tooptoop4 commented on a change in pull request #11745:
URL: https://github.com/apache/airflow/pull/11745#discussion_r510382075
##########
File path: airflow/www/app.py
##########
@@ -66,8 +66,16 @@ def create_app(config=None, testing=False, app_name="Airflow"):
flask_app = Flask(__name__)
flask_app.secret_key = conf.get('webserver', 'SECRET_KEY')
- session_lifetime_days = conf.getint('webserver', 'SESSION_LIFETIME_DAYS', fallback=30)
- flask_app.config['PERMANENT_SESSION_LIFETIME'] = timedelta(days=session_lifetime_days)
+ if conf.has_option('webserver', 'SESSION_LIFETIME_DAYS'):
+ warnings.warn('SESSION_LIFETIME_DAYS option is deprecated. Please use `SESSION_LIFETIME_MINUTES`',
+ DeprecationWarning)
+
+ if conf.has_option('webserver', 'FORCE_LOG_OUT_AFTER'):
+ warnings.warn('FORCE_LOG_OUT_AFTER option is deprecated. Please use `SESSION_LIFETIME_MINUTES`',
Review comment:
not deprecated, removed. can remove this and just populate the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] zhaitin-quotient commented on a change in pull request #11745: Unify user session lifetime configuration
Posted by GitBox <gi...@apache.org>.
zhaitin-quotient commented on a change in pull request #11745:
URL: https://github.com/apache/airflow/pull/11745#discussion_r510340862
##########
File path: tests/www/test_app.py
##########
@@ -215,3 +217,27 @@ def test_should_set_sqlalchemy_engine_options(self):
'max_overflow': 5
}
self.assertEqual(app.config['SQLALCHEMY_ENGINE_OPTIONS'], engine_params)
+
+ @conf_vars({
+ ('webserver', 'session_lifetime_minutes'): '3600',
+ })
+ @mock.patch("airflow.www.app.app", None)
+ def test_should_set_permanent_session_timeout(self):
+ app = application.cached_app(testing=True)
+ self.assertEqual(app.config['PERMANENT_SESSION_LIFETIME'], timedelta(minutes=3600))
+
+ @conf_vars({
+ ('webserver', 'session_lifetime_days'): '30',
+ ('webserver', 'force_log_out_after'): '30',
+ })
+ @mock.patch("airflow.www.app.app", None)
+ def test_should_emit_deprecation_warnings(self):
+ with pytest.warns(DeprecationWarning) as warns:
+ application.cached_app(testing=True)
+ app_path = str(Path(__file__).parents[2].joinpath('airflow', 'www', 'app.py'))
+ warns = [w for w in warns if w.filename == app_path]
+ self.assertEqual(len(warns), 2)
+ self.assertEqual(warns[0].message.args[0],
Review comment:
My main issue with this function is that if we tomorrow add new deprecation warnings it can break this test, while it shouldn't.
Why not just check in general that those messages exists in warns? Why filter by filename and be specific on the index number?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #11745: Unify user session lifetime configuration
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #11745:
URL: https://github.com/apache/airflow/pull/11745#discussion_r510427359
##########
File path: airflow/www/app.py
##########
@@ -66,8 +66,16 @@ def create_app(config=None, testing=False, app_name="Airflow"):
flask_app = Flask(__name__)
flask_app.secret_key = conf.get('webserver', 'SECRET_KEY')
- session_lifetime_days = conf.getint('webserver', 'SESSION_LIFETIME_DAYS', fallback=30)
- flask_app.config['PERMANENT_SESSION_LIFETIME'] = timedelta(days=session_lifetime_days)
+ if conf.has_option('webserver', 'SESSION_LIFETIME_DAYS'):
Review comment:
I think we should not warn users about configuration options that do not work if the user has the correct configuration options for Airflow 2.0. This will allow us to use one configuration file for Airflow 1.10 and 2.0. This is essential if users want to use Helm Chart and other generic deployment tools.
So I propose that you check if there is a `SESSION_LIFETIME_MINUTES` option in the config file. If so, use it.
If it is not there, we should display a deprecation warning and we should try to maintain backward compatibility. In this case, this means that the warning should display a message that option `SESSION_LIFETIME_DAYS` has been renamed to `SESSION_LIFETIME_MINUTES` and the unit from day to hour and that option `FORCE_LOG_OUT_AFTER` has been removed.
When this change has been merged, please create a ticket requesting a new rule for [airflow upgrade-check](https://github.com/apache/airflow/issues/8765).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] michalmisiewicz commented on a change in pull request #11745: Unify user session lifetime configuration
Posted by GitBox <gi...@apache.org>.
michalmisiewicz commented on a change in pull request #11745:
URL: https://github.com/apache/airflow/pull/11745#discussion_r510456780
##########
File path: airflow/www/app.py
##########
@@ -66,8 +66,16 @@ def create_app(config=None, testing=False, app_name="Airflow"):
flask_app = Flask(__name__)
flask_app.secret_key = conf.get('webserver', 'SECRET_KEY')
- session_lifetime_days = conf.getint('webserver', 'SESSION_LIFETIME_DAYS', fallback=30)
- flask_app.config['PERMANENT_SESSION_LIFETIME'] = timedelta(days=session_lifetime_days)
+ if conf.has_option('webserver', 'SESSION_LIFETIME_DAYS'):
Review comment:
@mik-laj I've updated the code
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #11745: Unify user session lifetime configuration
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #11745:
URL: https://github.com/apache/airflow/pull/11745#issuecomment-714642848
@tooptoop4 @zacharya19 Can you look at it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] michalmisiewicz commented on a change in pull request #11745: Unify user session lifetime configuration
Posted by GitBox <gi...@apache.org>.
michalmisiewicz commented on a change in pull request #11745:
URL: https://github.com/apache/airflow/pull/11745#discussion_r510361282
##########
File path: tests/www/test_app.py
##########
@@ -215,3 +217,27 @@ def test_should_set_sqlalchemy_engine_options(self):
'max_overflow': 5
}
self.assertEqual(app.config['SQLALCHEMY_ENGINE_OPTIONS'], engine_params)
+
+ @conf_vars({
+ ('webserver', 'session_lifetime_minutes'): '3600',
+ })
+ @mock.patch("airflow.www.app.app", None)
+ def test_should_set_permanent_session_timeout(self):
+ app = application.cached_app(testing=True)
+ self.assertEqual(app.config['PERMANENT_SESSION_LIFETIME'], timedelta(minutes=3600))
+
+ @conf_vars({
+ ('webserver', 'session_lifetime_days'): '30',
+ ('webserver', 'force_log_out_after'): '30',
+ })
+ @mock.patch("airflow.www.app.app", None)
+ def test_should_emit_deprecation_warnings(self):
+ with pytest.warns(DeprecationWarning) as warns:
+ application.cached_app(testing=True)
+ app_path = str(Path(__file__).parents[2].joinpath('airflow', 'www', 'app.py'))
+ warns = [w for w in warns if w.filename == app_path]
+ self.assertEqual(len(warns), 2)
+ self.assertEqual(warns[0].message.args[0],
Review comment:
Resolved
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] tooptoop4 commented on a change in pull request #11745: Unify user session lifetime configuration
Posted by GitBox <gi...@apache.org>.
tooptoop4 commented on a change in pull request #11745:
URL: https://github.com/apache/airflow/pull/11745#discussion_r510411609
##########
File path: airflow/www/app.py
##########
@@ -66,8 +66,16 @@ def create_app(config=None, testing=False, app_name="Airflow"):
flask_app = Flask(__name__)
flask_app.secret_key = conf.get('webserver', 'SECRET_KEY')
- session_lifetime_days = conf.getint('webserver', 'SESSION_LIFETIME_DAYS', fallback=30)
- flask_app.config['PERMANENT_SESSION_LIFETIME'] = timedelta(days=session_lifetime_days)
+ if conf.has_option('webserver', 'SESSION_LIFETIME_DAYS'):
+ warnings.warn('SESSION_LIFETIME_DAYS option is deprecated. Please use `SESSION_LIFETIME_MINUTES`',
+ DeprecationWarning)
+
+ if conf.has_option('webserver', 'FORCE_LOG_OUT_AFTER'):
+ warnings.warn('FORCE_LOG_OUT_AFTER option is deprecated. Please use `SESSION_LIFETIME_MINUTES`',
Review comment:
option is removed. Please use ...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org