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

[Feature] Data sources associated objects tab #1470

Merged
merged 8 commits into from
Feb 27, 2024

Conversation

RyanL1997
Copy link
Collaborator

@RyanL1997 RyanL1997 commented Feb 26, 2024

Description

Add Data sources associated objects tab

Issues Resolved

[List any issues this PR will resolve]

Demo

Without data:
Screenshot 2024-02-26 at 4 18 47 PM

With data:
Screenshot 2024-02-26 at 4 19 40 PM

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • 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.

Signed-off-by: Ryan Liang <jiallian@amazon.com>
Signed-off-by: Ryan Liang <jiallian@amazon.com>
Signed-off-by: Ryan Liang <jiallian@amazon.com>
opensearch_dashboards.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
<EuiInMemoryTable
items={associatedObjects}
columns={columns}
search={search}
Copy link
Member

Choose a reason for hiding this comment

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

May need to move this out for an external search bar component since the EuiInMemoryTable doesn't have support for search bar and filters, but that can be done in a future PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lets do this as a follow up

@sejli sejli added the enhancement New feature or request label Feb 26, 2024
@ps48
Copy link
Member

ps48 commented Feb 26, 2024

@RyanL1997 can you please fix linting and test apart from the above comments. Thanks!

@sejli
Copy link
Member

sejli commented Feb 26, 2024

In order to fix test and build workflows, yarn test -u should be run to update the snapshots. That should fix those workflows.

Signed-off-by: Ryan Liang <jiallian@amazon.com>
Signed-off-by: Ryan Liang <jiallian@amazon.com>
Signed-off-by: Ryan Liang <jiallian@amazon.com>
Signed-off-by: Ryan Liang <jiallian@amazon.com>
Copy link

codecov bot commented Feb 26, 2024

Codecov Report

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

Project coverage is 56.38%. Comparing base (403eece) to head (5004ee8).

Files Patch % Lines
...urces/components/manage/associated_objects_tab.tsx 4.00% 24 Missing ⚠️
...ts/manage/accelerations_recommendation_callout.tsx 50.00% 1 Missing ⚠️
.../datasources/components/manage/data_connection.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1470      +/-   ##
==========================================
- Coverage   56.49%   56.38%   -0.12%     
==========================================
  Files         320      322       +2     
  Lines       11739    11766      +27     
  Branches     3055     3059       +4     
==========================================
+ Hits         6632     6634       +2     
- Misses       5053     5078      +25     
  Partials       54       54              
Flag Coverage Δ
dashboards-observability 56.38% <7.14%> (-0.12%) ⬇️

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

@ps48 ps48 left a comment

Choose a reason for hiding this comment

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

@RyanL1997 For the below lint error let's add a console.error to log the error whenever it occurs.

136:15  error    'err' is defined but never used. Allowed unused args must match /^_/u   

Thanks! for quick the updates on the PR

Signed-off-by: Ryan Liang <jiallian@amazon.com>
@RyanL1997
Copy link
Collaborator Author

@ps48 @sejli , I have addressed the comments above and also fixed that lint ( that was irrelevant from this change btw. ). And I'm updating the screenshots to the description.

database: string;
type: string;
createdByIntegration: string;
accelerations: string;
Copy link
Member

Choose a reason for hiding this comment

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

would any of these fields be a smaller type than string, or all of them can be arbitrary strings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think for now they should be all strings.

<AccelerationsRecommendationCallout />
<EuiSpacer />
{associatedObjects.length > 0 ? (
<EuiInMemoryTable
Copy link
Member

Choose a reason for hiding this comment

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

would there be a case that associatedObjects gets large and doesn't fit in memory?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, I think for the memory fitting issue, we may need to do something like virtualization with react-window to limit the size of the list. However, since this this is a skeleton for now, and lets just leave like this. I will test it when we connect the actual data content, so that we can decide which approaches we need for addressing the memory issue. I will not resolve this comment btw until we have a concrete solution on this.

name: string;
database: string;
type: string;
createdByIntegration: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if createdByIntegration is a field that AssociatedObject would contain, I'd expect this to be queried from the integrations that store all the contained object information. Do you see us modifying Integrations to add fields like this to arbitrary objects? If so, it might be worth adding this requirement in #1442

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Transferring some of the discussion over here: for now, since we should able to have this field with the source of integration, lets have this field for now for this skeleton PR. We can sync up on this again when we actually do the connection of actual data content.

@joshuali925 joshuali925 merged commit 187b112 into opensearch-project:main Feb 27, 2024
14 of 21 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 27, 2024
Signed-off-by: Ryan Liang <jiallian@amazon.com>
(cherry picked from commit 187b112)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
RyanL1997 added a commit to RyanL1997/dashboards-observability that referenced this pull request Mar 5, 2024
Signed-off-by: Ryan Liang <jiallian@amazon.com>
ps48 pushed a commit that referenced this pull request Mar 5, 2024
Signed-off-by: Ryan Liang <jiallian@amazon.com>
amsiglan pushed a commit to amsiglan/dashboards-observability that referenced this pull request Jun 7, 2024
… (opensearch-project#1486)

Signed-off-by: Ryan Liang <jiallian@amazon.com>
(cherry picked from commit 7eaa762)
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants