-
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
[RAM] [Rule Form v2] Add feature flag #179184
Conversation
…flag # Conflicts: # x-pack/plugins/ml/public/application/app.tsx # x-pack/plugins/ml/public/application/contexts/ml/serverless_context.tsx
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
Pinging @elastic/response-ops (Team:ResponseOps) |
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.
Uptime changes LGTM
@@ -98,6 +100,7 @@ export async function mountManagementSection( | |||
|
|||
const enabledFeatures: TransformEnabledFeatures = { | |||
showNodeInfo: !isServerless, | |||
ruleFormV2Enabled: experimentalFeatures?.ruleFormV2Enabled ?? false, | |||
}; | |||
const unmountAppCallback = renderApp(element, appDependencies, enabledFeatures); |
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.
Not a super strong opinion on this one but I wonder if it will be more clear to continue passing experimentalFeatures
to renderApp(element, appDependencies, enabledFeatures, experimentalFeatures);
just like how it's done ML. This way the parameters passed down to are consistent, and also for clarity that it's an experimental feature. Just a thought, not a blocker.
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.
Sounds good, pushing a change
Tested ML & Transform changes and LGTM |
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.
ResponseOps changes LGTM
/ci |
@@ -354,6 +361,7 @@ export class DiscoverPlugin | |||
customizationContext: { | |||
displayMode: 'standalone', | |||
inlineTopNav: this.inlineTopNav, | |||
ruleFormV2Enabled: this.experimentalFeatures?.ruleFormV2Enabled ?? false, |
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.
Rather than incorporating this into the customization context, which is used for a different purpose in Discover, I'd prefer we pass ExperimentalFeatures
directly into renderApp
instead to keep things separated. Then when we need to use it, we can introduce something like a context provider for it.
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.
Sure thing, changed this in the latest commit
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.
Change to infra
config LGTM.
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.
Just have a small nitpick, otherwise LGTM
experimentalFeatures: ExperimentalFeatures, | ||
config: ConfigSchema | ||
) { | ||
if (config.experimental?.ruleFormV2?.enabled !== undefined) { |
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.
super small nitpick: maybe better to check if its a boolean?
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.
Trying to repeat the same pattern used elsewhere in this file
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.
Code-only review. Data Discovery changes LGTM
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 security changes LGTM
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.
@Zacqary I think we also need a feature flag for Observability "Create rule" form. wdyt?
@benakansara This same feature flag will apply to both the full page Create Rule form and in-context flyouts. For creating new rules from the Rules Management page, or editing existing ones, we'll be using this full-page experience: #179105 We'll also be integrating the new UI into flyouts, which we'll be using when the user creates a new rule from the upper-right dropdown from apps like Inventory, Metrics Explorer, etc: #179108 Both of those flows are part of the V2 project, so once they're both implemented, we want to be able to turn them both on or off together. |
@Zacqary thanks for the explanation. To confirm, correct me if I am wrong - The |
@benakansara I'd love to use just the Using this single feature flag would work if we were still sticking with the V1 method of embedding the rule form in solutions: exporting it from |
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services) |
As per discussion with @benakansara on Slack, added feature flags for |
const { appMountParameters } = usePluginContext(); | ||
|
||
const { appMountParameters, experimentalFeatures } = usePluginContext(); | ||
console.log('EXPERIMENTAL', experimentalFeatures); |
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.
could we remove console.log
?
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.
LGTM. Just left a comment about console.log
.
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
Summary
Closes #179110
This adds a series of feature flags to all plugins that are currently using the Rule Form Flyout, in preparation for development of the new Create Rule Flow V2 (#175634).
Unfortunately true Kibana feature-flagging is still not yet implemented, so we need to implement this as a config option for every individual affected plugin. The following
kibana.yml
config will enable the upcoming V2 rule form:These feature flags will not enable anything yet. This PR is for the sole purpose of adding these browser-enabled flags now, so that we can reduce the amount of code committed later.
Why feature flag every plugin?
The V1 rule form is currently exported from Triggers Actions UI. In V2, we're moving away from this model and exporting the rule form in a KBN package. Therefore we can't simply export the results of a single feature flag from Triggers Actions UI, we need to signal to each individual plugin whether it should be pulling from the new KBN package or from the old cross-plugin export.
Plugins affected
Triggers Actions UI, Infra, Observability, APM
These plugins already contained interfaces for experimental features or feature flags, so this PR simply adds a new flag to these existing structures.
Discover, ML, Transform, Uptime, SLO
These plugins did not have existing browser-exposed feature flag systems, so this PR adds these interfaces. The transform plugin notably did not yet have any config options configured at all.