-
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
[share] hideEmbed serverless configuration to hide 'Embed code' share option in serverless #162354
Conversation
…-ref HEAD~1..HEAD --fix'
…-ref HEAD~1..HEAD --fix'
Pinging @elastic/kibana-presentation (Team:Presentation) |
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.
Changes in test/plugin_functional/test_suites/core_plugins/rendering.ts
LGTM. Left one optional suggestion.
import { schema, TypeOf } from '@kbn/config-schema'; | ||
|
||
export const configSchema = schema.object({ | ||
hideEmbed: schema.conditional( |
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.
note: Up to you, but I'm wondering if it'd be easier\cleaner to rely on env.buildFlavor
provided by the Core (both on the server and client side) instead of creating a new, technically immutable, config flag.
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.
It doesn't have to be a config value. You can expose an API on the start
contract and call it from the relevant serverless
plugin.
Flexing based on is(serverless)
means you lose context on related serverless decisions. It will be hard to parse out what code applies to a particular functional decision if everything is keyed off of that binary proposition.
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.
It will be hard to parse out what code applies to a particular functional decision if everything is keyed off of that binary proposition.
Well, it might sound good in theory, but I'm not sure it's the case in practice. We have discussed this a lot internally for the past few weeks, so sorry for lots of text 🙈
The things like share.hideEmbed
just add an unnecessary level of indirection and complexity around something that is only relevant to the Serverless offering. As far as I am aware, there is no valid use case for "hiding embed" controls, no such "feature" is on our roadmap, and might never be. We're just trying to come up with a workaround for the limitations we have in one specific offering, pretending that it's not just an "isServerless" flag. Moreover, we might lift some of these limitations eventually, making many of these "fake-future" flags unnecessary.
When I read the code and see share.hideEmbed
and friends, unlike isServerless
, the following questions pop up in my mind:
- What use case is this for?
- What consumers outside of this plugin rely on it?
- Are there any consumers at all?
- Can I safely change or remove it if I have to?
- Since it's exposed through a contract, can I rely on it in my other plugin? If not, why?
No matter how you expose it, via config or via plugin contracts, these are all public APIs that we're supposed to support backward compatibility for. It's just that we usually ignore breaking changes in our programmatic API contracts since we don't have many external plugins that could be affected by these changes.
In summary, what I want to say is that we have at least three different use cases when it comes to Serverless, and trying to solve them with one tool might not be the best approach:
-
Kibana functionality that should not be available or work differently in Serverless but represents a well-isolated "domain" that makes sense on its own, and there is a high chance that this functionality might be disabled/re-configured in other contexts/offerings in the future. This is the perfect use case for feature config flags and additional APIs on the setup/start contracts.
-
Kibana functionality that mostly depends on counterpart Elasticsearch functionality that should not be available or work differently in Serverless. In this case, Kibana shouldn't care about Serverless directly and ideally should rely on available Elasticsearch capabilities exposed in one way or another. If Elasticsearch doesn't yet expose a way to advertise certain capabilities, relying on
isServerless
in Kibana as an interim solution sounds like the best\fairest approach. Alternatively, we can leverage ES capabilities API exposed by the Core's Elasticsearch client Introducing the concept of ES capabilities #164850. -
Kibana functionality which customizations only make sense in the Serverless context. Unless we really can tick all the boxes from the option 1, I don't think we should pretend that it's a "feature" and just use the
isServerless
flag until we feel it's indeed a feature so that we can dropisServerles
s and expose a proper feature flag or programmatic feature management API.
Having said that, it's not my call here, just wanted to share my perspective!
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.
Kibana functionality which customizations only make sense in the Serverless context. Unless we really can tick all the boxes from the option 1, I don't think we should pretend that it's a "feature" and just use the isServerless flag until we feel it's indeed a feature so that we can drop isServerless and expose a proper feature flag or programmatic feature management API.
Is there an example that uses isServerless
flag that you can point me too? I have no strong opinions about implementation path.
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.
The env.buildFlavor
, provided by the Core (both on the server and client side), is a relatively new addition, but we're planning to use it like that on the server side https://github.com/elastic/kibana/pull/162087/files#diff-4fbb178a08b40c270265f939de2e735ba79ea1181ea5c1ebca18dd032fcfcf2eR98, and I see Maps is using this flag on the client side as well:
if (buildFlavor === 'traditional' && kbnSemVer) { | |
landingPageUrl = `${landingPageUrl}/v${kbnSemVer.major}.${kbnSemVer.minor}`; | |
} |
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.
Thanks for the examples.
As another data point, #161528, mentioned in AppEx updates 2023-08-02, uses a yaml configuration to disable functionality in serverless. I don't think there is a consistent approach across Kibana
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 would +1 to the yaml config option as it allows us to flip this switch regardless of serverless. I.e. we might want to hide this functionality in all offerings in the future.
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 don't think there is a consistent approach across Kibana
Yes, that's true, unfortunately. The timeline for this looks like this:
timeline
title The history of "Serverless" flag in Kibana
Beginning : We agreed to use configuration feature flags to toggle/adjust features in Serverless
Next : We realized that configuration flags are essentially public APIs and we decided to gate them with "serverless" configuration context reference
Next : People started doing crazy things with this config context reference just to emulate "isServerless" config flag, because they didn't want to invent "fake" features just to have flags for them
Next : In parallel, other people started doing other ...interesting... things with optional dependencies to the "serverless"-only plugins just, again, to emulate "isServerless" config flag
Next : We realized that there are "serverless"-only things that depend on Elasticsearch capabilities for which config feature flags don't make sense
Next : The Core team introduced the `env.buildFlavour` to stop that madness (opinion is my own) with the three previous points.
We're here : Now we have multiple different solutions, but no clear guidance for when to use which. All in all, it's much more straightforward to migrate from `env.buildFlavour` to any other solution in the future then vice versa.
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: |
Replacing with #163104. |
Closes #161266
Overview
PR adds
share.hideEmbed
configurationTest
yarn start --serverless=es