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

[ML] Data frame analytics: Fix source index checks. #44479

Merged
merged 5 commits into from
Sep 2, 2019

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Aug 30, 2019

Summary

The source index selection is based in Kibana index patterns. The form validation was too strict in that regard and used the same check as the destination index.

  • Removes the check if the source index exists, it's not practical with index patterns containing wildcards.
  • Changes the name validation to use the Kibana index pattern name check.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@walterra walterra added bug Fixes for quality problems that affect the customer experience :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.4.0 v7.5.0 Feature:Data Frame Analytics ML data frame analytics features labels Aug 30, 2019
@walterra walterra requested a review from a team as a code owner August 30, 2019 10:12
@walterra walterra self-assigned this Aug 30, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

As discussed, the check for the valid source index pattern in the create form isn't really needed with the form as it is.

Otherwise tested and LGTM.

// based on code used by `ui/index_patterns` internally
// remove the space character from the list of illegal characters
INDEX_PATTERN_ILLEGAL_CHARACTERS.pop();
const characterList = INDEX_PATTERN_ILLEGAL_CHARACTERS.join(', ');
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to be checking against this list of characters, as the control for selecting the source index is a dropdown which list the Kibana index patterns, so presumably this check was done when the index pattern was created? So really the form just needs to be checking that the source index hasn't been left empty.

])
!sourceIndexNameEmpty &&
!sourceIndexNameValid && [
<Fragment>
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, this should never be shown as you can only select a valid KIbana index pattern as the source index.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@walterra
Copy link
Contributor Author

walterra commented Sep 2, 2019

@peteharverson You're right about the checks in your comments. This was originally done because the very first version was just a plain text input field. I'll leave it in for now because there will be more updates to this (e.g. support for saved searches). Once all features are in and the code is still not necessary I'll remove it.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@walterra walterra merged commit 20f2458 into elastic:master Sep 2, 2019
@walterra walterra deleted the ml-dfa-fix-source-index branch September 2, 2019 09:51
walterra added a commit to walterra/kibana that referenced this pull request Sep 2, 2019
The source index selection is based in Kibana index patterns. The form validation was too strict in that regard and used the same check as the destination index.
- Removes the check if the source index exists, it's not practical with index patterns containing wildcards.
- Changes the name validation to use the Kibana index pattern name check.
walterra added a commit to walterra/kibana that referenced this pull request Sep 2, 2019
The source index selection is based in Kibana index patterns. The form validation was too strict in that regard and used the same check as the destination index.
- Removes the check if the source index exists, it's not practical with index patterns containing wildcards.
- Changes the name validation to use the Kibana index pattern name check.
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 2, 2019
…-to-np-ready

* 'master' of github.com:elastic/kibana: (409 commits)
  [ML] Data frame analytics: Fix source index checks. (elastic#44479)
  [Code] try fix this test (elastic#43692)
  skip flaky suite (elastic#44572) (elastic#42111) (elastic#44286) (elastic#43557) (elastic#42567)
  skip flaky suite (elastic#44560)
  skip flaky suite (elastic#44250)
  disable flaky suite (elastic#41336)
  [code] Update download URLs for go lsp. (elastic#44581)
  disable flaky suite (elastic#44575)
  disable flaky suite (elastic#44576)
  [Code] add functional test to verify lang server full api symbol/reference counts (elastic#44051)
  Improve Storybook scripts and load time (elastic#44511)
  Update Dependencies (elastic#44519)
  Remove use of injecti18n in Embeddables plugin (elastic#44043)
  [SIEM] Adds a configuraton option for the default SIEM date time range (elastic#44540)
  [Uptime]Fix/issue 40584  section headline should be inside panel (elastic#43468)
  disable flaky suite (elastic#22322)
  Changes network to use ECS fields (elastic#44392)
  Fix 'workpad flash' when loading new workpad (elastic#44387)
  [renovate] bump new PR version labels
  Update dependency cmd-shim to ^2.1.0 (elastic#44034)
  ...

# Conflicts:
#	src/legacy/core_plugins/console/public/index.html
walterra added a commit that referenced this pull request Sep 2, 2019
The source index selection is based in Kibana index patterns. The form validation was too strict in that regard and used the same check as the destination index.
- Removes the check if the source index exists, it's not practical with index patterns containing wildcards.
- Changes the name validation to use the Kibana index pattern name check.
walterra added a commit that referenced this pull request Sep 2, 2019
The source index selection is based in Kibana index patterns. The form validation was too strict in that regard and used the same check as the destination index.
- Removes the check if the source index exists, it's not practical with index patterns containing wildcards.
- Changes the name validation to use the Kibana index pattern name check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Data Frame Analytics ML data frame analytics features :ml release_note:skip Skip the PR/issue when compiling release notes v7.4.0 v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants