[Chore] Update timeline default expression #2720

joshuarrrr · 2022-11-01T21:04:21Z

Description

Update default expression from .es(*) to opensearch(*) in the timeline visualization. Both work, but we'd prefer the latter

Issues Resolved

#1631

Check List

kavilla · 2022-11-01T21:11:00Z

Thank you so much for doing this!

zhongnansu

plz add change log
I ran a string search in code base and found more places using .es() than the changes you made in this PR. I haven't looked into details. Do you think those defaults need to be refactored as well?
- OpenSearch-Dashboards/src/plugins/timeline/public/services/_saved_sheet.ts
  
  Line 78 in 0c9ca96
  
  timelion_sheet: ['.es(*)'],

kavilla · 2022-11-02T09:26:55Z

plz add change log

I ran a string search in code base and found more places using .es() than the changes you made in this PR. I haven't looked into details. Do you think those defaults need to be refactored as well?

OpenSearch-Dashboards/src/plugins/timeline/public/services/_saved_sheet.ts

Line 78 in 0c9ca96

timelion_sheet: ['.es(*)'],

for the cypress ones i would leave the same*, those are BWC tests so it's there to ensure that .es(*) still works.

kavilla · 2022-11-02T09:30:47Z

Could we also add to this script to make the query .es https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/cypress/integration/without-security/helpers/generate_data.js#L59 (and do the with-security script as well? Sorry for the redundancy, I followed the format of the previously BWC tests for consistency but we should fix that).

But if we can add to the section to ensure that it updates the value to be .es because the BWC test is to verify end-users to migrated from the legacy application to OpenSearch Dashboards that their queries still work instead of having to change to .opensearch

ananzh · 2022-11-03T07:14:34Z

I am a bit curious. Won't this cause backward compatibility issue? For example, if an saved expression is using .es(). So is this fix for 3.0.0?

kavilla · 2022-11-03T17:12:40Z

I am a bit curious. Won't this cause backward compatibility issue? For example, if an saved expression is using .es(). So is this fix for 3.0.0?

It should only cause a failure in the test after generation existing timeline visualizations should still contain .es()

Update default expression from `.es(*)` to `opensearch(*)`. Both work, but we'd prefer the latter Signed-off-by: Josh Romero <[email protected]>

Signed-off-by: Josh Romero <[email protected]>

To make sure we continue supporting the old format. Signed-off-by: Josh Romero <[email protected]>

Signed-off-by: Josh Romero <[email protected]>

joshuarrrr · 2022-11-18T23:42:47Z

Thanks @zhongnansu - I went ahead and updated all the other references, except for those in the BWC cypress tests as per @kavilla's note.

ashwin-pc · 2022-11-30T01:54:50Z

@joshuarrrr @kavilla is there anything pending for this PR?

kavilla

Timeline BWC test is passing locally for me

joshuarrrr · 2022-12-07T01:08:08Z

Timeline BWC test is passing locally for me

huh... that's surprising. Maybe I'll try re-running and hoping. 🤞

joshuarrrr · 2022-12-14T21:45:47Z

converting to draft until I have some time to investigate/fix bwc tests

codecov-commenter · 2022-12-28T22:07:03Z

Codecov Report

Merging #2720 (6401ff4) into main (1e127b0) will decrease coverage by 0.02%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #2720      +/-   ##
==========================================
- Coverage   66.41%   66.39%   -0.02%     
==========================================
  Files        3206     3209       +3     
  Lines       61597    61633      +36     
  Branches     9503     9507       +4     
==========================================
+ Hits        40909    40921      +12     
- Misses      18410    18426      +16     
- Partials     2278     2286       +8

Flag	Coverage Δ
Linux	`66.39% <ø> (-0.02%)`	⬇️

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

see 38 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Josh Romero <[email protected]>

- wait for loader to finish after clicking update Signed-off-by: Josh Romero <[email protected]>

ananzh · 2023-03-21T19:08:53Z

cypress/integration/with-security/helpers/generate_data.js

@@ -79,6 +79,10 @@ describe('Generating BWC test data with security', () => {
      .find('[data-test-subj="newItemButton"]')
      .click();
    cy.get('[data-test-subj="visType-timelion"]').click();
+    // update default expression to use `.es(*)` instead of `.opensearch(*)` for bwc


NIT: do you think we could create a common function

Cypress.Commands.add('clearWithBackspaces', (selector, count) => { cy.get(selector).then($element => { const backspaces = '{backspace}'.repeat(count); $element.type(`{selectAll}${backspaces}`); }); });

So this could simplifies to

cy.clearWithBackspaces('[class="view-line"]', 15).type('.es(*)');

and this function could also be imported in other tests. I see there are 2 more tests using similar function. maybe put it in a until or common folder?

Also curious why we need these many backspaces

Yeah, I meant to call this out in a comment. For some reason, the {selectAll} cypress sequence doesn't work in this field the way it's supposed to (or at least the way I understand that it's supposed to work). Previously, when the default expression was .es(*), there were 6 total characters - you'll see that some tests "cleared" the input by repeating five backspaces (and reusing the .). Now that we've made the default expression opensearch(*), there are 14 total characters that need deleting, if you delete them one by one. I didn't try .clear() either, but because it's supposed to just alias type({selectAll}{backspace}), I suspect it won't work either.

I do like the simplification idea, although I don't think we need a common function for it.

See if you like my latest approach better! I sure do 😁

kk I like it

Signed-off-by: Josh Romero <[email protected]>

* [Chore] Update other timeline `es()` examples and test usage to make sure we continue supporting the old format. * [Chore] Update BWC data generation to use es(*) * [Test] Add better programmatic text clearing for monaco editor * [Test] Wait for loader to finish after clicking update Update default expression from `.es(*)` to `opensearch(*)`. Both work, but we'd prefer the latter Signed-off-by: Josh Romero <[email protected]> --------- Signed-off-by: Josh Romero <[email protected]> (cherry picked from commit 7efc231) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md

* [Chore] Update timeline default expression (#2720) * [Chore] Update other timeline `es()` examples and test usage to make sure we continue supporting the old format. * [Chore] Update BWC data generation to use es(*) * [Test] Add better programmatic text clearing for monaco editor * [Test] Wait for loader to finish after clicking update Update default expression from `.es(*)` to `opensearch(*)`. Both work, but we'd prefer the latter Signed-off-by: Josh Romero <[email protected]> --------- Signed-off-by: Josh Romero <[email protected]> (cherry picked from commit 7efc231) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md * add changelog Signed-off-by: Josh Romero <[email protected]> --------- Signed-off-by: Josh Romero <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Josh Romero <[email protected]>

* [Chore] Update other timeline `es()` examples and test usage to make sure we continue supporting the old format. * [Chore] Update BWC data generation to use es(*) * [Test] Add better programmatic text clearing for monaco editor * [Test] Wait for loader to finish after clicking update Update default expression from `.es(*)` to `opensearch(*)`. Both work, but we'd prefer the latter Signed-off-by: Josh Romero <[email protected]> --------- Signed-off-by: Josh Romero <[email protected]> Signed-off-by: David Sinclair <[email protected]>

joshuarrrr requested a review from a team as a code owner November 1, 2022 21:04

joshuarrrr requested a review from zhongnansu November 1, 2022 21:04

kavilla linked an issue Nov 1, 2022 that may be closed by this pull request

[BUG] Timeline expression initialization is still using "es" as function name #1631

Closed

zhongnansu reviewed Nov 1, 2022

View reviewed changes

joshuarrrr added 2 commits November 18, 2022 19:58

[Chore] Update timeline default expression

327212c

Update default expression from `.es(*)` to `opensearch(*)`. Both work, but we'd prefer the latter Signed-off-by: Josh Romero <[email protected]>

[Chore] Update other timeline es() examples and test usage

47303cf

Signed-off-by: Josh Romero <[email protected]>

joshuarrrr force-pushed the chore/update-default-timeline-exp branch from a16af6b to 47303cf Compare November 18, 2022 23:22

joshuarrrr added 2 commits November 18, 2022 23:37

[Chore] Update BWC data generation to use es(*)

8413249

To make sure we continue supporting the old format. Signed-off-by: Josh Romero <[email protected]>

[Chore] Update changelog

8b6c195

Signed-off-by: Josh Romero <[email protected]>

joshuarrrr added the v2.5.0 'Issues and PRs related to version v2.5.0' label Nov 22, 2022

zhongnansu previously approved these changes Nov 22, 2022

View reviewed changes

kavilla previously approved these changes Dec 1, 2022

View reviewed changes

kavilla added the backport 2.x label Dec 1, 2022

joshuarrrr marked this pull request as draft December 14, 2022 21:45

Merge branch 'main' into chore/update-default-timeline-exp

fee215a

joshuarrrr dismissed stale reviews from kavilla and zhongnansu via fee215a December 28, 2022 21:24

Merge branch 'main' into chore/update-default-timeline-exp

7e8eba6

abbyhu2000 added v2.6.0 and removed v2.5.0 'Issues and PRs related to version v2.5.0' labels Jan 6, 2023

joshuarrrr added v2.7.0 and removed v2.6.0 labels Feb 17, 2023

ashwin-pc assigned joshuarrrr Mar 7, 2023

ashwin-pc marked this pull request as ready for review March 7, 2023 23:24

joshuarrrr added 6 commits March 8, 2023 15:22

Merge branch 'main' into chore/update-default-timeline-exp

0667e2b

Merge branch 'main' into chore/update-default-timeline-exp

0e7c231

Merge branch 'main' into chore/update-default-timeline-exp

3485523

Test: update generate_data to account for selectAll not working

189a239

Signed-off-by: Josh Romero <[email protected]>

Test: clear expression before each timeline test

76d6b3f

Signed-off-by: Josh Romero <[email protected]>

Test: fully clear expression text before each

db8983d

- wait for loader to finish after clicking update Signed-off-by: Josh Romero <[email protected]>

ananzh reviewed Mar 21, 2023

View reviewed changes

joshuarrrr mentioned this pull request Mar 23, 2023

2.7.0 Platform Upgrades for Plugins (O11y, MDS, Geospatial,+) #3674

Closed

23 tasks

joshuarrrr added 2 commits March 24, 2023 01:46

try clear() in before each

bdaff53

Signed-off-by: Josh Romero <[email protected]>

Add better programmatic text clearing for monaco editor

0cc1d3f

Signed-off-by: Josh Romero <[email protected]>

ananzh previously approved these changes Mar 24, 2023

View reviewed changes

kavilla previously approved these changes Mar 24, 2023

View reviewed changes

Update the expression clearing in the generate_data bwc helpers

6401ff4

Signed-off-by: Josh Romero <[email protected]>

joshuarrrr dismissed stale reviews from kavilla and ananzh via 6401ff4 March 24, 2023 21:36

ananzh approved these changes Mar 24, 2023

View reviewed changes

kavilla approved these changes Mar 27, 2023

View reviewed changes

joshuarrrr merged commit 7efc231 into opensearch-project:main Mar 27, 2023

opensearch-trigger-bot bot mentioned this pull request Mar 27, 2023

[Backport 2.x] [Chore] Update timeline default expression #3697

Merged

joshuarrrr deleted the chore/update-default-timeline-exp branch March 27, 2023 17:24

manasvinibs mentioned this pull request Jun 2, 2023

[CI][Tests] Add BWC tests for 2.7 and 2.8 #4023

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Chore] Update timeline default expression #2720

[Chore] Update timeline default expression #2720

joshuarrrr commented Nov 1, 2022 •

edited by kavilla

Loading

kavilla commented Nov 1, 2022

zhongnansu left a comment

kavilla commented Nov 2, 2022

kavilla commented Nov 2, 2022

ananzh commented Nov 3, 2022

kavilla commented Nov 3, 2022

joshuarrrr commented Nov 18, 2022

ashwin-pc commented Nov 30, 2022

kavilla left a comment

joshuarrrr commented Dec 7, 2022

joshuarrrr commented Dec 14, 2022

codecov-commenter commented Dec 28, 2022 •

edited

Loading

ananzh Mar 21, 2023

joshuarrrr Mar 22, 2023

joshuarrrr Mar 24, 2023

ananzh Mar 24, 2023

[Chore] Update timeline default expression #2720

[Chore] Update timeline default expression #2720

Conversation

joshuarrrr commented Nov 1, 2022 • edited by kavilla Loading

Description

Issues Resolved

Check List

kavilla commented Nov 1, 2022

zhongnansu left a comment

Choose a reason for hiding this comment

kavilla commented Nov 2, 2022

kavilla commented Nov 2, 2022

ananzh commented Nov 3, 2022

kavilla commented Nov 3, 2022

joshuarrrr commented Nov 18, 2022

ashwin-pc commented Nov 30, 2022

kavilla left a comment

Choose a reason for hiding this comment

joshuarrrr commented Dec 7, 2022

joshuarrrr commented Dec 14, 2022

codecov-commenter commented Dec 28, 2022 • edited Loading

Codecov Report

ananzh Mar 21, 2023

Choose a reason for hiding this comment

joshuarrrr Mar 22, 2023

Choose a reason for hiding this comment

joshuarrrr Mar 24, 2023

Choose a reason for hiding this comment

ananzh Mar 24, 2023

Choose a reason for hiding this comment

joshuarrrr commented Nov 1, 2022 •

edited by kavilla

Loading

codecov-commenter commented Dec 28, 2022 •

edited

Loading