[Discover] Update Version Filter for Data Sources #8785

sejli · 2024-11-01T07:04:06Z

Description

Uses semver to filter out data sources with invalid versions or versions that are lower than 1.0.0.
Adding tests for index_type.ts

Issues Resolved

Screenshot

Testing the changes

Changelog

skip

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

Signed-off-by: Sean Li <[email protected]>

codecov · 2024-11-01T07:18:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.76%. Comparing base (92e0766) to head (3966cee).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8785      +/-   ##
==========================================
+ Coverage   60.75%   60.76%   +0.01%     
==========================================
  Files        3798     3798              
  Lines       90690    90692       +2     
  Branches    14277    14277              
==========================================
+ Hits        55101    55112      +11     
+ Misses      32090    32081       -9     
  Partials     3499     3499

Flag	Coverage Δ
Linux_1	`29.05% <0.00%> (-0.01%)`	⬇️
Linux_2	`56.39% <ø> (ø)`
Linux_3	`37.62% <100.00%> (+0.02%)`	⬆️
Linux_4	`29.82% <50.00%> (+<0.01%)`	⬆️
Windows_1	`29.07% <0.00%> (-0.01%)`	⬇️
Windows_2	`56.34% <ø> (ø)`
Windows_3	`37.62% <100.00%> (+0.01%)`	⬆️
Windows_4	`29.82% <50.00%> (+<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.

Hailong-am · 2024-11-01T10:09:15Z

src/plugins/query_enhancements/public/datasets/s3_type.ts

+    .filter((savedObject) => {
+      const coercedVersion = semver.coerce(savedObject.attributes.dataSourceVersion);
+      return coercedVersion ? semver.satisfies(coercedVersion, '>=1.0.0') : false;
+    })


should we use semver.vaild(savedObject.attributes.dataSourceVersion) to check datsource version is valid first and then do satisfies check?

+1, If semver.coerce returns null, this will lead to semver.satisfies(null, '>=x.x.x'), could cause an issue? You may want to add a null check before using semver.satisfies, as semver.satisfies doesn't handle the null

Doesn't the truthy check before satisfies handle your concern?

Yes. truthy check should handle the concerns about null and undefined, should we validate the datasource version is valid? Thanks

It's my understanding that if the dataSourceVersion is not valid in our situation, then semver.coerce() should return null.

Only text which lacks digits will fail coercion

dataSourceVersion is "invalid" when it is an empty string and gets caught by the truthy check.

Hailong-am · 2024-11-01T10:10:22Z

src/plugins/query_enhancements/public/datasets/s3_type.test.ts

+    it('should filter out data sources with versions lower than 1.0.0', async () => {
      mockSavedObjectsClient.find = jest.fn().mockResolvedValue({
        savedObjects: [
-          { id: 'ds1', attributes: { title: 'DataSource 1', dataSourceEngineType: 'OpenSearch' } },
+          { id: 'ds1', attributes: { title: 'DataSource 1', dataSourceVersion: '1.0' } },
          {
            id: 'ds2',
-            attributes: { title: 'DataSource 2', dataSourceEngineType: 'OpenSearch Serverless' },
+            attributes: { title: 'DataSource 2', dataSourceVersion: '' },
+          },
+          { id: 'ds3', attributes: { title: 'DataSource 3', dataSourceVersion: '2.17.0' } },
+          {
+            id: 'ds4',
+            attributes: { title: 'DataSource 4', dataSourceVersion: '.0' },
          },


can we add some test to check invalid semver version like a.b.c and semver version with snapshot/alpha (3.0.0-snapshot) etc?

What happens on an invalid version? I assume coerce is just not touching it and it fails satisfies? Data source 2 is an example of an invalid version, but the truthy check on coercedVersion handles that case (I assume).

Docs looks like coerce should handle second case so don't think that's a big issue.

Confirmed an invalid version returns null locally, so lgtm

virajsanghvi

just blocking approval on the question on invalid version

virajsanghvi · 2024-11-01T15:48:40Z

src/plugins/query_enhancements/public/datasets/s3_type.test.ts

+    it('should filter out data sources with versions lower than 1.0.0', async () => {
      mockSavedObjectsClient.find = jest.fn().mockResolvedValue({
        savedObjects: [
-          { id: 'ds1', attributes: { title: 'DataSource 1', dataSourceEngineType: 'OpenSearch' } },
+          { id: 'ds1', attributes: { title: 'DataSource 1', dataSourceVersion: '1.0' } },
          {
            id: 'ds2',
-            attributes: { title: 'DataSource 2', dataSourceEngineType: 'OpenSearch Serverless' },
+            attributes: { title: 'DataSource 2', dataSourceVersion: '' },
+          },
+          { id: 'ds3', attributes: { title: 'DataSource 3', dataSourceVersion: '2.17.0' } },
+          {
+            id: 'ds4',
+            attributes: { title: 'DataSource 4', dataSourceVersion: '.0' },
          },


What happens on an invalid version? I assume coerce is just not touching it and it fails satisfies? Data source 2 is an example of an invalid version, but the truthy check on coercedVersion handles that case (I assume).

Docs looks like coerce should handle second case so don't think that's a big issue.

virajsanghvi · 2024-11-01T15:51:54Z

src/plugins/data/public/query/query_string/dataset_service/lib/index_type.ts

-        !savedObject.attributes?.dataSourceEngineType?.includes('OpenSearch Serverless')
-    )
+    .filter((savedObject) => {
+      const coercedVersion = semver.coerce(savedObject.attributes.dataSourceVersion);


This attribute always exists?

yeah @Flyingliuhub @ZilongX confirming this is the correct attribute?

We should get cypress tests once i merge the workflow in #8703

@virajsanghvi should do a scan on the usage of this property? I see a number of references to dataSourceEngineType in main right now

yes dataSourceVersion would always exist, it may carry empty value for invalid datasource but the filed itself would be there

virajsanghvi · 2024-11-01T15:53:57Z

src/plugins/data/public/query/query_string/dataset_service/lib/index_type.test.ts

+    expect(result.children).toHaveLength(2);
+    expect(result.children?.[0].title).toBe('DataSource 1');
+    expect(result.children?.[1].title).toBe('DataSource 3');
+    expect(result.children?.some((child) => child.title === 'DataSource 2')).toBe(false);


nit: not sure how valuable this is

virajsanghvi · 2024-11-01T16:24:16Z

src/plugins/query_enhancements/public/datasets/s3_type.test.ts

+    it('should filter out data sources with versions lower than 1.0.0', async () => {
      mockSavedObjectsClient.find = jest.fn().mockResolvedValue({
        savedObjects: [
-          { id: 'ds1', attributes: { title: 'DataSource 1', dataSourceEngineType: 'OpenSearch' } },
+          { id: 'ds1', attributes: { title: 'DataSource 1', dataSourceVersion: '1.0' } },
          {
            id: 'ds2',
-            attributes: { title: 'DataSource 2', dataSourceEngineType: 'OpenSearch Serverless' },
+            attributes: { title: 'DataSource 2', dataSourceVersion: '' },
+          },
+          { id: 'ds3', attributes: { title: 'DataSource 3', dataSourceVersion: '2.17.0' } },
+          {
+            id: 'ds4',
+            attributes: { title: 'DataSource 4', dataSourceVersion: '.0' },
          },


Confirmed an invalid version returns null locally, so lgtm

kavilla · 2024-11-01T17:03:24Z

src/plugins/data/public/query/query_string/dataset_service/lib/index_type.ts

-    )
+    .filter((savedObject) => {
+      const coercedVersion = semver.coerce(savedObject.attributes.dataSourceVersion);
+      return coercedVersion ? semver.satisfies(coercedVersion, '>=1.0.0') : false;


ah i see this what we meant with putting version. if this is the case would we be able to go with the original suggestion of adding an optional property of the dataset type ?

kavilla · 2024-11-02T02:07:42Z

Merging as validated it is working but we should revisit this

opensearch-trigger-bot · 2024-11-02T02:08:17Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-8785-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 9f7f8aa4c39b0710374438daf7066a11327e4452
# Push it to GitHub
git push --set-upstream origin backport/backport-8785-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-8785-to-2.x.

opensearch-trigger-bot · 2024-11-02T02:08:26Z

The backport to 2.18 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.18 2.18
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.18
# Create a new branch
git switch --create backport/backport-8785-to-2.18
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 9f7f8aa4c39b0710374438daf7066a11327e4452
# Push it to GitHub
git push --set-upstream origin backport/backport-8785-to-2.18
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.18

Then, create a pull request where the base branch is 2.18 and the compare/head branch is backport/backport-8785-to-2.18.

use semver for filtering data sources

3966cee

Signed-off-by: Sean Li <[email protected]>

sejli added backport 2.x discover for discover reinvent labels Nov 1, 2024

github-actions bot added seasoned-contributor Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry labels Nov 1, 2024

Hailong-am reviewed Nov 1, 2024

View reviewed changes

virajsanghvi reviewed Nov 1, 2024

View reviewed changes

virajsanghvi approved these changes Nov 1, 2024

View reviewed changes

Flyingliuhub approved these changes Nov 1, 2024

View reviewed changes

kavilla reviewed Nov 1, 2024

View reviewed changes

kavilla approved these changes Nov 1, 2024

View reviewed changes

ZilongX approved these changes Nov 1, 2024

View reviewed changes

kavilla merged commit 9f7f8aa into opensearch-project:main Nov 2, 2024
74 checks passed

kavilla added backport 2.18 v2.18.0 labels Nov 2, 2024

opensearch-trigger-bot bot added the failed backport label Nov 2, 2024

ashwin-pc added v2.19.0 and removed backport 2.18 v2.18.0 labels Nov 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Update Version Filter for Data Sources #8785

[Discover] Update Version Filter for Data Sources #8785

sejli commented Nov 1, 2024

codecov bot commented Nov 1, 2024 •

edited

Loading

Hailong-am Nov 1, 2024

Flyingliuhub Nov 1, 2024

virajsanghvi Nov 1, 2024

Flyingliuhub Nov 1, 2024

sejli Nov 1, 2024

Hailong-am Nov 1, 2024 •

edited

Loading

virajsanghvi Nov 1, 2024

virajsanghvi Nov 1, 2024

virajsanghvi left a comment

virajsanghvi Nov 1, 2024

virajsanghvi Nov 1, 2024

kavilla Nov 1, 2024 •

edited

Loading

ZilongX Nov 1, 2024

virajsanghvi Nov 1, 2024

virajsanghvi Nov 1, 2024

kavilla Nov 1, 2024

kavilla commented Nov 2, 2024

opensearch-trigger-bot bot commented Nov 2, 2024

opensearch-trigger-bot bot commented Nov 2, 2024

[Discover] Update Version Filter for Data Sources #8785

[Discover] Update Version Filter for Data Sources #8785

Conversation

sejli commented Nov 1, 2024

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

codecov bot commented Nov 1, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hailong-am Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virajsanghvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kavilla Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kavilla commented Nov 2, 2024

opensearch-trigger-bot bot commented Nov 2, 2024

opensearch-trigger-bot bot commented Nov 2, 2024

codecov bot commented Nov 1, 2024 •

edited

Loading

Hailong-am Nov 1, 2024 •

edited

Loading

kavilla Nov 1, 2024 •

edited

Loading