Refactor handling of non-managed resources #1360
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These are plain Ruby objects representing the concepts of engine and data store on Discovery Engine. Our architecture only has a single one of each, and that is unlikely to change in the medium term, so these are not persisted in the database but rather just have a single default instance available as `.default`.
This changes `Control` and its actions to use the new models instead of getting their engine and data store names straight from the Rails config.
This removes the unnecessary configuration of a data store, and also removes the erroneous test environment configuration for engine that was left in as part of a previous PR.
This refactors the repetitive name generation into a `DiscoveryEngineNameable` mixin.
This could come in helpful for debugging.
leenagupte
reviewed
Feb 6, 2025
This is a more sensible name that doesn't cause confusion around "engines".
leenagupte
approved these changes
Feb 6, 2025
| # parent resource is not the default collection. | ||
| module DiscoveryEngineNameable | ||
| # The name (fully qualified path) of this Discovery Engine resource on GCP | ||
| def name | ||
| [parent_name, resource_path_fragment, discovery_engine_id].join("/") | ||
| [parent_name, resource_path_fragment, remote_resource_id].join("/") |
There was a problem hiding this comment.
This is a much better name. Thanks for updating! ⭐
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We currently need the GCP "name" (actually a path/unique identifier!) for two resources that aren't managed by Search Admin:
Controls need the name of our default engine (as they are created as child resources on the engine)Control::BoostActions andControl::FilterActions need the name of our default data store (as that is the data store they are active against).So far we have been setting separate environment variables for the data store and engine on the app (one of these was missing in a previous PR causing a bug). But both of these share a common parent (the GCP project's default collection), so we only really need to configure that and infer the data store and engine name based on that.
This PR:
DataStoreandEngineas classes, with a single default instance, and uses those inControland its actionsDiscoveryNameablemixinScreenshots