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

[TSVB] Fix more than one empty labels in markdown breaks the values for all other labels #100432

Merged
merged 9 commits into from
May 25, 2021

Conversation

Kuznietsov
Copy link
Contributor

@Kuznietsov Kuznietsov commented May 21, 2021

Fixes #98760.

When the user created a markdown with more than one variables based on empty labels, the markdown appeared broken.

This PR replaces single adding of brackets to empty label with global by using regexp.

Before:
image

After:
image

@kibanamachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@alexwizp alexwizp requested a review from VladLasitsa May 21, 2021 09:04
@alexwizp alexwizp changed the title #98760 Fix. [TSVB] Fix more than one empty labels in markdown breaks the values for all other labels May 21, 2021
@alexwizp alexwizp removed the request for review from VladLasitsa May 21, 2021 09:53
@alexwizp alexwizp changed the title [TSVB] Fix more than one empty labels in markdown breaks the values for all other labels [WIP][TSVB] Fix more than one empty labels in markdown breaks the values for all other labels May 21, 2021
@alexwizp alexwizp added the WIP Work in progress label May 21, 2021
@Kuznietsov Kuznietsov changed the title [WIP][TSVB] Fix more than one empty labels in markdown breaks the values for all other labels [TSVB] Fix more than one empty labels in markdown breaks the values for all other labels May 21, 2021
@alexwizp alexwizp removed the WIP Work in progress label May 21, 2021
@alexwizp alexwizp requested a review from VladLasitsa May 21, 2021 12:39
Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

LGTM! Tested locally, works fine

image

@alexwizp alexwizp requested a review from stratoula May 24, 2021 11:58
@alexwizp alexwizp added v7.14.0 v8.0.0 Feature:TSVB TSVB (Time Series Visual Builder) release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure labels May 24, 2021
@alexwizp alexwizp marked this pull request as ready for review May 24, 2021 11:59
@alexwizp alexwizp requested a review from a team May 24, 2021 11:59
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@alexwizp
Copy link
Contributor

@elasticmachine merge upstream

@stratoula
Copy link
Contributor

Jenkins, test this

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

This works great @Kunzetsov 👏
I noticed that we don't have any test case on the replace_vars.test.js to test the empty labels scenarios.

Can you please add one? Thank you a lot!

@Kuznietsov
Copy link
Contributor Author

This works great @Kunzetsov 👏
I noticed that we don't have any test case on the replace_vars.test.js to test the empty labels scenarios.

Can you please add one? Thank you a lot!

Sure, I'll write tests.

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

@stratoula
Copy link
Contributor

Jenkins, test this

@@ -14,7 +14,8 @@ import { i18n } from '@kbn/i18n';
export function replaceVars(str, args = {}, vars = {}) {
try {
// we need add '[]' for emptyLabel because this value contains special characters. (https://handlebarsjs.com/guide/expressions.html#literal-segments)
const template = handlebars.compile(str.replace(emptyLabel, `[${emptyLabel}]`), {
// ''.split(...).join(...) as a save alternative of the replaceAll...
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think this comment is not needed here.

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

LGTM, I tested it locally and works fine with multiple variables based on empty labels.
Please merge in case of green ci. Thank you a lot for the test 👏

@stratoula
Copy link
Contributor

Jenkins, test this

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
visTypeTimeseries 1.7MB 1.7MB +16.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @Kunzetsov

@Kuznietsov Kuznietsov merged commit 818fa90 into elastic:master May 25, 2021
Kuznietsov added a commit to Kuznietsov/kibana that referenced this pull request May 25, 2021
…or all other labels (elastic#100432)

* Fixed multiple empty labels monitoring with [].

* Fixed regexp group behavior and replaced by ''.replaceAll.

* Changed ''.replaceAll with ''.split(...).join(...)

* Added test for (empty) label case.

* Removed not necessary comment.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Kuznietsov added a commit that referenced this pull request May 26, 2021
…or all other labels (#100432) (#100548)

* Fixed multiple empty labels monitoring with [].

* Fixed regexp group behavior and replaced by ''.replaceAll.

* Changed ''.replaceAll with ''.split(...).join(...)

* Added test for (empty) label case.

* Removed not necessary comment.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
ecezalp pushed a commit to ecezalp/kibana that referenced this pull request May 26, 2021
…or all other labels (elastic#100432)

* Fixed multiple empty labels monitoring with [].

* Fixed regexp group behavior and replaced by ''.replaceAll.

* Changed ''.replaceAll with ''.split(...).join(...)

* Added test for (empty) label case.

* Removed not necessary comment.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
ecezalp pushed a commit to ecezalp/kibana that referenced this pull request May 26, 2021
…or all other labels (elastic#100432)

* Fixed multiple empty labels monitoring with [].

* Fixed regexp group behavior and replaced by ''.replaceAll.

* Changed ''.replaceAll with ''.split(...).join(...)

* Added test for (empty) label case.

* Removed not necessary comment.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💝community Feature:TSVB TSVB (Time Series Visual Builder) release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TSVB] More than one empty labels in markdown breaks the values for all other labels.
5 participants