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

[data view mgmt] change urls from indexPatterns to dataViews #114912

Merged
merged 15 commits into from
Oct 21, 2021

Conversation

mattkime
Copy link
Contributor

@mattkime mattkime commented Oct 13, 2021

Summary

Changing data view management urls from indexPatterns to dataViews. Includes redirects for old urls.

@mattkime mattkime added buildkite-ci Team:AppServicesSv Feature:Data Views Data Views code and UI - index patterns before 8.0 Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages v8.0.0 backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes labels Oct 13, 2021
@mattkime mattkime changed the title index pattern management to data view url changes [data view mgmt] change urls from indexPatterns to dataViews Oct 13, 2021
@elastic elastic deleted a comment from kibanamachine Oct 14, 2021
@mattkime mattkime marked this pull request as ready for review October 19, 2021 17:01
@mattkime mattkime requested review from a team as code owners October 19, 2021 17:01
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

maps changes LGTM

Copy link
Contributor

@stephmilovic stephmilovic left a comment

Choose a reason for hiding this comment

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

LGTM! thanks @mattkime 🚀 🎸

@qn895
Copy link
Member

qn895 commented Oct 19, 2021

ML changes LGTM 🎉

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.

KibanaVisEditors changes LGTM, I tested it locally and redirects seem to work fine :)

@@ -57,3 +58,5 @@ export const ManagementRouter = memo(
</Router>
)
);

//
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 that this has been added by accident.

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

This PR is introducing specific hacks for one use case in the otherwise generic management framework which I think is the wrong way to implement this.

what about keeping an “indexpattern” management app registered which, on mount, simply redirects to the dataviews app? This is how we solved this for forwarding legacy “app/kibana” calls to “app/dashboards”, “app/visualize” and so on.

See here: https://github.com/elastic/kibana/blob/master/src/plugins/url_forwarding/public/forward_app/forward_app.ts#L23

similar for the capabilities thing - maybe we should slightly extend the “app” definition here to allow a separate capability key which is not app.id ?

@mattkime
Copy link
Contributor Author

mattkime commented Oct 20, 2021

@flash1293 We could do that but we'd need to devise a way to prevent the management app from being shown in the sidebar. What do you think?

I think I'd prefer to keep this as is until there are more instances of management redirects. This isn't a 'clean' way of doing things but its simpler than implementing a more generic solution.

@flash1293
Copy link
Contributor

flash1293 commented Oct 20, 2021

@mattkime Good point about hiding the redirect app, but I it's worth making redirects and capabilities under another id a concept of the ManagementApp - IMHO placing a little hack to get something done is fine as long as it's encapsulated in a plugin, but this is a cross-plugin mixture of responsibilities here which leads us down the wrong track. In terms of code changes it doesn't seem to be much more complex to add optional aliasPath and capabilitiesId props to the ManagementApp class and use them in the same way you placed data view specific workarounds in the current PR.

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Seems there are still quite a bunch of old URLs in code. Did you skip them intentionally to rely on redirect or were these overlooked?

Screen Shot 2021-10-20 at 17 50 38

@mattkime
Copy link
Contributor Author

@Dosant Looks like you found a better way to find instances of these urls in the code base than I had used. I don't see a need to get all of them due to the redirects. I'd rather not expand the scope of this PR since I'm trying to get this merged by EOD tomorrow since I'm out next week. The additional urls could be addressed in a follow up.

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Tested that redirect works as expected. Also tested without needed mgmt capabilities.

only remark: #114912 (comment)

@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
fileUpload 673.4KB 673.4KB -4.0B
indexPatternManagement 77.1KB 77.2KB +61.0B
management 8.9KB 9.0KB +144.0B
maps 2.7MB 2.7MB -4.0B
ml 3.6MB 3.6MB -4.0B
securitySolution 4.6MB 4.6MB -4.0B
stackAlerts 159.0KB 159.0KB -4.0B
total +185.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
dataViews 38.0KB 38.0KB -8.0B
indexPatternManagement 4.1KB 4.2KB +59.0B
management 10.0KB 10.2KB +165.0B
total +216.0B

History

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

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for making these changes @mattkime ! Tested and both redirect and disabled capability works fine.

@mattkime mattkime merged commit d2dea68 into elastic:master Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting buildkite-ci Feature:Data Views Data Views code and UI - index patterns before 8.0 Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages release_note:skip Skip the PR/issue when compiling release notes v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants