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

Unify user session lifetime configuration #11970

Merged

Conversation

michalmisiewicz
Copy link
Contributor

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 has impact on session lifetime, but it is limited to values in day. In this PR I have simplified user session lifetime configuration.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Oct 30, 2020
@michalmisiewicz
Copy link
Contributor Author

michalmisiewicz commented Oct 30, 2020

@mik-laj I had to recreate the #11745 PR

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Oct 30, 2020
@github-actions
Copy link

The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run it!

airflow/www/app.py Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the full tests needed We need to run full set of tests for this PR to merge label Oct 30, 2020
@kaxil
Copy link
Member

kaxil commented Nov 4, 2020

Can you please rebased your PR on latest Master since we have applied Black and PyUpgrade on Master.

It will help if your squash your commits into single commit first so that there are less conflicts.

@michalmisiewicz michalmisiewicz force-pushed the unify-user-session-lifetime-configuration branch from 67f55a9 to 362b45f Compare November 4, 2020 10:54
@michalmisiewicz
Copy link
Contributor Author

@kaxil I've rebased with master

@github-actions
Copy link

github-actions bot commented Nov 4, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

github-actions bot commented Nov 4, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@mik-laj mik-laj requested a review from XD-DENG November 4, 2020 18:04
@mik-laj
Copy link
Member

mik-laj commented Nov 4, 2020

@michalmisiewicz One comment left. #11970 (comment) Can you look at it, please?

@XD-DENG
Copy link
Member

XD-DENG commented Nov 4, 2020

Hi @mik-laj , would like to have your opinion as well: if you find my comment above makes sense, let's wait for the contributor to reply; But if you find the current implementation is good enough, I'm ok to have it merged.

@mik-laj
Copy link
Member

mik-laj commented Nov 4, 2020

@XD-DENG Both approaches are fine for me. I have no preferences. I can see that @msumit gave the thumbs up, so I guess it'll be better if we accept your proposal.

@github-actions
Copy link

github-actions bot commented Nov 6, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

airflow/www/extensions/stop_webserver.py Outdated Show resolved Hide resolved
airflow/www/app.py Outdated Show resolved Hide resolved
airflow/www/app.py Outdated Show resolved Hide resolved
@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

UPDATING.md Outdated Show resolved Hide resolved
@michalmisiewicz michalmisiewicz force-pushed the unify-user-session-lifetime-configuration branch from 54d6037 to 6d3c5f8 Compare November 11, 2020 08:53
Copy link
Member

@XD-DENG XD-DENG left a comment

Choose a reason for hiding this comment

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

Many thanks @michalmisiewicz

@github-actions
Copy link

The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run it!

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Nov 11, 2020
@XD-DENG XD-DENG merged commit e03a3f4 into apache:master Nov 11, 2020
'from `webserver` section. Please update your configuration.'
)
# Stop gunicorn server https://github.com/benoitc/gunicorn/blob/20.0.4/gunicorn/arbiter.py#L526
sys.exit(4)
Copy link
Member

Choose a reason for hiding this comment

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

NOOOOOOOOO. Upgrade, don't error.

@paolaperaza paolaperaza added this to the Airflow 1.10.13 milestone Nov 16, 2020
kaxil pushed a commit that referenced this pull request Nov 20, 2020
* Unify user session lifetime configuration

* align with new linting rules

* exit app when removed args are provided in conf

* add one more test

* extract stopping gunicorn to method

* add docstring to stop_webserver method

* use lazy formatting

* exit webserver when removed options are provided

* align with markdown lint

* Move unify user session lifetime configuration section to master

* add new line

* remove quotes
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
* Unify user session lifetime configuration

* align with new linting rules

* exit app when removed args are provided in conf

* add one more test

* extract stopping gunicorn to method

* add docstring to stop_webserver method

* use lazy formatting

* exit webserver when removed options are provided

* align with markdown lint

* Move unify user session lifetime configuration section to master

* add new line

* remove quotes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants