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 integration test for workspace saved objects client wrapper #206

Conversation

wanglam
Copy link
Collaborator

@wanglam wanglam commented Sep 28, 2023

Description

  1. Fix permission validate for addToWorkspaces and deleteByWorkspace in client wrapper
  2. Add integration tests for workspace saved object client wrapper

Issues Resolved

Screenshot

Testing the changes

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Signed-off-by: Lin Wang <wonglam@amazon.com>
@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2023

Codecov Report

Merging #206 (354d1af) into workspace (29ccc20) will increase coverage by 35.79%.
Report is 3 commits behind head on workspace.
The diff coverage is n/a.

@@              Coverage Diff               @@
##           workspace     #206       +/-   ##
==============================================
+ Coverage      30.42%   66.21%   +35.79%     
==============================================
  Files           2199     3420     +1221     
  Lines          45154    65755    +20601     
  Branches        7023    10589     +3566     
==============================================
+ Hits           13737    43541    +29804     
+ Misses         30810    19571    -11239     
- Partials         607     2643     +2036     
Flag Coverage Δ
Linux_1 30.42% <ø> (-0.01%) ⬇️
Linux_2 55.38% <ø> (?)
Linux_3 42.76% <ø> (?)
Linux_4 34.51% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 2212 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: Lin Wang <wonglam@amazon.com>
…ear all

Signed-off-by: Lin Wang <wonglam@amazon.com>
Signed-off-by: Lin Wang <wonglam@amazon.com>
@wanglam wanglam marked this pull request as ready for review September 28, 2023 14:01
@@ -145,7 +145,7 @@ function getClauseForWorkspace(workspace: string) {

return {
bool: {
must: [{ term: { workspaces: workspace } }],
must: [{ term: { 'workspaces.keyword': workspace } }],
Copy link
Collaborator

Choose a reason for hiding this comment

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

the type of workspaces is keyword, do we need keyword here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The integration tests in linux will be failed, if we don't add the .keyword suffix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can create .kibana index first with pre-defined mappings, otherwise openSearch will set the type automatically. the default type for string is text with sub field keyword, that's the case you have mentioned.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there anyway to define this type in the saved objects side? It seems the workspaces field will be text type if mapping not defined. It could be a bug because we can't create .kibana index with pre-defined mapping when create saved objects in the real server.

Copy link
Collaborator

Choose a reason for hiding this comment

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

how about doing this in beforeAll method, you can replace mappings with real .kibana mappings

 await opensearchServer.opensearch.getClient().indices.create({
    body: { mappings, settings },
    index,
  });

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This only worked for integration tests right? Will we have the same problems in the real case?

Copy link
Collaborator

@Hailong-am Hailong-am Oct 9, 2023

Choose a reason for hiding this comment

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

No, IndexMigrator will create .kibana with expected mapping when OSD startup. Test server disabled the migration with https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/core/test_helpers/osd_server.ts#L63.

with this, you may override the setting for test server to enable migration .

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, feel free to help me re-review it.

Signed-off-by: Lin Wang <wonglam@amazon.com>
@wanglam wanglam requested a review from Hailong-am October 9, 2023 06:40
Signed-off-by: Lin Wang <wonglam@amazon.com>
@wanglam wanglam merged commit 8505341 into ruanyl:workspace Oct 9, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants