-
Notifications
You must be signed in to change notification settings - Fork 968
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
[MDS] Fix showing DQS sources list in workspaces #8838
Conversation
Signed-off-by: jowg-amazon <jowg@amazon.com>
ℹ️ Manual Changeset Creation ReminderPlease ensure manual commit for changeset file 8838.yml under folder changelogs/fragments to complete this PR. If you want to use the available OpenSearch Changeset Bot App to avoid manual creation of changeset file you can install it in your forked repository following this link. For more information about formatting of changeset files, please visit OpenSearch Auto Changeset and Release Notes Tool. |
Signed-off-by: jowg-amazon <jowg@amazon.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8838 +/- ##
=======================================
Coverage 60.85% 60.86%
=======================================
Files 3808 3808
Lines 91159 91175 +16
Branches 14393 14396 +3
=======================================
+ Hits 55478 55493 +15
- Misses 32139 32141 +2
+ Partials 3542 3541 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
...urces_components/direct_query_data_connection/manage_direct_query_data_connections_table.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Joanne Wang <jowg@amazon.com>
Signed-off-by: Joanne Wang <jowg@amazon.com>
Signed-off-by: Joanne Wang <jowg@amazon.com>
type: | ||
{ | ||
S3GLUE: DatasourceTypeToDisplayName.S3GLUE, | ||
PROMETHEUS: DatasourceTypeToDisplayName.PROMETHEUS, | ||
}[dataConnection.connector] || dataConnection.connector, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I understand this part of the code is barely being changed in this PR, pushing for higher maintainability standards would be to ask for these to be moved to a proper definition of their own as known connectors. This is especially important because this object is used in multiple places.
setData([...openSearchConnections, ...directQueryConnections]); | ||
} catch (error) { | ||
// eslint-disable-next-line no-console | ||
console.log('Error fetching OpenSearch and Direct Query Connections', error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Should we remove this? The user won't benefit from this. Do we want to show the notification toast instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added notification toast
) { | ||
// TODO: link to details page for security lake and cloudwatch | ||
return <span style={indentStyle}> {name}</span>; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Don't need else
here since we only return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the else
Approving with minor comments |
Signed-off-by: Joanne Wang <jowg@amazon.com>
* Fix showing DQS sources list in workspaces Signed-off-by: jowg-amazon <jowg@amazon.com> * add changelog Signed-off-by: jowg-amazon <jowg@amazon.com> * Changeset file for PR #8838 created/updated * respond to comments Signed-off-by: Joanne Wang <jowg@amazon.com> * change to use DatasourceTypeToDisplayName Signed-off-by: Joanne Wang <jowg@amazon.com> * update tests Signed-off-by: Joanne Wang <jowg@amazon.com> * respond to comments Signed-off-by: Joanne Wang <jowg@amazon.com> --------- Signed-off-by: jowg-amazon <jowg@amazon.com> Signed-off-by: Joanne Wang <jowg@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit f85cd6d) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* Fix showing DQS sources list in workspaces * add changelog * Changeset file for PR #8838 created/updated * respond to comments * change to use DatasourceTypeToDisplayName * update tests * respond to comments --------- (cherry picked from commit f85cd6d) Signed-off-by: jowg-amazon <jowg@amazon.com> Signed-off-by: Joanne Wang <jowg@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Description
Issues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration