-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Update Project Selection in Serverless Top Navigation #163076
Changes from 8 commits
3726abc
1c962b0
5c349b9
7c28472
b8adfc0
8b85a2e
f8c59e9
dd35820
72be0c0
bd6e7e1
eebd9fb
662e886
f84d6b6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -225,6 +225,11 @@ export class ChromeService { | |
projectNavigation.setProjectHome(homeHref); | ||
}; | ||
|
||
const setProjectsUrl = (projectsUrl: string) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if at some point replacing all the setters with a single one would make sense... Maybe not as that function would be a big one instead of many small functions... 🤔 updateProjectConfig({ ... partial configs here }); |
||
validateChromeStyle(); | ||
projectNavigation.setProjectsUrl(projectsUrl); | ||
}; | ||
|
||
const isIE = () => { | ||
const ua = window.navigator.userAgent; | ||
const msie = ua.indexOf('MSIE '); // IE 10 or older | ||
|
@@ -317,6 +322,7 @@ export class ChromeService { | |
loadingCount$={http.getLoadingCount$()} | ||
headerBanner$={headerBanner$.pipe(takeUntil(this.stop$))} | ||
homeHref$={projectNavigation.getProjectHome$()} | ||
projectsUrl$={projectNavigation.getProjectsUrl$()} | ||
docLinks={docLinks} | ||
kibanaVersion={injectedMetadata.getKibanaVersion()} | ||
prependBasePath={http.basePath.prepend} | ||
|
@@ -444,6 +450,7 @@ export class ChromeService { | |
getChromeStyle$: () => chromeStyle$.pipe(takeUntil(this.stop$)), | ||
project: { | ||
setHome: setProjectHome, | ||
setProjectsUrl, | ||
setNavigation: setProjectNavigation, | ||
setSideNavComponent: setProjectSideNavComponent, | ||
setBreadcrumbs: setProjectBreadcrumbs, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,10 @@ export interface CloudStart { | |
* The full URL to the users and roles page on Elastic Cloud. Undefined if not running on Cloud. | ||
*/ | ||
usersAndRolesUrl?: string; | ||
/** | ||
* The full URL to the serverless projects page on Elastic Cloud. Undefined if not running in Serverless. | ||
*/ | ||
projectsUrl?: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about this, because this could also go inside There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is ok here. Serverless is Cloud. Now if we want to group the urls under a "serverless" parent that could be an option interface CloudStart {
serverless: {
projectsUrl?: string;
}
...
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is already There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for hijacking, but will There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think exposing all URLs server-side makes sense; It shouldn't be a problem as they are all static. But I think it better to do in a separate pr There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thanks! Do you need a GitHub issue for that, or is it already on your radar? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @azasypkin, I am passing by this code myself 😅, I think this is for the @elastic/kibana-core team and I was just sharing my opinion on exposing the URLs. If they agree, I can help out and follow up after this pr There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Roger that @Dosant! Then, forwarding my question to @elastic/kibana-core - do you folks want me to file a GitHub issue to expose these configuration flags via the server-side contract, or is it already on your radar (unless there is a reason not to expose this)? We need this for #162887 that is targeting Serverless MVP. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 from me to expose URL values in |
||
/** | ||
* The full URL to the elasticsearch cluster. | ||
*/ | ||
|
@@ -90,6 +94,10 @@ export interface CloudSetup { | |
* The full URL to the deployment management page on Elastic Cloud. Undefined if not running on Cloud. | ||
*/ | ||
deploymentUrl?: string; | ||
/** | ||
* The full URL to the serverless projects page on Elastic Cloud. Undefined if not running in Serverless. | ||
*/ | ||
projectsUrl?: string; | ||
/** | ||
* The full URL to the user profile page on Elastic Cloud. Undefined if not running on Cloud. | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,5 +24,6 @@ | |
"@kbn/core-chrome-browser", | ||
"@kbn/core-chrome-browser-internal", | ||
"@kbn/i18n-react", | ||
"@kbn/cloud-plugin", | ||
] | ||
} |
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.
Curious what are your thoughts on this?
We will propagate the correct value from cloud, but I'd like to have something displayed locally for testing. Is it fine to keep it here? Or should I hardcode some fallback from code?
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.
I wouldn't commit it in the repo. Good to mention it in the PR description for local testing. I removed all those URLs here https://github.com/elastic/kibana/pull/161971/files#diff-ebeb9e9dc4c3ee5e81f743f0eda3c06f1fc161a11127b374d21f0da5e6368fd9
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.
So I am not sure about this, because developers starting it locally would not see the cloud URLs you've added in the sidenav and wouldn't see this link in the header.
@elastic/kibana-core, Curious, what would you recommend?
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.
Seems that we need a
serverless.dev.yml
thenThere 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.
One alternative is to handle this at the config level:
Would that be appropriate for something like this setting? This way it can always be overridden, but
--serverless
flag would make it default to/projects
.Feel like this might be a broader discussion as it would be useful for cases other than this!
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.
We could even make
schema.maybe(schema.string())
=>schema.never()
if this is only intended for serverless.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.
I like this approach, we also have other URLs like this that we didn't add to config, so they are undefined locally and aren't visible. I'd personally prefer to see them even if they lead to nowhere or to a wrong env.
https://github.com/elastic/kibana/pull/161971/files#diff-ebeb9e9dc4c3ee5e81f743f0eda3c06f1fc161a11127b374d21f0da5e6368fd9
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.
@jloleysens, I've follow you suggestion for this new config: eebd9fb
I didn't touch other existing configs