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

Update Project Selection in Serverless Top Navigation #163076

Merged
merged 13 commits into from
Aug 7, 2023

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Aug 3, 2023

Summary

close #163014

  • Changing My Deployments -> Projects
  • Removing hardcoded url and passing one from the config

@Dosant to add this setting to cloud

@@ -9,6 +9,7 @@ xpack.fleet.internal.activeAgentsSoftLimit: 25000

# Cloud links
xpack.cloud.base_url: "https://cloud.elastic.co"
xpack.cloud.projects_url: "/projects/"
Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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 then

Copy link
Contributor

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:

  projects_url: schema.conditional(
      schema.contextRef('serverless'),
      true,
      schema.string({ defaultValue: '/projects'}),
      schema.maybe(schema.string())
  )

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.

Seems that we need a serverless.dev.yml then

Feel like this might be a broader discussion as it would be useful for cases other than this!

Copy link
Contributor

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.

Copy link
Contributor Author

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:

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

Copy link
Contributor Author

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

/**
* The full URL to the serverless projects page on Elastic Cloud. Undefined if not running in Serverless.
*/
projectsUrl?: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about this, because this could also go inside serverless.*
The problem with putting it inside serverless is that it would be tricky to test this locally as serveless.* must specify projectid

Copy link
Contributor

Choose a reason for hiding this comment

The 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;
  }
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is already serverless parent config, but I am on a fence about putting it there because a valid servleless config requires projectid and the testing local config that just includes serverless.prjectsUrl would be considered invalid

@Dosant Dosant changed the title D/2023 08 03 change deployments link Update Project Selection in Serverless Top Navigation Aug 3, 2023
@Dosant Dosant added release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) Project:Serverless Work as part of the Serverless project for its initial release Feature:Chrome Core's Chrome UI (sidenav, header, breadcrumbs) labels Aug 3, 2023
@Dosant Dosant marked this pull request as ready for review August 3, 2023 15:58
@Dosant Dosant requested review from a team as code owners August 3, 2023 15:58
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@Dosant Dosant requested a review from sebelga August 3, 2023 15:58
@Dosant Dosant requested a review from a team as a code owner August 4, 2023 09:52
@Dosant
Copy link
Contributor Author

Dosant commented Aug 4, 2023

@elasticmachine merge upstream

Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@@ -9,6 +9,7 @@ xpack.fleet.internal.activeAgentsSoftLimit: 25000

# Cloud links
xpack.cloud.base_url: "https://cloud.elastic.co"
xpack.cloud.projects_url: "/projects/"
Copy link
Contributor

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

@@ -225,6 +225,11 @@ export class ChromeService {
projectNavigation.setProjectHome(homeHref);
};

const setProjectsUrl = (projectsUrl: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 });

/**
* The full URL to the serverless projects page on Elastic Cloud. Undefined if not running in Serverless.
*/
projectsUrl?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The 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;
  }
  ...
}

/**
* The full URL to the serverless projects page on Elastic Cloud. Undefined if not running in Serverless.
*/
projectsUrl?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for hijacking, but will projectsUrl or at least baseUrl be exposed on the server-side contract? We need this on the server side to implement #162887

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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

Thanks! Do you need a GitHub issue for that, or is it already on your radar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

@jloleysens jloleysens Aug 8, 2023

Choose a reason for hiding this comment

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

+1 from me to expose URL values in cloud plugin's server side. Might be good to have an issue to know which values you need, thanks!

@Dosant Dosant requested a review from jloleysens August 7, 2023 09:51
Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Config changes LGTM

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
cloud 15 16 +1

Page load bundle

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

id before after diff
cloud 5.4KB 5.4KB +62.0B
core 372.0KB 372.2KB +233.0B
serverless 5.4KB 5.4KB +60.0B
total +355.0B
Unknown metric groups

API count

id before after diff
cloud 65 68 +3

History

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

@Dosant Dosant requested a review from azasypkin August 7, 2023 13:34
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Changes in plugin_functional/test_suites/core_plugins/rendering.ts LGTM.

@Dosant Dosant merged commit 1047eef into elastic:main Aug 7, 2023
@kibanamachine kibanamachine added v8.10.0 backport:skip This commit does not require backporting labels Aug 7, 2023
bryce-b pushed a commit to bryce-b/kibana that referenced this pull request Aug 9, 2023
## Summary

close elastic#163014

- Changing `My Deployments -> Projects`
- Removing hardcoded url and passing one from the config
@Dosant Dosant self-assigned this Aug 10, 2023
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 Feature:Chrome Core's Chrome UI (sidenav, header, breadcrumbs) Project:Serverless Work as part of the Serverless project for its initial release release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Project Selection in Serverless Top Navigation
7 participants