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.

Sign up for GitHub

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

Jump to bottom

[Enterprise Search] Refactor product server route registrations to their own files/folders #82663

Merged
merged 4 commits into from
Nov 5, 2020
Merged

[Enterprise Search] Refactor product server route registrations to their own files/folders #82663

merged 4 commits into from
Nov 5, 2020

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Nov 4, 2020 •
edited
Loading

Summary

I was adding a new Engine server route to App Search and noticed our server/plugin.ts was getting a little bloated/unwieldy. I decided to refactor/move all App Search and Workplace Search server route registrations to their own individual files (e.g. server/routes/index.ts) rather than being at the top level of server/plugin.ts.

Benefits:

  1. Our import list at the top of plugin.ts doesn't grow larger and larger, making the actual file/logic harder to read
  2. Products can scale to any size without affecting one another
  3. No need to namespace route registrations internally within the product folders (48019ca)

Checklist

@cee-chen cee-chen added Feature:Plugins backport:skip This commit does not require backporting v7.11.0 labels Nov 4, 2020
@cee-chen cee-chen requested review from JasonStoltz, scottybollinger and a team November 4, 2020 21:36
cee-chen
cee-chen commented Nov 4, 2020
View reviewed changes
x-pack/plugins/enterprise_search/server/routes/workplace_search/groups.ts Outdated
Comment on lines 248 to 251
export function registerGroupRoutes(dependencies: IRouteDependencies) {
registerGroupsRoute(dependencies);
registerSearchGroupsRoute(dependencies);
registerGroupRoute(dependencies);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scottybollinger I wasn't sure if you wanted to move all these registrations to the new index.ts or keep them grouped in this file for simplicity - let me know if you have a preference and I can make that change in this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Moving it is fine. Thanks! Will definitely have an effect on #82669 but I can make changes after we merge this or vice versa.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sweet, thanks Scotty! I'll move them here shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved in ac5ddaa - hope I got it right, feel free to tweak further in #82669 if I didn't

@cee-chen cee-chen added release_note:skip Skip the PR/issue when compiling release notes and removed backport:skip This commit does not require backporting labels Nov 4, 2020
cee-chen
cee-chen commented Nov 4, 2020
View reviewed changes
x-pack/plugins/enterprise_search/server/routes/app_search/index.test.ts
import { registerAppSearchRoutes } from './';

describe('registerAppSearchRoutes', () => {
it('runs without errors', () => {
Copy link
Contributor Author

@cee-chen cee-chen Nov 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I def know these aren't great tests - it basically just confirms that the parent fn runs w/o assertions, which is not my favorite but in this case I'm not sure it's worth the LOE to expand them further since:

  1. we weren't testing route registration calls originally anyway in plugin.ts
  2. In theory each individual registerXRoute should have its own fleshed out unit testing in any case

Copy link
Contributor

@scottybollinger scottybollinger Nov 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I ran into this same issue with #82669 and just replaced the first test in that file with the registerWSGroupRoutes and coverage passed but good to know we can do this for coverage purposes.

scottybollinger
scottybollinger approved these changes Nov 4, 2020
View reviewed changes
Copy link
Contributor

@scottybollinger scottybollinger left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for doing this!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

distributable file count

id before after diff
default 42717 42719 +2

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@cee-chen cee-chen merged commit b264498 into elastic:master Nov 5, 2020
@cee-chen cee-chen deleted the server-routes branch November 5, 2020 01:43
cee-chen pushed a commit to cee-chen/kibana that referenced this pull request Nov 5, 2020
@cee-chen
[Enterprise Search] Refactor product server route registrations to th…
ec5981f 
…eir own files/folders (elastic#82663)

* Refactor all AS & WS route registrations to their own file/folders

* Remove WS in route registrations since the main fn is already specific

* Cover index.ts files

- super basic, leaves out assertions to avoid brittleness

* Move WS group routes to index.ts
@cee-chen cee-chen mentioned this pull request Nov 5, 2020
Merged
cee-chen pushed a commit that referenced this pull request Nov 5, 2020
[Enterprise Search] Refactor product server route registrations to th…
08e7728 
…eir own files/folders (#82663) (#82680)

* Refactor all AS & WS route registrations to their own file/folders

* Remove WS in route registrations since the main fn is already specific

* Cover index.ts files

- super basic, leaves out assertions to avoid brittleness

* Move WS group routes to index.ts
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 5, 2020
@gmmorris
Merge branch 'master' into alerts/assign-action-groups
2730301 
* master: (127 commits)
  [ILM] Fix breadcrumbs (elastic#82594)
  [UX]Swap env filter with percentile (elastic#82246)
  Add platform's missing READMEs (elastic#82268)
  [Discover] Adding uiMetric to track Visualize link click (elastic#82344)
  [Search] Add used index pattern name to the search agg error field (elastic#82604)
  improve client-side SO client get pooling (elastic#82603)
  [Security Solution] Unskips Overview tests (elastic#82459)
  Embeddables/migrations (elastic#82296)
  [Enterprise Search] Refactor product server route registrations to their own files/folders (elastic#82663)
  Moving reinstall function outside of promise.all (elastic#82672)
  Load choropleth layer correctly (elastic#82628)
  Master  backport elastic#81233 (elastic#82642)
  [Fleet] Allow snake cased Kibana assets (elastic#77515)
  Reduce saved objects authorization checks (elastic#82204)
  [data.search] Add request handler context and asScoped pattern (elastic#80775)
  [ML] Fixes formatting of fields in index data visualizer (elastic#82593)
  Usage collector readme (elastic#82548)
  [Lens] Visualization validation and better error messages (elastic#81439)
  [ML] Add annotation markers to time series brush area to indicate annotations exist outside of selected range (elastic#81490)
  chore(NA): install microdnf in UBI docker build only (elastic#82611)
  ...
@scottybollinger scottybollinger mentioned this pull request Nov 9, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottybollinger scottybollinger scottybollinger approved these changes

@JasonStoltz JasonStoltz Awaiting requested review from JasonStoltz

Assignees
No one assigned
Labels
Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.11.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cee-chen @kibanamachine @scottybollinger