-
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
Closed
Closed
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
bdabc48
[share] hideEmbed serverless to hide 'Embed code' share option in ser…
nreese 9e4d8d4
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine 613507b
fix unit test
nreese f857aa7
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine d89aa82
fix expects
nreese 6830c77
Merge branch 'main' into issue_161266
kibanamachine File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
import { schema, TypeOf } from '@kbn/config-schema'; | ||
|
||
export const configSchema = schema.object({ | ||
hideEmbed: schema.conditional( | ||
schema.contextRef('serverless'), | ||
true, | ||
schema.maybe(schema.boolean({ defaultValue: false })), | ||
schema.never() | ||
), | ||
}); | ||
|
||
export type ShareConfig = TypeOf<typeof configSchema>; | ||
|
||
export interface SharePublicConfig { | ||
hideEmbed?: boolean; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 relevantserverless
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.
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, unlikeisServerless
, the following questions pop up in my mind: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.
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:kibana/src/plugins/maps_ems/public/lazy_load_bundle/create_ems_client.ts
Lines 24 to 26 in 490c34c
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.
Yes, that's true, unfortunately. The timeline for this looks like this: