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

Swap defaultAppId with defaultRoute in example config #28547

Merged

Conversation

matschaffer
Copy link
Contributor

Connected to #6902

Summary

Swaps example config based on current actual functionality. I got
confused about which setting to use after using the example config as a
starting point.

Checklist

For maintainers

@matschaffer matschaffer added Team:Operations Team label for Operations Team review labels Jan 11, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations

@matschaffer matschaffer requested a review from epixa January 11, 2019 01:51
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@matschaffer matschaffer changed the title Swap defaultAddId with defaultRoute in example config Swap defaultAppId with defaultRoute in example config Jan 11, 2019
Copy link
Member

@mistic mistic left a comment

Choose a reason for hiding this comment

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

Maybe we can also deprecate the old defaultAppId option. What do you think @tylersmalley ?

@matschaffer
Copy link
Contributor Author

Does defaultAppId still do something? When I tried to set it to "monitoring" kibana just gave me a blank screen (on 6.3.2)

@mistic
Copy link
Member

mistic commented Jan 15, 2019

@matschaffer I believe it is working. At least I just set into the kibana.yml
kibana.defaultAppId: "dashboards"

And it worked. Let's see if @tylersmalley has any insights on this.

@fbaligand
Copy link
Contributor

Now that we have Kibana Spaces, it would be great that "defaultRoute" is available as advanced setting!
It would let to define a default route per space!

@fbaligand
Copy link
Contributor

BTW, for this PR, it could be interesting to fix documentation for "kibana.defaultAppId" default value.
Currently, it is indicated "discover", although the right default value is "home":
https://github.com/elastic/kibana/blob/master/docs/setup/settings.asciidoc

@fbaligand
Copy link
Contributor

Compared to defaultAppId, I see a major limitation in defaultRoute:

  • defaultAppId is the default home page per space (as it contains only hash part)
  • defaultRoute is not per space (as it contains full URI)

Personally, I use defaultAppId to define a custom dashboard home page per space.
Example: "dashboard/home"

@jbudz
Copy link
Member

jbudz commented Jan 22, 2019

Yeah this is a little confusing, but +1 on the swap to de-emphasize kibana the application. Taking a step back, we have the kibana platform which has applications. Inside that there's a SPA called kibana which includes discover, visualize, dashboard, home, dev tools).

kibana.defaultAppId sets the page that you'll be redirected to if you visit localhost:5601/app/kibana using the browser router

server.defaultRoute sets the page that you'll be redirected to if you visit localhost:5601/ using the server router

I think deprecating defaultAppId isn't going to be very straight forward because the triggers are separate.

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

LGTM, #28547 (comment) would be cool too. Spaces integration sounds interesting but I'm not 100% familiar with it yet

@fbaligand
Copy link
Contributor

I realized today that for a "home page per Kibana Space", there is already an issue for that :
#26126

@epixa epixa removed their request for review March 5, 2019 16:15
@epixa
Copy link
Contributor

epixa commented Mar 5, 2019

I'm removing my review on this since @jbudz already reviewed it. I'm not sure what the plan is here since it has been idle since January.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@matschaffer matschaffer force-pushed the default-app-vs-default-route-config branch from 34d1e9b to 02452c0 Compare March 7, 2019 04:43
@matschaffer
Copy link
Contributor Author

Rebased to try to clear CI failures. Diff looks unchanged. @jbudz - let me know if I'm clear to click squash merge or if something else should happen first (after CI passes, of course).

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jbudz
Copy link
Member

jbudz commented Mar 7, 2019

👍 go for it

@matschaffer matschaffer merged commit 87f1bef into elastic:master Mar 8, 2019
@matschaffer matschaffer deleted the default-app-vs-default-route-config branch March 8, 2019 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Operations Team label for Operations Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants