Skip to content

[Streams 🌊] Ensure the members array is unique for GroupStreamDefinitions#210089

Merged
simianhacker merged 5 commits intoelastic:mainfrom
simianhacker:streams/member-uniqness-for-group-streams
Feb 19, 2025
Merged

[Streams 🌊] Ensure the members array is unique for GroupStreamDefinitions#210089
simianhacker merged 5 commits intoelastic:mainfrom
simianhacker:streams/member-uniqness-for-group-streams

Conversation

@simianhacker
Copy link
Copy Markdown
Member

@simianhacker simianhacker commented Feb 6, 2025

Summary

This PR ensures that the definition.group.members is a unique array of strings. I introduced a new private function to the StreamsClient called parseDefinition that will parse the definition being upserted with the runtime schemas to ensure they are properly formatted. This is also a good extension point for doing any transformations we need.

@simianhacker simianhacker marked this pull request as ready for review February 6, 2025 18:50
@simianhacker simianhacker requested a review from a team as a code owner February 6, 2025 18:50
@simianhacker simianhacker added v9.0.0 backport:version Backport to applied version labels v9.1.0 v8.19.0 release_note:fix Feature:Streams This is the label for the Streams Project labels Feb 6, 2025
Copy link
Copy Markdown
Contributor

@tonyghiani tonyghiani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the runtime parsing step necessary given all these data will be already schema validated on request?
I like having this extension point for transformations on a definition, but wouldn't be enough to have it as a normalizeDefinition and rely on the Typescript types to enforce check the structure?

Copy link
Copy Markdown
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @tonyghiani s comment that from that point Typescript should be enough to make sure the definition is proper.

Should we fail in this scenario instead of deduping? Not sure what makes more sense, happy to go either way really.

@simianhacker
Copy link
Copy Markdown
Member Author

@flash1293 @tonyghiani Will the client always be used by the HTTP API?

@tonyghiani
Copy link
Copy Markdown
Contributor

Will the client always be used by the HTTP API?

At the moment, it should. And even if we need to expose the client to other Kibana plugins by the start contract for server-side usage, they should adhere to the Typescript defined interface for the streams definition.

If it's not a requirement now, I'd rather skip this layer of complexity at runtime.

@flash1293
Copy link
Copy Markdown
Contributor

Yeah, I don't think the client needs to provide any correctness guarantees at this point - let's not worry about exposing it internally just yet. In general I feel @miltonhultgren is raising great points about how we can structure our validation code here: https://github.com/elastic/streams-program/discussions/147

@simianhacker
Copy link
Copy Markdown
Member Author

I'll move the transform and checks to the route then.

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

Copy link
Copy Markdown
Contributor

@tonyghiani tonyghiani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👌

@simianhacker simianhacker merged commit a28e400 into elastic:main Feb 19, 2025
@kibanamachine
Copy link
Copy Markdown
Contributor

Starting backport for target branches: 8.x, 9.0

https://github.com/elastic/kibana/actions/runs/13416447559

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 19, 2025
…ions (elastic#210089)

## Summary

This PR ensures that the `definition.group.members` is a unique array of
strings. I introduced a new private function to the StreamsClient called
`parseDefinition` that will parse the definition being upserted with the
runtime schemas to ensure they are properly formatted. This is also a
good extension point for doing any transformations we need.

(cherry picked from commit a28e400)
@kibanamachine
Copy link
Copy Markdown
Contributor

💔 Some backports could not be created

Status Branch Result
8.x
9.0 Backport failed because of merge conflicts

Note: Successful backport PRs will be merged automatically after passing CI.

Manual backport

To create the backport manually run:

node scripts/backport --pr 210089

Questions ?

Please refer to the Backport tool documentation

@flash1293
Copy link
Copy Markdown
Contributor

@simianhacker as 9.0 feature-froze already, we shouldn't backport any additional streams changes at this point.

kibanamachine added a commit that referenced this pull request Feb 20, 2025
…efinitions (#210089) (#211798)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Streams 🌊] Ensure the members array is unique for
GroupStreamDefinitions
(#210089)](#210089)

<!--- Backport version: 9.6.6 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

<!--BACKPORT [{"author":{"name":"Chris
Cowan","email":"chris@elastic.co"},"sourceCommit":{"committedDate":"2025-02-19T15:37:56Z","message":"[Streams
🌊] Ensure the members array is unique for GroupStreamDefinitions
(#210089)\n\n## Summary\n\nThis PR ensures that the
`definition.group.members` is a unique array of\nstrings. I introduced a
new private function to the StreamsClient called\n`parseDefinition` that
will parse the definition being upserted with the\nruntime schemas to
ensure they are properly formatted. This is also a\ngood extension point
for doing any transformations we
need.","sha":"a28e40069fd96ad82bbbde14479b85dc21f88370","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","v9.0.0","backport:version","Feature:Streams","v9.1.0","v8.19.0"],"title":"[Streams
🌊] Ensure the members array is unique for
GroupStreamDefinitions","number":210089,"url":"https://github.com/elastic/kibana/pull/210089","mergeCommit":{"message":"[Streams
🌊] Ensure the members array is unique for GroupStreamDefinitions
(#210089)\n\n## Summary\n\nThis PR ensures that the
`definition.group.members` is a unique array of\nstrings. I introduced a
new private function to the StreamsClient called\n`parseDefinition` that
will parse the definition being upserted with the\nruntime schemas to
ensure they are properly formatted. This is also a\ngood extension point
for doing any transformations we
need.","sha":"a28e40069fd96ad82bbbde14479b85dc21f88370"}},"sourceBranch":"main","suggestedTargetBranches":["9.0","8.x"],"targetPullRequestStates":[{"branch":"9.0","label":"v9.0.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/210089","number":210089,"mergeCommit":{"message":"[Streams
🌊] Ensure the members array is unique for GroupStreamDefinitions
(#210089)\n\n## Summary\n\nThis PR ensures that the
`definition.group.members` is a unique array of\nstrings. I introduced a
new private function to the StreamsClient called\n`parseDefinition` that
will parse the definition being upserted with the\nruntime schemas to
ensure they are properly formatted. This is also a\ngood extension point
for doing any transformations we
need.","sha":"a28e40069fd96ad82bbbde14479b85dc21f88370"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Chris Cowan <chris@elastic.co>
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Mar 22, 2025
…ions (elastic#210089)

## Summary

This PR ensures that the `definition.group.members` is a unique array of
strings. I introduced a new private function to the StreamsClient called
`parseDefinition` that will parse the definition being upserted with the
runtime schemas to ensure they are properly formatted. This is also a
good extension point for doing any transformations we need.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:version Backport to applied version labels Feature:Streams This is the label for the Streams Project release_note:fix v8.19.0 v9.0.0 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants