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

[OSCI] Fix type errors in Multiple Data Sources #5546

Closed
wants to merge 27 commits into from

Conversation

CMDWillYang
Copy link
Contributor

@CMDWillYang CMDWillYang commented Nov 28, 2023

Description

Fixed type errors found in several data source files:

  • 11 errors in data_source_saved_objects_client_wrapper.ts:
  • 4 errors in src/plugins/data_source/server/legacy/configure_legacy_client.ts
  • 4 errors in src/plugins/data_source/server/routes/test_connection.ts
  • 9 errors in src/plugins/data_source_management/public/components/create_data_source_wizard/components/create_form/create_data_source_form.tsx
  • 6 errors in src/plugins/data_source_management/public/components/edit_data_source/components/edit_form/edit_data_source_form.test.tsx
  • 16 errors in src/plugins/data_source_management/public/components/edit_data_source/components/edit_form/edit_data_source_form.tsx

Issues Resolved

Fixes several errors outlined in issue #3026

Screenshot

https://docs.google.com/document/d/1EUXvMTZCUQYPzHRl2fgUGFCgc9KWJUH0p82zaR9UpQI/edit?usp=sharing

Testing the changes

Check List

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

Copy link

codecov bot commented Nov 28, 2023

Codecov Report

Attention: Patch coverage is 33.33333% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 67.02%. Comparing base (a5c45a3) to head (1c0f444).
Report is 106 commits behind head on main.

Files Patch % Lines
...rce/components/edit_form/edit_data_source_form.tsx 30.00% 4 Missing and 3 partials ⚠️
...components/create_form/create_data_source_form.tsx 28.57% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5546      +/-   ##
==========================================
+ Coverage   66.98%   67.02%   +0.03%     
==========================================
  Files        3294     3294              
  Lines       63296    63308      +12     
  Branches    10066    10075       +9     
==========================================
+ Hits        42398    42430      +32     
+ Misses      18500    18432      -68     
- Partials     2398     2446      +48     
Flag Coverage Δ
Linux_1 35.24% <ø> (-0.01%) ⬇️
Linux_2 55.19% <ø> (?)
Linux_4 35.34% <ø> (-0.01%) ⬇️
Windows_1 35.26% <ø> (-0.01%) ⬇️
Windows_2 55.15% <ø> (+0.01%) ⬆️
Windows_3 43.88% <33.33%> (-0.02%) ⬇️
Windows_4 35.34% <ø> (-0.01%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ruanyl ruanyl left a comment

Choose a reason for hiding this comment

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

Thank you for your patient to continuously improve this PR, I left a few more comment 👍

const client = new LegacyClient({
connectionClass: HttpAmazonESConnector,
awsConfig: new Config({
region,
credentials: new Credentials({ accessKeyId: accessKey, secretAccessKey: secretKey }),
}),
service,

Copy link
Member

Choose a reason for hiding this comment

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

I understand that service is NOT a known property of ConfigOptions, but maybe you want to make sure that service is not needed and can be safely removed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it is not needed. The only usage of getAWSClient in the repo is line 120. const awsClient is used in addClientToPool and callAPI.bind(null, awsClient) which getQueryClient then returns. None of them should use service to my knowledge/investigation, and also type legacyClient / class Client does not have this service property so that should be a good indication.

const awsClient = rootClient ? rootClient : getAWSClient(awsCredential, clientOptions);

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification!

Copy link
Contributor Author

@CMDWillYang CMDWillYang Dec 20, 2023

Choose a reason for hiding this comment

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

I did notice, however, that there is a test expecting service to still be included.
I have yet to figure out why this might be needed. still researching.

test('configure client with auth.type == sigv4 and service param, should call new Client() with service param', async () => {
savedObjectsMock.get.mockReset().mockResolvedValueOnce({
id: DATA_SOURCE_ID,
type: DATA_SOURCE_SAVED_OBJECT_TYPE,
attributes: {
...dataSourceAttr,
auth: {
type: AuthType.SigV4,
credentials: { ...sigV4AuthContent, service: 'aoss' },
},
},
references: [],
});

@ashwin-pc
Copy link
Member

Closing this since this is quite old and a lot has changed since. Thanks for the PR @CMDWillYang

@ashwin-pc ashwin-pc closed this Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x first-time-contributor technical debt If not paid, jeapardizes long-term success and maintainability of the repository. valued effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants