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

[Data source] Fix datasource filtering issue #5484

Merged
merged 3 commits into from
Nov 29, 2023

Conversation

mengweieric
Copy link
Collaborator

@mengweieric mengweieric commented Nov 16, 2023

Description

The filtering functionality of datasource picker is not working. This PR is for addressing this issue.

E2E tests: opensearch-project/opensearch-dashboards-functional-test#965

Issues Resolved

#5319
#5468

Screenshot

Before

Screenshot 2023-11-27 at 1 12 47 PM

After

Screenshot 2023-11-27 at 1 14 08 PM

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

Signed-off-by: Eric <menwe@amazon.com>
Copy link

codecov bot commented Nov 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f6ce6e7) 66.98% compared to head (4de6601) 66.94%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5484      +/-   ##
==========================================
- Coverage   66.98%   66.94%   -0.05%     
==========================================
  Files        3293     3293              
  Lines       63281    63281              
  Branches    10061    10061              
==========================================
- Hits        42389    42362      -27     
- Misses      18451    18456       +5     
- Partials     2441     2463      +22     
Flag Coverage Δ
Linux_1 35.24% <ø> (ø)
Linux_2 ?
Linux_3 ?
Linux_4 ?
Windows_1 35.27% <ø> (ø)
Windows_2 55.18% <ø> (ø)
Windows_3 43.80% <ø> (+<0.01%) ⬆️
Windows_4 35.33% <ø> (ø)

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.

@abbyhu2000
Copy link
Member

abbyhu2000 commented Nov 17, 2023

Thank you!

If you want to add cypress test, you could add it in this folder: cypress/integration/core-opensearch-dashboards/opensearch-dashboards/apps/data_explorer. I don't think there are any current cypress tests related to data source dropdown, so you could prob write those in a new file under the data_explorer folder.

@mengweieric
Copy link
Collaborator Author

Thank you!

If you want to add cypress test, you could add it in this folder: cypress/integration/core-opensearch-dashboards/opensearch-dashboards/apps/data_explorer. I don't think there are any current cypress tests related to data source dropdown, so you could prob write those in a new file under the data_explorer folder.

sure that is one of the to-dos for this PR

@@ -31,6 +31,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [BUG] Add platform "darwin-arm64" to unit test ([#5290](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5290))
- [BUG][Dev Tool] Add dev tool documentation link to dev tool's help menu [#5166](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5166)
- Fix missing border for header navigation control on right ([#5450](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5450))
- [BUG] Fix filtering issue in data source selector ([5484](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5484))
Copy link
Member

Choose a reason for hiding this comment

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

lgtm.

question for core team, why do we need to manually maintain changelog? can it be generated automatically with
https://github.com/tj/git-extras/blob/HEAD/Commands.md#git-changelog
or
https://github.com/orhun/git-cliff

Copy link
Member

Choose a reason for hiding this comment

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

I think it would make sense but not every commit message is clean enough for the changelog. So it would require clean commit messages.

We also have another POC in the process: #5519

In response to opensearch-project/.github#148

Copy link
Member

Choose a reason for hiding this comment

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

but not every commit message is clean enough

i think that should anyways be improved, one option is to enforce conventional commits. Then it becomes easy to auto generate changelog or changeset, also git becomes more useful. thanks for the references, if i have time i might raise something

Copy link
Member

Choose a reason for hiding this comment

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

@joshuali925 There was a whole big analysis of this problem - the working group decided that developer commit messages were not sufficient for representing changes to a non-dev audience.

Copy link
Member

Choose a reason for hiding this comment

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

@joshuarrrr I want to bring this conversation off from this PR. but I asked why not to automate it because after randomly clicking a few items in the change log, the wording for each item is pretty much the same as its PR title.

the working group decided that developer commit messages were not sufficient for representing changes to a non-dev audience

I agree putting commit messages together is not enough for a release note, but with changelog I still find it difficult to understand as a user. For example, looking at [Discover] Display inner properties in the left navigation bar under "Features/Enhancements" in the changelog, i have no idea what this does.

From https://keepachangelog.com/en/1.1.0/,

A changelog is a file which contains a curated, chronologically ordered list of notable changes for each version of a project.

A commit should represent a change, to me a list of notable commits sounds like the definition of a changelog. I think changelog or commit messages with release notes (summary and highlights) should be enough to represent changes to a non-dev audience (this is commonly done in other projects, i can give examples). My ask is mainly to avoid manually writing the changelog if possible

@mengweieric mengweieric changed the title Fix datasource filtering issue [Data source] Fix datasource filtering issue Nov 27, 2023
Copy link
Contributor

github-actions bot commented Nov 28, 2023

[MANUAL CYPRESS TEST RUN RESULTS]

❌ Cypress test run failed!

Inputs:

Test repo: 'mengweieric/opensearch-dashboards-functional-test'
Test branch: 'feature/add-test-for-datasource-selector'

Test spec:
''

Link to results:

https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/7023707975

opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 29, 2023
Fixes data source filtering issue within Discover. When typing into the index pattern
field it will filter out and highlight the value being typed.

Issue resolved:
#5533
#5499
#5468
#5319

Signed-off-by: Eric <menwe@amazon.com>
(cherry picked from commit dadfefa)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 29, 2023
Fixes data source filtering issue within Discover. When typing into the index pattern
field it will filter out and highlight the value being typed.

Issue resolved:
#5533
#5499
#5468
#5319

Signed-off-by: Eric <menwe@amazon.com>
(cherry picked from commit dadfefa)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
ashwin-pc pushed a commit that referenced this pull request Nov 30, 2023
Fixes data source filtering issue within Discover. When typing into the index pattern
field it will filter out and highlight the value being typed.

Issue resolved:
#5533
#5499
#5468
#5319


(cherry picked from commit dadfefa)

Signed-off-by: Eric <menwe@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>
ananzh pushed a commit that referenced this pull request Dec 13, 2023
Fixes data source filtering issue within Discover. When typing into the index pattern
field it will filter out and highlight the value being typed.

Issue resolved:
#5533
#5499
#5468
#5319


(cherry picked from commit dadfefa)

Signed-off-by: Eric <menwe@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>
@amirkkn
Copy link

amirkkn commented Jan 10, 2024

The fix is not still available in image 2.11.0 and 2.11.1. Is this fix released in 2.12? Currently we blocked to upgrade our production environments. We use official opensearch dashboard helm chart.

@ashwin-pc
Copy link
Member

Yeah unfortunately this fix was merged after the 2.11.1 release was cut. So this will either come in 2.12 or 2.11.2 (If 2.11.2 is released, which at this stage is unlikely)

@ghost
Copy link

ghost commented Jan 26, 2024

this fix is utterly needed 👯‍♀️

@Kluk
Copy link

Kluk commented Jan 29, 2024

this fix is utterly needed 👯‍♀️

I agree. It would make the application much more easy to use. A lot of users are complaining because of this bug.
Also, it appeared 3 months ago, it should have been fixed by now.
And it seems it won't be fixed in version 2.11, but only in 2.12. This is bad news for AWS OpenSearch users as we only get odd versions since version 1.3. This means we would have to wait for version 2.13 to get this finally fixed.

Is there nothing to do to get this fix ASAP?

@ananzh
Copy link
Member

ananzh commented Jan 29, 2024

@Kluk This PR has been backported to 2.11 #5554. It might take some time for AWS OpenSearch to pick up this bug fixes into a patch release from open source but it should not take too long.

@ghost
Copy link

ghost commented Jan 29, 2024

@ananzh Does that mean that the fix is available in the 2.11.1 docker image already? Or what does "backported to 2.11" mean in this case? Sorry for newbie question but I'm a newbie ^^

@Kluk
Copy link

Kluk commented Jan 30, 2024

@ananzh Does that mean that the fix is available in the 2.11.1 docker image already? Or what does "backported to 2.11" mean in this case? Sorry for newbie question but I'm a newbie ^^

I tested the Dashboards docker images 2.11.0 and 2.11.1. The fix is not there.
But the service software update OpenSearch_2_11_R20231113-P2 is available in AWS. This update contains the fix (or the bug was magically fixed another way).
I checked the Dashboards version after the update, it still displays 2.11.0. So, I don't understand how the patching works between the official releases and AWS'.
At least, it's fixed in AWS :)

@ghost
Copy link

ghost commented Jan 30, 2024

oh great, AWS got it running and the rest is left behind... Somehow, I'm not surprised. Hard, hard truth...

@ananzh
Copy link
Member

ananzh commented Jan 30, 2024

@Kluk thanks for checking this
@yoicomment Backported to 2.11 means we merge to fix to 2.11 branch even though it is released. Normally, we will backport security fixes or major fallback bugs if it impact users if there is no newer version (say before 2.12 is released).

@ghost
Copy link

ghost commented Jan 31, 2024

@Kluk thanks for checking this @yoicomment Backported to 2.11 means we merge to fix to 2.11 branch even though it is released. Normally, we will backport security fixes or major fallback bugs if it impact users if there is no newer version (say before 2.12 is released).

So, if I wanna use this fix and I'm using docker images I need to build my own docker image based on the 2.11 branch instead of getting from dockerhub, am I right?

@Kluk
Copy link

Kluk commented Jan 31, 2024

@Kluk thanks for checking this @yoicomment Backported to 2.11 means we merge to fix to 2.11 branch even though it is released. Normally, we will backport security fixes or major fallback bugs if it impact users if there is no newer version (say before 2.12 is released).

I also opened an AWS case to get more information from their side.

  • They confirmed their version is not the same as the public one.
  • It's not possible to get a docker image of the AWS version
  • As of today, there is no way to see differences between the public and the AWS version

So, if I wanna use this fix and I'm using docker images I need to build my own docker image based on the 2.11 branch instead of getting from dockerhub, am I right?

There is a Dockerfile in the 2.11 branch, so I guess you could. I didn't try it myself, but let me know if you do :)

@wbeckler
Copy link

wbeckler commented Feb 1, 2024

This is a bad enough bug to justify backporting to 2.11 so that future patch releases don't contain the bug

@ghost
Copy link

ghost commented Feb 16, 2024

still don't know how to use the fix. Do we have to wait until the next release or build our own docker image using the dockerfile?

@amirkkn
Copy link

amirkkn commented Feb 16, 2024

They promised to release the fix in 2.12. It should be the next days, hopefully. That is however very strange reaction from them so far. Such a terrible UI bug still exist in all their docker images. Very frustrating!

@ghost
Copy link

ghost commented Feb 16, 2024

I'm just wondering what bugs we have to deal with version 2.12

@ashwin-pc
Copy link
Member

still don't know how to use the fix. Do we have to wait until the next release or build our own docker image using the dockerfile?

@yoicomment firstly, I'm sorry that you are blocked by this issue. This fix will be a part of the 2.12 distribution in opensource and a part of the 2.11 distribution in AWS.

They promised to release the fix in 2.12. It should be the next days, hopefully. That is however very strange reaction from them so far. Such a terrible UI bug still exist in all their docker images. Very frustrating!

@amirkkn sorry for the confusion, but what was the strange reaction, hopefully I can answer why you felt that.

I'm just wondering what bugs we have to deal with version 2.12

Hopefully none! We have received a lot of feedback from the 2.10 and 2.11 releases of discover and the team has spent the entire release cycle only addressing all the bugs and concerns from the previous releases. We have also made changes to our processes on how we respond to issues so that we catch them sooner and address them. Not making any promises but I do think you will like the latest release.

oh great, AWS got it running and the rest is left behind... Somehow, I'm not surprised. Hard, hard truth...

I understand how this can seem like the case but it couldn't be further from the truth. As @Kluk called out earlier, AWS currently only picks up odd versions of the release and since 2.12 isn't one of them, users of AWS would have to wait until April to get this fix if we didn't backport it there. As for opensource, since the 2.12 release is already underway, it didn't make sense to dedicate a lot more time to patch just this since they can pick up the 2.12 release which has a lot more changes than this and will be out sooner than a patch to 2.11 will be. Also there are a lot more fixes than just this to discover that were made based on feedback from users of OSD like yourself, so the team dedicated the time to fix those instead of patching individual bugs.

As for why we cant release a fix sooner after it's been merged, that has to do with our release process that takes significant effort and coordination between teams to do, so creating ones for each fix will be impractical. We are working on improving our processes so that things like this don't slip through the cracks, but I'll be the first to admit, we aren't there yet.

Hopefully this was useful and I appreciate the feedback!

@amirkkn
Copy link

amirkkn commented Feb 17, 2024

still don't know how to use the fix. Do we have to wait until the next release or build our own docker image using the dockerfile?

@yoicomment firstly, I'm sorry that you are blocked by this issue. This fix will be a part of the 2.12 distribution in opensource and a part of the 2.11 distribution in AWS.

They promised to release the fix in 2.12. It should be the next days, hopefully. That is however very strange reaction from them so far. Such a terrible UI bug still exist in all their docker images. Very frustrating!

@amirkkn sorry for the confusion, but what was the strange reaction, hopefully I can answer why you felt that.

I'm just wondering what bugs we have to deal with version 2.12

Hopefully none! We have received a lot of feedback from the 2.10 and 2.11 releases of discover and the team has spent the entire release cycle only addressing all the bugs and concerns from the previous releases. We have also made changes to our processes on how we respond to issues so that we catch them sooner and address them. Not making any promises but I do think you will like the latest release.

oh great, AWS got it running and the rest is left behind... Somehow, I'm not surprised. Hard, hard truth...

I understand how this can seem like the case but it couldn't be further from the truth. As @Kluk called out earlier, AWS currently only picks up odd versions of the release and since 2.12 isn't one of them, users of AWS would have to wait until April to get this fix if we didn't backport it there. As for opensource, since the 2.12 release is already underway, it didn't make sense to dedicate a lot more time to patch just this since they can pick up the 2.12 release which has a lot more changes than this and will be out sooner than a patch to 2.11 will be. Also there are a lot more fixes than just this to discover that were made based on feedback from users of OSD like yourself, so the team dedicated the time to fix those instead of patching individual bugs.

As for why we cant release a fix sooner after it's been merged, that has to do with our release process that takes significant effort and coordination between teams to do, so creating ones for each fix will be impractical. We are working on improving our processes so that things like this don't slip through the cracks, but I'll be the first to admit, we aren't there yet.

Hopefully this was useful and I appreciate the feedback!

Thanks a lot for your clarifications. What I meant is because the fix was merged and backported 3-4 months ago, but not in docker images. But I can understand that the team needs a lot of alignments for a single release. Please don't get me wrong, opensearch is always my favourite, and i got always support by opensearch team for any issues. I'm looking forward to the 2.12 release 😀.

@ghost
Copy link

ghost commented Feb 17, 2024

#5319 was created in 18th Oct and 2.11.1 was released 30th Nov.
A lot of time to fix an issue like this.
Nevertheless, we are thankful to your service. Let's work together to make this product great 👍

@ghost
Copy link

ghost commented Feb 21, 2024

@ashwin-pc will the 2.11.1 or 2.11.0 docker images rebuild with the backported fixes or do we have to use version 2.12.0 if we want to have the bug fixes?

@wbeckler
Copy link

wbeckler commented Feb 21, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet