Skip to content

Fixed the tenant switching after timeout#1090

Merged
peternied merged 1 commit intoopensearch-project:mainfrom
RyanL1997:bug-1084
Sep 14, 2022
Merged

Fixed the tenant switching after timeout#1090
peternied merged 1 commit intoopensearch-project:mainfrom
RyanL1997:bug-1084

Conversation

@RyanL1997
Copy link
Collaborator

@RyanL1997 RyanL1997 commented Aug 31, 2022

Signed-off-by: Ryan Liang jiallian@amazon.com

Description

Fix the issue of user tenant automatically switch back to Private/Global by default after session timeout.

Category

Bug fix

Issues Resolved

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@RyanL1997 RyanL1997 requested a review from a team August 31, 2022 14:33
@RyanL1997 RyanL1997 marked this pull request as draft August 31, 2022 21:31
@RyanL1997 RyanL1997 force-pushed the bug-1084 branch 3 times, most recently from 8f2521d to a9c1f3e Compare August 31, 2022 23:54
@RyanL1997 RyanL1997 marked this pull request as ready for review September 1, 2022 01:00
cliu123
cliu123 previously approved these changes Sep 1, 2022
cwperks
cwperks previously approved these changes Sep 2, 2022
Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @RyanL1997!

@cwperks cwperks added the backport 2.x backport to 2.x branch label Sep 2, 2022
@expani expani mentioned this pull request Sep 5, 2022
3 tasks
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Need to add tests to verify this change

@cliu123
Copy link
Member

cliu123 commented Sep 6, 2022

@RyanL1997 Could you please add the manual testing in the PR description for now and add ITs in a follow up PR if adding ITs will take time?

@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2022

Codecov Report

Merging #1090 (0036ccd) into main (d89d31b) will increase coverage by 0.16%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1090      +/-   ##
==========================================
+ Coverage   72.27%   72.43%   +0.16%     
==========================================
  Files          87       88       +1     
  Lines        1915     1919       +4     
  Branches      244      246       +2     
==========================================
+ Hits         1384     1390       +6     
+ Misses        478      474       -4     
- Partials       53       55       +2     
Impacted Files Coverage Δ
...lugins/security-dashboards-plugin/public/plugin.ts 0.00% <0.00%> (ø)
...ty-dashboards-plugin/public/utils/logout-utils.tsx 66.66% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@RyanL1997 RyanL1997 marked this pull request as draft September 12, 2022 15:47
@RyanL1997 RyanL1997 dismissed stale reviews from cwperks and cliu123 via 438e7ed September 12, 2022 16:33
@RyanL1997 RyanL1997 force-pushed the bug-1084 branch 2 times, most recently from 7b6bcf9 to 169abd6 Compare September 12, 2022 17:54
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests - a couple of comments about theses and I think we should add another test case

@RyanL1997 RyanL1997 force-pushed the bug-1084 branch 2 times, most recently from 39bb689 to 1f8c84b Compare September 12, 2022 18:27
@RyanL1997 RyanL1997 force-pushed the bug-1084 branch 4 times, most recently from 5af7e6a to 03610fb Compare September 12, 2022 21:02
@RyanL1997 RyanL1997 marked this pull request as ready for review September 12, 2022 21:14
peternied
peternied previously approved these changes Sep 12, 2022
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Looks good, I think you'll need to rebase 2.4.0 has merged otherwise intergration tests aren't going to work

@peternied
Copy link
Member

See #1096

@RyanL1997
Copy link
Collaborator Author

@RyanL1997 Could you please add the manual testing in the PR description for now and add ITs in a follow up PR if adding ITs will take time?

@cliu123 Sorry for the late reply. I have been working on the unit tests and I just updated the PR with automation tests cases.

@RyanL1997
Copy link
Collaborator Author

Looks good, I think you'll need to rebase 2.4.0 has merged otherwise intergration tests aren't going to work

@peternied Got it! Thank you again for the review!

Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

@RyanL1997
Copy link
Collaborator Author

Looks good Ryan. Can you please rebase it with https://github.com/cwperks/security-dashboards-plugin/tree/increment-to-2.4.0.0 so ITs can work

@DarshitChanpura I just did the rebase and pushed. Thx DC!

@cliu123
Copy link
Member

cliu123 commented Sep 13, 2022

@RyanL1997 The rebase was not done properly. The version bump changes are not supposed to be present in this PR.

Signed-off-by: Ryan Liang <jiallian@amazon.com>
@RyanL1997
Copy link
Collaborator Author

@RyanL1997 The rebase was not done properly. The version bump changes are not supposed to be present in this PR.

Hi @cliu123, ty for the review, and I just fixed this.

Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

Ty @RyanL1997 !

@peternied peternied merged commit 5d018b0 into opensearch-project:main Sep 14, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 14, 2022
Signed-off-by: Ryan Liang <jiallian@amazon.com>
(cherry picked from commit 5d018b0)
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 14, 2022
Signed-off-by: Ryan Liang <jiallian@amazon.com>
(cherry picked from commit 5d018b0)
peternied pushed a commit that referenced this pull request Sep 14, 2022
Signed-off-by: Ryan Liang <jiallian@amazon.com>
(cherry picked from commit 5d018b0)

Co-authored-by: Ryan Liang <109499885+RyanL1997@users.noreply.github.com>
peternied pushed a commit that referenced this pull request Sep 14, 2022
Signed-off-by: Ryan Liang <jiallian@amazon.com>
(cherry picked from commit 5d018b0)

Co-authored-by: Ryan Liang <109499885+RyanL1997@users.noreply.github.com>
aoguan1990 pushed a commit to aoguan1990/security-dashboards-plugin that referenced this pull request Sep 26, 2022
Signed-off-by: Ryan Liang <jiallian@amazon.com>
Signed-off-by: Aozixuan Priscilla Guan <aoguan@amazon.com>
@vinayak15 vinayak15 mentioned this pull request Oct 6, 2022
3 tasks
@RyanL1997 RyanL1997 deleted the bug-1084 branch December 6, 2022 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 1.3 backport 2.x backport to 2.x branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Tenant keeps switching to Global

6 participants