XD-DENG commented on a change in pull request #11970:
URL: https://github.com/apache/airflow/pull/11970#discussion_r518932462
##########
File path: airflow/www/app.py
##########
@@ -70,8 +71,27 @@ 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)
+ default_session_lifetime = 43200
Review comment:
A minor one: I suggest to make the name more explicit, say
`default_session_lifetime_mins`.
##########
File path: airflow/www/app.py
##########
@@ -70,8 +71,27 @@ 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)
+ default_session_lifetime = 43200
+ if conf.has_option('webserver', 'SESSION_LIFETIME_MINUTES'):
+ session_lifetime_minutes = conf.getint(
+ 'webserver', 'SESSION_LIFETIME_MINUTES',
fallback=default_session_lifetime
+ )
+ else:
+ if conf.has_option('webserver', 'SESSION_LIFETIME_DAYS') or
conf.has_option(
+ 'webserver', 'FORCE_LOG_OUT_AFTER'
+ ):
+ logging.error(
+ '`SESSION_LIFETIME_DAYS` option from `webserver` section has
been '
+ 'renamed to `SESSION_LIFETIME_MINUTES`. New option allows to
configure '
+ 'session lifetime in minutes. FORCE_LOG_OUT_AFTER option has
been removed '
+ 'from `webserver` section. Please update your configuration.'
+ )
+ stop_webserver()
+ else:
+ logging.info('Using default value for `SESSION_LIFETIME_MINUTES`:
%s', default_session_lifetime)
+ session_lifetime_minutes = default_session_lifetime
Review comment:
This `if-else` condition check is allowing one situation which should
not happen in my opinion: if a user has both `SESSION_LIFETIME_MINUTES` AND
`SESSION_LIFETIME_DAYS`/`FORCE_LOG_OUT_AFTER` in his/her configuration, NO
error will be raised. Of course it will correctly use
`SESSION_LIFETIME_MINUTES`. However, when another user looks at this
configuration, it will be confusing and misleading.
So I would like to suggest change the `if-else` check into structure below:
```python
if conf has SESSION_LIFETIME_DAYS or FORCE_LOG_OUT_AFTER:
logging.error() + exit()
if conf.has_option('webserver', 'SESSION_LIFETIME_MINUTES'):
session_lifetime_minutes = conf.getint('webserver',
'SESSION_LIFETIME_MINUTES', ......)
else:
logging.info("Using default value for SESSION_LIFETIME_MINUTES)
session_lifetime_minutes = default_session_lifetime
```
Let me know your thoughts? Thanks.
----------------------------------------------------------------
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:
[email protected]
