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.

Sign up for GitHub

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

Jump to bottom

[nodejs client] hookup js client with dashboards #1342

Merged
merged 1 commit into from
Mar 23, 2022
Merged

[nodejs client] hookup js client with dashboards #1342

merged 1 commit into from
Mar 23, 2022

Conversation

ananzh
Copy link
Member

@ananzh ananzh commented Mar 12, 2022 •
edited by kavilla
Loading

Description

This is a merged PR from feature/nodejs branch.
1.change @elastic/elasticsearch to @opensearch-project/opensearch
2.add path in tsconfig.base.json to let nodejs types point to api/new.d.ts
which is the new defined types
3.fix hookup issues in src/core/server/opensearch
4.fix hookup issues in src/core/server/core_usage_data_service.ts
5.fix hookup issues in src/core/server/saved_objects

Sub-Issue Resolved: #1193

Sub-Issue Resolved: #1224

Sub-Issue Resolved: #1216

Issue Resolved: #837

Check List

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

@ananzh ananzh requested a review from a team as a code owner March 12, 2022 03:56
@ananzh ananzh added nodejs 🍭 issues related to nodejs client v2.0.0 labels Mar 12, 2022
@ananzh ananzh self-assigned this Mar 12, 2022
tmarkley
tmarkley suggested changes Mar 14, 2022
View reviewed changes
Copy link
Contributor

@tmarkley tmarkley left a comment

Choose a reason for hiding this comment

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

This looks good Anan! I'd recommend adding descriptions for each ts-expect, or try to eliminate those exceptions if possible. Otherwise I added some comments about the types which may not require any action but I just wanted to check.

yarn.lock Outdated Show resolved Hide resolved
src/core/server/core_usage_data/core_usage_data_service.ts Show resolved Hide resolved
src/core/server/saved_objects/migrations/core/build_active_mappings.test.ts Outdated Show resolved Hide resolved
src/core/server/saved_objects/migrations/core/index_migrator.test.ts Show resolved Hide resolved
src/core/server/saved_objects/migrations/core/index_migrator.test.ts Outdated Show resolved Hide resolved
...server/saved_objects/migrations/opensearch_dashboards/opensearch_dashboards_migrator.test.ts Outdated Show resolved Hide resolved
...server/saved_objects/migrations/opensearch_dashboards/opensearch_dashboards_migrator.test.ts Outdated Show resolved Hide resolved
src/core/server/saved_objects/service/lib/repository.ts Show resolved Hide resolved
src/core/server/saved_objects/service/lib/repository.ts Show resolved Hide resolved
src/core/server/opensearch/version_check/ensure_opensearch_version.ts Show resolved Hide resolved
@kavilla
Copy link
Member

kavilla commented Mar 16, 2022 •
edited
Loading

Things I tested with this PR:

  • Basic smoke test (add sample data, filter, discover create viz) [SUCCESS]
  • Build release yarn build-platform --linux --skip-os-packages --release, extracted and ran the executable then ran the basic smoke test [SUCCESS]
  • Basic smoke test against OpenSearch 1.4 snapshot [SUCCESS]
  • Basic smoke test with Batch concurrent requests [SUCCESS]

Haven't tested, saving settings restarting or exporting saved objects or importing saved objects from version OSD 1.x. Could you verify these?

Also, I'm not positive if it was just my VM churning with the multiple processes I'm doing on it or my recent OpenSearch snapshot, but the loading of a dashboard seems a little bit slower than usual. Could you verify that it's just not my machine? If you do notice a difference in the load time from main to this branch can we get some metrics?

\m/ Awesome work! \m/

Thanks

kavilla
kavilla reviewed Mar 16, 2022
View reviewed changes
package.json Show resolved Hide resolved
@ananzh
[nodejs client] hookup js client with dashboards
9d187ce 
1.change @elastic/elasticsearch to @opensearch-project/opensearch
2.add path in tsconfig.base.json to let nodejs types point to api/new.d.ts
which is the new defined types
3.fix hookup issues in src/core/server/opensearch
4.fix hookup issues in src/core/server/core_usage_data_service.ts
5.fix hookup issues in src/core/server/saved_objects

fix PR comments and add more explains to type mismatch

Sub-Issue Resolved: opensearch-project#1193
Sub-Issue Resolved: opensearch-project#1224
Sub-Issue Resolved: opensearch-project#1216
Issue Resolved: opensearch-project#837

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
tmarkley
tmarkley approved these changes Mar 23, 2022
View reviewed changes
Copy link
Contributor

@tmarkley tmarkley left a comment

Choose a reason for hiding this comment

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

This looks great Anan, thanks!

src/core/server/saved_objects/migrations/core/index_migrator.ts Show resolved Hide resolved
kavilla
kavilla approved these changes Mar 23, 2022
View reviewed changes
Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

LGTM! Like I am to start and run, on my opinion that we should get this merged in and handle in any fall out if any.

@kavilla kavilla merged commit 6b66b11 into opensearch-project:main Mar 23, 2022
@tmarkley tmarkley mentioned this pull request Mar 23, 2022
Merged
7 tasks
@ananzh ananzh deleted the nodejs-client branch June 1, 2022 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmarkley tmarkley tmarkley approved these changes

@kavilla kavilla kavilla approved these changes

Assignees

@ananzh ananzh

Labels
nodejs 🍭 issues related to nodejs client v2.0.0
Projects
None yet
Milestone
No milestone
3 participants
@ananzh @kavilla @tmarkley