Skip to content
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

[Streams] Refactoring streams routes #206526

Merged

Conversation

simianhacker
Copy link
Member

@simianhacker simianhacker commented Jan 13, 2025

Summary

This PR consolidates the multiple server/streams/* route files into 4 route.ts files to optimize the Typescript parsing. I tried to organize the routes into 4 logical groups:

  • CRUD - edit, list, read, delete
  • Management - fork, resync, status, sample
  • Schema - unmapped fields, schema simulation
  • Enablement - disable, enable

I left everything else "as is" since @dgieselaar is currently doing a refactor to consolidate most of the features into a new StreamsClient similar to the AssetClient

@dgieselaar dgieselaar added the Feature:Streams This is the label for the Streams Project label Jan 14, 2025
@simianhacker simianhacker added release_note:skip Skip the PR/issue when compiling release notes v9.0.0 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Jan 14, 2025
@simianhacker simianhacker marked this pull request as ready for review January 14, 2025 16:02
@simianhacker simianhacker requested a review from a team as a code owner January 14, 2025 16:02
Copy link
Contributor

Choose a reason for hiding this comment

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

The simulateProcessorRoute registration has gone lost, might be good to move it to the management folder, wdyt?
Screenshot 2025-01-14 at 17 39 31

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops... I should have moved this back to "Draft". I just added those while you were reviewing it.

Copy link
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

@flash1293
Copy link
Contributor

This is gonna clash a bit with #206438 , but I'd argue it makes it better because Darios PR moves a bunch of logic out of the route handlers into the streams client, so the consolidated route.ts files get smaller.

I like the new separation, I would argue sample should go into "schema" as well (and maybe it should be called "processing" or so). With this, we already have pretty nice boundaries for splitting internal and external APIs.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

@simianhacker simianhacker merged commit 24f5153 into elastic:main Jan 16, 2025
9 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

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

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 16, 2025
## Summary

This PR consolidates the multiple `server/streams/*` route files into 4
`route.ts` files to optimize the Typescript parsing. I tried to organize
the routes into 4 logical groups:

- CRUD - edit, list, read, delete
- Management - fork, resync, status, sample
- Schema - unmapped fields, schema simulation
- Enablement - disable, enable

I left everything else "as is" since @dgieselaar is currently doing a
refactor to consolidate most of the features into a new `StreamsClient`
similar to the `AssetClient`

(cherry picked from commit 24f5153)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

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

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Jan 16, 2025
# Backport

This will backport the following commits from `main` to `8.x`:
- [[Streams] Refactoring streams routes
(#206526)](#206526)

<!--- Backport version: 9.4.3 -->

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

<!--BACKPORT [{"author":{"name":"Chris
Cowan","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-01-16T16:53:01Z","message":"[Streams]
Refactoring streams routes (#206526)\n\n## Summary\r\n\r\nThis PR
consolidates the multiple `server/streams/*` route files into
4\r\n`route.ts` files to optimize the Typescript parsing. I tried to
organize\r\nthe routes into 4 logical groups:\r\n\r\n- CRUD - edit,
list, read, delete\r\n- Management - fork, resync, status, sample\r\n-
Schema - unmapped fields, schema simulation\r\n- Enablement - disable,
enable\r\n\r\nI left everything else \"as is\" since @dgieselaar is
currently doing a\r\nrefactor to consolidate most of the features into a
new `StreamsClient`\r\nsimilar to the
`AssetClient`","sha":"24f5153d8505714cd953bdf7c529131a29c2c7ca","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor","Feature:Streams"],"title":"[Streams]
Refactoring streams
routes","number":206526,"url":"https://github.com/elastic/kibana/pull/206526","mergeCommit":{"message":"[Streams]
Refactoring streams routes (#206526)\n\n## Summary\r\n\r\nThis PR
consolidates the multiple `server/streams/*` route files into
4\r\n`route.ts` files to optimize the Typescript parsing. I tried to
organize\r\nthe routes into 4 logical groups:\r\n\r\n- CRUD - edit,
list, read, delete\r\n- Management - fork, resync, status, sample\r\n-
Schema - unmapped fields, schema simulation\r\n- Enablement - disable,
enable\r\n\r\nI left everything else \"as is\" since @dgieselaar is
currently doing a\r\nrefactor to consolidate most of the features into a
new `StreamsClient`\r\nsimilar to the
`AssetClient`","sha":"24f5153d8505714cd953bdf7c529131a29c2c7ca"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/206526","number":206526,"mergeCommit":{"message":"[Streams]
Refactoring streams routes (#206526)\n\n## Summary\r\n\r\nThis PR
consolidates the multiple `server/streams/*` route files into
4\r\n`route.ts` files to optimize the Typescript parsing. I tried to
organize\r\nthe routes into 4 logical groups:\r\n\r\n- CRUD - edit,
list, read, delete\r\n- Management - fork, resync, status, sample\r\n-
Schema - unmapped fields, schema simulation\r\n- Enablement - disable,
enable\r\n\r\nI left everything else \"as is\" since @dgieselaar is
currently doing a\r\nrefactor to consolidate most of the features into a
new `StreamsClient`\r\nsimilar to the
`AssetClient`","sha":"24f5153d8505714cd953bdf7c529131a29c2c7ca"}}]}]
BACKPORT-->

Co-authored-by: Chris Cowan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Streams This is the label for the Streams Project release_note:skip Skip the PR/issue when compiling release notes v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants