-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data #53794
Conversation
Closes: elastic#43444 Make sure that all index pattern related imports use NP (and delete index_pattern folder from shim)
Pinging @elastic/kibana-app-arch (Team:AppArch) |
💚 Build SucceededTo update your PR or re-run it, just comment with: |
@elasticmachine merge upstream |
💚 Build SucceededTo update your PR or re-run it, just comment with: |
@elasticmachine merge upstream |
1 similar comment
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ML changes LGTM
# Conflicts: # src/legacy/ui/public/vis/editors/default/components/agg_params.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM. I've added a comment on a ML part that I think it should be changed
|
||
// 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(', '); | ||
indexPatterns.ILLEGAL_CHARACTERS.pop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure we want to allow this mutation? I know that it was there before but I'm not sure if we want to allow it. Also the above commit that use the old ui/index_patterns
folder should change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe instead of using ILLEGAL_CHARACTERS
just use ILLEGAL_CHARACTERS_VISIBLE
that doesn't contain the space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes to avoid the mutation, could do
const illegalCharacters = [...INDEX_PATTERN_ILLEGAL_CHARACTERS];
illegalCharacters.pop();
const characterList = illegalCharacters.join(', ');
and the
based on code used by ui/index_patterns internally
comment, could be edited to just based on code used by indexPatterns internally
to remove the reference to the old ui_index_patterns
folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markov00 I agree with you, we cannot leave it as it is. Looks like you are absolutely right and here we should use ILLEGAL_CHARACTERS_VISIBLE
@liza-mae @peteharverson I've updated this PR please have a look it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest edits to remove the mutation of the illegal characters list LGTM.
Outside of the scope of this PR I guess, but am wondering if the name ILLEGAL_CHARACTERS_VISIBLE
defined in src/plugins/data/public/index_patterns/lib/types.ts
is that intuitive?
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…lastic#53794) * Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data Closes: elastic#43444 Make sure that all index pattern related imports use NP (and delete index_pattern folder from shim) * fix JEST tests * fix CI * fix PR comments Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* master: Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794) moved Task Manager server code under "server" directory (elastic#53777)
…t-types * alerting/created_at-and-updated_at: updatedAt should equal createdAt on creation Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794) moved Task Manager server code under "server" directory (elastic#53777) Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886) Updating transitive dependencies to use handlebars@4.5.3 (elastic#53899) [Reporting/Tests] consolidate functional test configs (elastic#52671) [Reporting] Correct the docvalue_fields params in the search query Download CSV from Dashboard Panel (elastic#52833) [Test/Newsfeed] Re-enable test and add news item to be filtered (elastic#53905) cleanup server-log action (elastic#53326) [Uptime] Delete uptime eslint rule skip (elastic#50912) [skip-ci] Expression Lifecycle Docs (elastic#51494) [Endpoint] add react router to endpoint app (elastic#53808) [SIEM][Detection Engine] Silence 409 errors on signal creation (elastic#53859) [Maps] get max_result_window and max_inner_result_window from index settings (elastic#53500) [ML] New Platform server shim: update analytics routes to use new platform router (elastic#53521) fixes typo on engine detection page (elastic#53877) [Maps] push mapbox value extraction from VectorStyle and into DynamicStyleProperty (elastic#53806) Fix suggested value for time_zone in range query (elastic#53841) Clean up generic hooks, use react-use instead (elastic#53822)
* master: Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794) moved Task Manager server code under "server" directory (elastic#53777)
* master: [SR] Enable component integration tests (elastic#53893) Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794) moved Task Manager server code under "server" directory (elastic#53777)
…53794) (#53936) * Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data Closes: #43444 Make sure that all index pattern related imports use NP (and delete index_pattern folder from shim) * fix JEST tests * fix CI * fix PR comments Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
…nsole-dependencies * 'master' of github.com:elastic/kibana: (33 commits) adds strict types to Alerting Client (elastic#53821) [Dashboard] Empty screen redesign (elastic#53681) Migrate config deprecations and `ShieldUser` functionality to the New Platform (elastic#53768) increase delay to make sure license refetched (elastic#53882) Allow custom NP plugin paths in production (elastic#53562) [Maps] show custom color ramps in legend (elastic#53780) [Lens] Expression type on document can be null (elastic#53883) [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778) Update dependency @elastic/charts to v16.0.2 (elastic#52619) Set consistent EOL symbol in core API docs (elastic#53815) [Logs UI] Refactor query bar state to hooks (elastic#52656) [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937) Invalidate alert API Key when generating a new one (elastic#53732) [Logs UI] HTTP API for log entries (elastic#53798) [kbn/pm] add caching to bootstrap (elastic#53622) adds createdAt and updatedAt fields to alerting (elastic#53793) [SR] Enable component integration tests (elastic#53893) Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794) moved Task Manager server code under "server" directory (elastic#53777) Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886) ... # Conflicts: # yarn.lock
Closes: #43444
Summary
Summarize your PR. If it involves visual changes include a screenshot or gif.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers