Expose projectsUrl and baseUrl on the Cloud's plugin setup and start server-side contracts
#163380
Conversation
jloleysens
added
Team:Core
|
Pinging @elastic/kibana-core (Team:Core) |
| /** | ||
| * The full URL to the serverless projects. | ||
| */ | ||
| projectsUrl?: string; |
There was a problem hiding this comment.
Looking at this naming I'm not convinced it is 100% correct as we default it to a path, not a URL in our serverless config (the typescript, not YAML) 🤔 , will it actually be a full URL in our serverless configuration? CC @Dosant
There was a problem hiding this comment.
I'm wondering if these should go under serverless property, provided that they are serverless specific.
There was a problem hiding this comment.
Yeah I was also thinking that... It's currently not scoped that way in the cloud plugins' config.ts as there are a number of URL values not scoped to serverless key either... I think it may already be in use too so I figured we'd mirror that structure in our contract.
Don't feel strongly about it, so I'll go with your opinion if you disagree with my reasoning?
There was a problem hiding this comment.
Looked at each of the 2:
baseUrl: I'm fine with this one being top level. It exists for more than 2 years and does not seem to be serverless specific. Perhaps we could update the description in the config object, so that both match.projectsUrl: Added yesterday through this PR. This one seems to be serverless specific, and since 8.10.0 hasn't been released yet, perhaps we can challenge the fact that it is not under the serverless object. CC @Dosant WDYT? BTW the property is not present in theREADME.md.
There was a problem hiding this comment.
BTW the property is not present in the README.md.
sidenote on the readme: seems like we can capture this info in one place: the TS interface itself 😅
There was a problem hiding this comment.
projectsUrl: Added yesterday through this PR. This one seems to be serverless specific, and since 8.10.0 hasn't been released yet, perhaps we can challenge the fact that it is not under the serverless object. CC @Dosant WDYT? BTW the property is not present in the README.md.
I don't mind, your call.
We will also need to update it in the project-controller.
The reason I didn't put it under serverless initially called out here:
https://github.com/elastic/kibana/pull/163076/files#r1283288347
I wanted to have the default value and be able to test it locally, but to add serverless.* config we require to also add serverless.projectid. So I couldn't just add serverless.project_url for testing without adding serverless.projectid
it also seemed fine to follow other URLs for simplicity. also I can imagine we might need projects_url in the future in non-project deployments
There was a problem hiding this comment.
I'm not sure what the purpose of the project_url is, but by the name + description it would seem that it is exclusive to serverless.
FWIW Pierre made the project_id nullable, but still, if you want to test locally I'm afraid you'll need the project_id anyway, as it is this very property that we rely on to determine whether isServerlessEnabled:
https://github.com/elastic/kibana/blob/main/x-pack/plugins/cloud/server/plugin.ts#L112
There was a problem hiding this comment.
TBH I can see @Dosant reasoning, one could argue this is under the concern of Cloud (since it is a cloud URL) I am happy with it living outside serverless -- i.e., leave it as it is now.
(also already being used in project controller)
Let's call it here and commit to placing any future values inside of serverless but leave cloud URLs where they are. WDYT @Dosant @gsoldevila ? I'll capture the decision in a comment on the interface.
There was a problem hiding this comment.
My bad, my brain read projectUrl (singular) as if it was a URL of a single project.
That's why for me it was obvious that it should go under serverless config.
jloleysens
changed the title
projectsUrl and baseUrl on the Cloud's plugin setup and start server-side contracts
| The `cloud` plugin adds Cloud-specific features to Kibana. |
There was a problem hiding this comment.
This is just a chore I decided to include here: move all of these definitions onto the TS interfaces where they should be more useful and easier to keep in sync (i.e., be deleted or added when the code changes).
| export const cloudMock = { | ||
| createSetup: createSetupMock, | ||
| createStart: createStartMock, |
There was a problem hiding this comment.
Expose this missing mock for consumers of the cloud plugin
* main: (108 commits) [Telemetry Schema Validation] Allow `null` on `string` (elastic#163499) [Search] Add Slack and Gmail connectors (elastic#163321) [ML] Provide hints for empty fields in dropdown options in Anomaly detection & Transform creation wizards, Change point detection view (elastic#163371) chore(slo): Add response required fields (elastic#163430) [AO] Fix add_to_case functional test (elastic#163155) unskip license type functional test (elastic#163199) fix(NA): yarn env vars for node_modules mirrors (elastic#163549) [Response Ops][Task Manager] Expose SLI metrics in HTTP API (elastic#162178) [Logs UI] Adapt test to ES highlighting changes and unskip (elastic#163592) [Infra UI] Implement Telemetry on 'Show' buttons within Inventory (elastic#163587) [Enterprise Search]Migrate all usages of EuiPage*_Deprecated (elastic#163482) fix(slo): settings and access for serverless (elastic#163514) [Infra UI] Implement telemetry for the asset details flyout (elastic#163078) [Fleet] Add a banner to the top of the Kafka Output UI to say that Elastic Defend integration is not supported (elastic#163579) [Fleet] Re-enable and fix Fleet policy secret integration tests (elastic#163428) [Fleet] add managed to imported saved object (elastic#163526) [Index Management] Disable index actions using contextRef (elastic#163475) [Discover] Inline shard failures warnings (elastic#161271) [Security Solution][Detection engine] skips geo_point non-ecs validation (elastic#163487) Update EUI layout components in bfetch example plugin (elastic#163490) ...
💚 Build Succeeded
Metrics [docs]History
To update your PR or re-run it, just comment with: |
Close #163379