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

Add test for aggregation view feature #334

Merged
merged 3 commits into from
Oct 21, 2022

Conversation

RyanL1997
Copy link
Contributor

@RyanL1997 RyanL1997 commented Oct 18, 2022

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

Description

Add test for security dashboard plugin aggregation view feature

  • Modify current visit() command to support non-admin authentication and tenant switching when security feature is enabled.
  • Extend current createIndexPattern() command to support index pattern creation with tenant header when security feature is enabled

Issues Resolved

Check List

  • 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.

@CCongWang
Copy link
Collaborator

looks like many tests share the same code like creating user/role/etc, to make the code clean and reusable, one thing you could do is to create custom commands, like createUser() and createRole(), please check out index management plugin commands as example

@CCongWang
Copy link
Collaborator

do you need to backport the tests to other branches like 2.x?

@RyanL1997
Copy link
Contributor Author

do you need to backport the tests to other branches like 2.x?

Hi @CCongWang , thank you for asking. Yes, I believe we need to backport to 2.x.

@RyanL1997
Copy link
Contributor Author

RyanL1997 commented Oct 19, 2022

looks like many tests share the same code like creating user/role/etc, to make the code clean and reusable, one thing you could do is to create custom commands, like createUser() and createRole(), please check out index management plugin commands as example

Hi @CCongWang, thank you for the review! I just restructured the entire test with your suggestion of adding API call commands, such as, createUser() and createTenant(). By doing this, I believe it will also resolve all the rest comments. Feel free to review the latest commit and let me know if you have further questions.

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Hey Ryan, just have some comments to reduce some redundancy.

cypress/utils/commands.js Outdated Show resolved Hide resolved
cypress/utils/plugins/security/constants.js Outdated Show resolved Hide resolved
@RyanL1997 RyanL1997 requested review from CCongWang and ashwin-pc and removed request for CCongWang and ashwin-pc October 19, 2022 21:14
@RyanL1997 RyanL1997 force-pushed the saved-objects-test branch 2 times, most recently from 75d6771 to 2a6974b Compare October 19, 2022 22:03
cypress/utils/index.d.ts Outdated Show resolved Hide resolved
@tianleh
Copy link
Member

tianleh commented Oct 20, 2022

Hi @CCongWang , thank you for asking. Yes, I believe we need to backport to 2.x.

Please add a label called "backport 2.x" to this PR so that our backport workflow can automatically generate a new PR to backport once the current PR is merged.

@tianleh
Copy link
Member

tianleh commented Oct 20, 2022

Any reason why we need AGGREGATION_VIEW flag around the tests? Is this feature disabled by default?

@RyanL1997
Copy link
Contributor Author

Any reason why we need AGGREGATION_VIEW flag around the tests? Is this feature disabled by default?

Hi @tianleh, thanks for the review! Yes, the feature is disabled by default, so that the UI behaviors will be different. The test will only run when both of SECURITY_ENABLED and AGGREGATION_VIEW flags are true.

@RyanL1997 RyanL1997 changed the title Add test for aggregation view feature [Backport 2.x] Add test for aggregation view feature Oct 20, 2022
@RyanL1997
Copy link
Contributor Author

Hi @CCongWang , thank you for asking. Yes, I believe we need to backport to 2.x.

Please add a label called "backport 2.x" to this PR so that our backport workflow can automatically generate a new PR to backport once the current PR is merged.

Done. Thanks for the instruction.

@tianleh tianleh changed the title [Backport 2.x] Add test for aggregation view feature Add test for aggregation view feature Oct 20, 2022
@tianleh
Copy link
Member

tianleh commented Oct 20, 2022

Done. Thanks for the instruction.

I just added the label. (Label is different from PR title)

Screen Shot 2022-10-19 at 8 40 55 PM

@tianleh
Copy link
Member

tianleh commented Oct 20, 2022

Hi @tianleh, thanks for the review! Yes, the feature is disabled by default, so that the UI behaviors will be different. The test will only run when both of SECURITY_ENABLED and AGGREGATION_VIEW flags are true.

In this case, do you have plan to enable the feature when Github workflow runs https://github.com/opensearch-project/opensearch-dashboards-functional-test/blob/main/.github/workflows/security-release-e2e-workflow.yml ?

We have another feature which is disabled by default. https://github.com/opensearch-project/opensearch-dashboards-functional-test/pull/325/files The PR updates the Github workflow to start OSD with that feature enabled. I am not sure whether your feature can be enabled as simple as that or has to modify the OSD yml file.

The current run https://github.com/opensearch-project/opensearch-dashboards-functional-test/actions/runs/3286221746/jobs/5414100963 doesn't run your tests. Let me know your plan.

@cliu123
Copy link
Member

cliu123 commented Oct 20, 2022

@tianleh We don't have plan to enable the feature when the release e2e workflow runs. The feature gets enabled by setting a feature flag to true in opensearch_dashboards.yml file.
These tests run as a part of security-dashboards-plugin CI workflow: https://github.com/opensearch-project/security-dashboards-plugin/actions/runs/3285025563

Signed-off-by: Ryan Liang <jiallian@amazon.com>
Signed-off-by: Ryan Liang <jiallian@amazon.com>
@RyanL1997 RyanL1997 force-pushed the saved-objects-test branch 2 times, most recently from c71f27a to 08de7d0 Compare October 20, 2022 22:07
Signed-off-by: Ryan Liang <jiallian@amazon.com>
@RyanL1997
Copy link
Contributor Author

Thanks for all the reviews! I have resolved all the comments, and this PR is ready to be merged now.

@tianleh tianleh merged commit 4ebfa3d into opensearch-project:main Oct 21, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 21, 2022
@seraphjiang seraphjiang added enhancement New feature or request v2.4.0 'Issues and PRs related to version v2.4.0' labels Oct 25, 2022
seraphjiang pushed a commit that referenced this pull request Oct 25, 2022
(cherry picked from commit 4ebfa3d)

Co-authored-by: Ryan Liang <109499885+RyanL1997@users.noreply.github.com>
jakubp-eliatra pushed a commit to sebastianmichalski/opensearch-dashboards-functional-test that referenced this pull request Mar 24, 2023
Signed-off-by: Jakub Przybylski <jakub.przybylski@eliatra.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x enhancement New feature or request v2.4.0 'Issues and PRs related to version v2.4.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants