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.

Sign up for GitHub

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

Jump to bottom

Index Pattern class - remove toJSON and toString #76246

Merged
merged 8 commits into from Sep 2, 2020
Merged

Index Pattern class - remove toJSON and toString #76246

merged 8 commits into from Sep 2, 2020

Conversation

mattkime
Copy link
Contributor

@mattkime mattkime commented Aug 29, 2020 •
edited

Summary

Index Pattern class - remove toJSON and toString - can be replaced by indexPattern.id

Checklist

@mattkime mattkime changed the title remove toJSON Index Pattern class - remove toJSON and toString Aug 30, 2020
@mattkime mattkime added v8.0.0 v7.10.0 Team:AppArch Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:skip Skip the PR/issue when compiling release notes labels Aug 30, 2020
@mattkime mattkime added this to In progress in kibana-app-arch via automation Aug 30, 2020
@elastic elastic deleted a comment from kibanamachine Aug 30, 2020
@mattkime mattkime marked this pull request as ready for review August 30, 2020 02:19
@mattkime mattkime requested review from a team as code owners August 30, 2020 02:19
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

lizozom
lizozom approved these changes Aug 31, 2020
View reviewed changes
Copy link
Contributor

@lizozom lizozom left a comment •
edited

Choose a reason for hiding this comment

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

LGTM once green

ppisljar
ppisljar approved these changes Aug 31, 2020
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

kibana-app-arch automation moved this from In progress to Review in progress Sep 2, 2020
peteharverson
peteharverson requested changes Sep 2, 2020
View reviewed changes
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.

Added a suggestion for fixing the link that's broken in the ML data visualizer.

.../ml/public/application/datavisualizer/index_based/components/actions_panel/actions_panel.tsx Outdated
@@ -31,7 +31,7 @@ export const ActionsPanel: FC<Props> = ({ indexPattern }) => {
function openAdvancedJobWizard() {
// TODO - pass the search string to the advanced job page as well as the index pattern
// (add in with new advanced job wizard?)
window.open(`#/jobs/new_job/advanced?index=${indexPattern}`, '_self');
window.open(`#/jobs/new_job/advanced?index=${indexPattern.id}`, '_self');
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is causing the link to the advanced job editor in the data visualizer to fail.

image

Your change seems to have exposed an issue with the routing code in the ML plugin. In order to fix the link to the job wizard and get this PR through, I suggest making a slight change to the way the URL is built up here.

Add this to the list of imports:

import { getBasePath } from '../../../../util/dependency_cache';

and then construct the URL by:

const basePath = getBasePath();
window.open(`${basePath.get()}/app/ml/jobs/new_job/advanced?index=${indexPattern.id}`, '_self');

This fixes the link in the ML data visualizer for me (hopefully this will fix the associated functional tests too).

We can then follow-up with a cleaner refactor as part of the work we are already looking at as part of our switch to BrowserRouter in #72013.

@elastic elastic deleted a comment from kibanamachine Sep 2, 2020
peteharverson
peteharverson approved these changes Sep 2, 2020
View reviewed changes
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.

ML edits LGTM

@mattkime
Copy link
Contributor Author

mattkime commented Sep 2, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
ml 8.2MB +230.0B 8.2MB

page load bundle size

id value diff baseline
data 1.4MB -122.0B 1.4MB

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

@mattkime mattkime merged commit ee2ceef into elastic:master Sep 2, 2020
kibana-app-arch automation moved this from Review in progress to Done in current release Sep 2, 2020
mattkime added a commit to mattkime/kibana that referenced this pull request Sep 2, 2020
@mattkime @elasticmachine
Index Pattern class - remove toJSON and toString (elastic#76246)
be59e9f 
* remove toJSON

* remove toJSON, toString

* fix ml url

* Update actions_panel.tsx

* fix url

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@mattkime mattkime mentioned this pull request Sep 2, 2020
Merged
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 3, 2020
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into ingest-pipeli…
81bad4f 
…nes/processors-forms-k-s

* 'master' of github.com:elastic/kibana: (216 commits)
  [Ingest Manager] Split Registry errors into Connection & Response (elastic#76558)
  [Security Solution] add an excess validation instead of the exact match (elastic#76472)
  Introduce TS incremental builds & move src/test_utils to TS project  (elastic#76082)
  fix bad merge (elastic#76629)
  [Newsfeed] Ensure the version format when calling the API (elastic#76381)
  remove server_extensions mixin (elastic#76606)
  Remove legacy applications and legacy mode (elastic#75987)
  [Discover] Fix sidebar element focus behavior when adding / removing columns (elastic#75749)
  Replace FetchOptions with ISearchOptions (elastic#76538)
  Move streams utils to the core  (elastic#76397)
  [Ingest Manager] Wording & linking improvements (elastic#76284)
  remove legacy kibana plugin (elastic#76064)
  [Form lib] Fix regression on field not being validated after reset to its default value. (elastic#76379)
  Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal (elastic#76482)
  [APM] Service maps layout enhancements (elastic#76481)
  [Security Solution][Detection Engine] Remove RuleTypeSchema in favor of Type for TypeScript (elastic#76586)
  [Security Solution][Exceptions] - Updates enum schema and tests (elastic#76544)
  Index Pattern class - remove toJSON and toString (elastic#76246)
  skip failing suite (elastic#76581)
  Split up and clarify Enterprise Search codeowners (elastic#76580)
  ...

# Conflicts:
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processor_settings_fields.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/foreach.tsx
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 3, 2020
@jloleysens
Merge branch 'ingest-pipelines/processors-forms-k-s' of github.com:jl…
480523f 
…oleysens/kibana into ingest-pipelines/processors-forms-k-s

* 'ingest-pipelines/processors-forms-k-s' of github.com:jloleysens/kibana: (216 commits)
  [Ingest Manager] Split Registry errors into Connection & Response (elastic#76558)
  [Security Solution] add an excess validation instead of the exact match (elastic#76472)
  Introduce TS incremental builds & move src/test_utils to TS project  (elastic#76082)
  fix bad merge (elastic#76629)
  [Newsfeed] Ensure the version format when calling the API (elastic#76381)
  remove server_extensions mixin (elastic#76606)
  Remove legacy applications and legacy mode (elastic#75987)
  [Discover] Fix sidebar element focus behavior when adding / removing columns (elastic#75749)
  Replace FetchOptions with ISearchOptions (elastic#76538)
  Move streams utils to the core  (elastic#76397)
  [Ingest Manager] Wording & linking improvements (elastic#76284)
  remove legacy kibana plugin (elastic#76064)
  [Form lib] Fix regression on field not being validated after reset to its default value. (elastic#76379)
  Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal (elastic#76482)
  [APM] Service maps layout enhancements (elastic#76481)
  [Security Solution][Detection Engine] Remove RuleTypeSchema in favor of Type for TypeScript (elastic#76586)
  [Security Solution][Exceptions] - Updates enum schema and tests (elastic#76544)
  Index Pattern class - remove toJSON and toString (elastic#76246)
  skip failing suite (elastic#76581)
  Split up and clarify Enterprise Search codeowners (elastic#76580)
  ...
mattkime added a commit that referenced this pull request Sep 3, 2020
@mattkime
Index Pattern class - remove toJSON and toString (#76246) (#76582)
1f179bb 
* remove toJSON, toString
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizozom lizozom lizozom approved these changes

@peteharverson peteharverson peteharverson approved these changes

@ppisljar ppisljar ppisljar approved these changes

Assignees
No one assigned
Labels
Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
kibana-app-arch
  
Done in current release
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mattkime @elasticmachine @kibanamachine @lizozom @peteharverson @ppisljar