Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Feature: Session lifetime config in resource policy #1304

Merged
merged 6 commits into from
May 10, 2022

Conversation

lizable
Copy link
Contributor

@lizable lizable commented May 9, 2022

Description

This PR resolves a new configuration max_session_lifetime, which can be applied in resource policy.
Related: manager#583

⚠️ NOTE:

  • Value "0" in max_session_lifetime means Infinite(♾) likewise in idle_timeout.
  • This feature is available from 22.03.

Before

  • Create Resource Policy Dialog

Create-Resource-Policy-before

  • Update Resource Policy Dialog

Update-Resource-Policy-before

After

  • Create Resource Policy Dialog

Create-Resource-Policy

  • Update Resource Policy Dialog

Update-Resource-Policy

@lizable lizable added type:enhance Add new features good first issue effort:easy Need to understand only a specific region of codes (good first issue, easy). area:lib Library and SDK related issue. area:ux UI / UX issue. labels May 9, 2022
@lizable lizable added this to the 22.03 milestone May 9, 2022
@lizable lizable requested review from inureyes, adrysn and agatha197 May 9, 2022 09:16
@lizable lizable self-assigned this May 9, 2022
@github-actions github-actions bot added the area:i18n Localization label May 9, 2022
Copy link
Member

@adrysn adrysn left a comment

Choose a reason for hiding this comment

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

Minor code style issues.

Comment on lines 3373 to 3375
@click="${(e) => {this._chooseResourceTemplate(e);}}"
@click="${(e) => {
this._chooseResourceTemplate(e);
}}"
Copy link
Member

Choose a reason for hiding this comment

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

This worsens the readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, I guess it's because of "eslint" code styling. I'll revert to the previous code style.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps, we need to update the eslint config not to rearrange the code in a weird way.

@lizable lizable requested a review from adrysn May 9, 2022 15:20
Copy link
Member

@adrysn adrysn left a comment

Choose a reason for hiding this comment

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

Further minor styling issues. We may need to change the eslint config.

Comment on lines 3444 to 3446
@change="${(e) => {this._applyResourceValueChanges(e); this._updateShmemLimit()}}"
@change="${(e) => {
this._applyResourceValueChanges(e); this._updateShmemLimit();
}}"
Copy link
Member

Choose a reason for hiding this comment

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

Here as well.

Comment on lines 3459 to 3463
@change="${(e) => {this._applyResourceValueChanges(e); this._updateShmemLimit()}}"
@change="${(e) => {
this._applyResourceValueChanges(e); this._updateShmemLimit();
}}"
Copy link
Member

Choose a reason for hiding this comment

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

Here as well.

@lizable
Copy link
Contributor Author

lizable commented May 9, 2022

Further minor styling issues. We may need to change the eslint config.

I couldn't agree more about updating our linting rules. I found a workaround plugin that doesn't ruin the indentation of template literal (not 100% but, much better than the current linting rules). I'll apply it as a new feature and make PR for it.

Copy link
Member

@adrysn adrysn left a comment

Choose a reason for hiding this comment

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

LGTM.

@adrysn adrysn merged commit aab21fb into main May 10, 2022
@adrysn adrysn deleted the feature/session-lifetime-config-in-resource-policy branch May 10, 2022 00:57
@inureyes inureyes removed the effort:1 label Mar 27, 2023
@Yaminyam Yaminyam added the size:XL 500~ LoC label Apr 13, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area:i18n Localization area:lib Library and SDK related issue. area:ux UI / UX issue. effort:easy Need to understand only a specific region of codes (good first issue, easy). size:XL 500~ LoC type:enhance Add new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants