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

Split search backend plugin initialization into init and start phases #24796

Merged
merged 6 commits into from
May 16, 2024

Conversation

brianphillips
Copy link
Contributor

Hey, I just made a Pull Request!

As discussed over in #24794, the backend search plugin needs to have two phases during initialization: build and start.

The build phase is synchronous and ensures that the searchIndexService state is initialized before creating the HTTP router. This "build" phase is necessary to ensure the collators are all added to the searchIndexService object before trying to use that list of document types within the zod request schema that is used to validate incoming requests. Prior to this change, the set of types would be empty at initialization time and incoming requests would be rejected due to validation errors.

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

Fixes backstage#24794. The build process takes place synchronously to the plugin
initialization rather than at startup so that the HTTP router has the
relevant types available to validate incoming requests.

Signed-off-by: Brian Phillips <28457+brianphillips@users.noreply.github.com>
Signed-off-by: Brian Phillips <28457+brianphillips@users.noreply.github.com>
@brianphillips brianphillips requested a review from a team as a code owner May 15, 2024 16:14
@github-actions github-actions bot added search Things related to Search area:discoverability Related to the Discoverability Project Area labels May 15, 2024
@backstage-goalie
Copy link
Contributor

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/plugin-search-backend-node plugins/search-backend-node patch v1.2.22
@backstage/plugin-search-backend plugins/search-backend patch v1.5.8

Signed-off-by: Brian Phillips <28457+brianphillips@users.noreply.github.com>
@Pike
Copy link
Contributor

Pike commented May 15, 2024

Tested this locally, and works. Also the code looks like what I expected.

CC @drodil @freben

PS: The PR state is weird? No reviewers assigned, no uffizi? Haven't looked at PRs in a bit, no idea if that's normal these days.

@freben freben requested review from a team, freben and Rugvip and removed request for a team May 15, 2024 17:49
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

Thank you! 🎉

Just API design nits

@backstage/discoverability-maintainers could you have a look too?

plugins/search-backend-node/api-report-alpha.md Outdated Show resolved Hide resolved
Copy link
Contributor

@tikabom tikabom left a comment

Choose a reason for hiding this comment

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

lgtm + agree with @Rugvip on his naming nits.

Copy link
Member

@emmaindal emmaindal left a comment

Choose a reason for hiding this comment

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

This makes sense to me, but I like the suggestion from @Rugvip about using init or initialize instead of build, so would be nice to get that updated!

Signed-off-by: Brian Phillips <28457+brianphillips@users.noreply.github.com>
@brianphillips brianphillips changed the title Split search backend plugin initialization into build and start phases Split search backend plugin initialization into init and start phases May 16, 2024
@brianphillips brianphillips requested a review from Rugvip May 16, 2024 13:28
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! 🎉

Co-authored-by: Emma Indal <emma.indahl@gmail.com>
Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>
Copy link
Member

@emmaindal emmaindal left a comment

Choose a reason for hiding this comment

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

🎉

@Rugvip Rugvip enabled auto-merge May 16, 2024 13:41
Copy link
Collaborator

@awanlin awanlin left a comment

Choose a reason for hiding this comment

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

Thanks for this @brianphillips, left one small nit but it's not blocking

plugins/search-backend-node/src/alpha.ts Outdated Show resolved Hide resolved
@Rugvip Rugvip disabled auto-merge May 16, 2024 13:42
Co-authored-by: Andre Wanlin <67169551+awanlin@users.noreply.github.com>
Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>
@Rugvip Rugvip merged commit 1c8a0cb into backstage:master May 16, 2024
30 checks passed
@brianphillips brianphillips deleted the search-backend-build-ordering branch May 16, 2024 14:01
Copy link
Contributor

Thank you for contributing to Backstage! The changes in this pull request will be part of the 1.28.0 release, scheduled for Tue, 18 Jun 2024.

Copy link
Contributor

Uffizzi Cluster pr-24796 was deleted.

benjdlambert added a commit that referenced this pull request May 16, 2024
Signed-off-by: blam <ben@blam.sh>
Rugvip added a commit that referenced this pull request May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:discoverability Related to the Discoverability Project Area search Things related to Search
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants