-
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
[Logs+] Enforce dataset names #166654
[Logs+] Enforce dataset names #166654
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
aef73d8
to
51d5376
Compare
51d5376
to
48e146f
Compare
Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI) |
Pinging @elastic/fleet (Team:Fleet) |
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
return pipe( | ||
[context, event] as ValuesTuple, | ||
updateFields(context), | ||
updateTouchedFields(context), | ||
maybeMatchDatasetNameToIntegrationName(context), | ||
replaceSpecialCharacters(context) | ||
); |
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.
👏
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.
Good job, it works well and code 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.
Fleet changes look good 🚀
import type { CustomPackageDatasetConfiguration } from '../../install'; | ||
|
||
// Dataset name must either match integration name exactly OR be prefixed with integration name and a dot. | ||
export const checkDatasetsNameFormat = ( |
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 think this function is a good candidate for unit testing :)
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 🧙🏼♀️
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @Kerry350 |
`OBSERVABILITY_ONBOARDING_LOCATOR` and `ObservabilityOnboardingLocatorParams` were removed from `observability_onboarding` plugin in [#165962](#165962) and are now part of `@kbn/deeplinks-observability/locators`. `datsetName` was transformed into an optional variable in the wizard state in [#166654](#166654) which makes sense for first step of custom logs onboarding but it's required in onboarding saved object.
Summary
Closes #163830
This adds server side validation to enforce dataset name format rules for custom integrations. It then enhances the custom integrations Kibana package to handle this seamlessly in the create form.
There is no client side validation for the rules per se because as long as the dataset name passes other validations (length etc) then it is always valid - it just comes down to whether it's prefixed or not.
Other notes
UI / UX changes