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

Refactor Discovery Engine resource handling #384

Merged
merged 5 commits into from
Feb 7, 2025
Merged

Refactor Discovery Engine resource handling #384

merged 5 commits into from
Feb 7, 2025

Conversation

csutter
Copy link
Contributor

@csutter csutter commented Feb 7, 2025

We currently have separate environment variables (mirrored in app configuration) for the Discovery Engine data store, data store branch, engine, and serving config.

These all share a common ancestor (the default collection), so we don't need to set them all up separately.

This PR adopts the new data model from Search Admin in this application too, with first class models for each of the above Discovery Engine resources, and removes the previous configuration.

see alphagov/search-admin#1360

This adds a new Rails app configuration option for the default
collection, the root ancestor of all Discovery Engine resources. This
will allow us to construct fully qualified names (paths) for all
resources without having to configure several separate environment
variables for the datastore, the engine, the serving config, etc.
In the vein of "a little duplication is better than the wrong
abstraction", we probably want to avoid having a shared library for the
handful of relatively trivial shared data models between this app and
Search Admin.

- Copy `DataStore` and `Engine` models (and `DiscoveryEngineNameable`
  concern) from Search Admin into this app
This changes the usage of Rails configuration/environment variables for
getting the default data store to use `DataStore.default` instead.
This adds a `Branch` model and a representation of a data store's
default branch, allowing us to remove the more specific environment
variable/app configuration.

Branches are annoying because they are undocumented and don't formally
form part of the Discovery Engine API. Every data store has exactly one
branch (the default one), and you don't interact with it in any way
_other than_ when you add/update/remove documents on the datastore.
This adds a `ServingConfig` model and a representation of the existing
default serving configuration, allowing us to remove the more specific
environment variable/app configuration.
Copy link
Contributor

@leenagupte leenagupte left a comment

Choose a reason for hiding this comment

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

The commits are very nicely laid out. 🤩

@csutter csutter merged commit 9bc12ba into main Feb 7, 2025
8 checks passed
@csutter csutter deleted the models branch February 7, 2025 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants