Support matched images for a spec#2095
Merged
cb-github-robot merged 1 commit intocloud-barista:mainfrom Aug 14, 2025
Merged
Conversation
Signed-off-by: seokho-son <shsongist@gmail.com>
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for matched spec-based image filtering, allowing users to search for images that are compatible with a specific compute specification. The changes introduce a new matchedSpecId parameter to the image search API that automatically applies spec constraints (provider, region, architecture) and CSP-specific filtering rules.
Key changes:
- Added
matchedSpecIdfield to image search request models and API documentation - Implemented spec-based filtering logic that extracts provider, region, and architecture from specs
- Added CSP-specific filtering for NCP provider using CorrespondingImageIds
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/interface/rest/docs/swagger.yaml | Updated API documentation to include matchedSpecId parameter |
| src/interface/rest/docs/swagger.json | Updated JSON API documentation with matchedSpecId field |
| src/interface/rest/docs/docs.go | Updated generated Go documentation with matchedSpecId support |
| src/core/resource/image.go | Implemented core logic for spec-based image filtering and CSP-specific rules |
| src/core/model/spec.go | Updated spec ID example format to use new convention |
| src/core/model/image.go | Added matchedSpecId fields to search request models |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| req.RegionName = specInfo.RegionName | ||
| log.Debug().Msgf("Applied RegionName from spec: %s", req.RegionName) | ||
| } | ||
| if specInfo.Architecture != "" && specInfo.Architecture != string(model.ArchitectureNA) { |
There was a problem hiding this comment.
The spec filtering logic overwrites existing request parameters without checking if they were already set by the user. Consider preserving user-provided values or documenting this behavior clearly.
Suggested change
| if specInfo.Architecture != "" && specInfo.Architecture != string(model.ArchitectureNA) { | |
| if specInfo.ProviderName != "" && req.ProviderName == "" { | |
| req.ProviderName = specInfo.ProviderName | |
| log.Debug().Msgf("Applied ProviderName from spec: %s", req.ProviderName) | |
| } | |
| if specInfo.RegionName != "" && req.RegionName == "" { | |
| req.RegionName = specInfo.RegionName | |
| log.Debug().Msgf("Applied RegionName from spec: %s", req.RegionName) | |
| } | |
| if specInfo.Architecture != "" && specInfo.Architecture != string(model.ArchitectureNA) && | |
| (req.OSArchitecture == "" || req.OSArchitecture == model.OSArchitecture(model.ArchitectureNA)) { |
| default: | ||
| // No specific filtering for other CSPs | ||
| return images | ||
| } |
There was a problem hiding this comment.
[nitpick] The switch statement only handles NCP with commented placeholders for other CSPs. Consider using a more extensible pattern like a registry or interface-based approach for CSP-specific filtering rules.
Suggested change
| } | |
| // cspImageFilterFunc defines the function signature for CSP-specific image filtering | |
| type cspImageFilterFunc func([]model.TbImageInfo, model.TbSpecInfo) []model.TbImageInfo | |
| // cspImageFilterRegistry maps provider names to their filtering functions | |
| var cspImageFilterRegistry = map[string]cspImageFilterFunc{ | |
| strings.ToLower(csp.NCP): filterImagesByCorrespondingIds, | |
| // strings.ToLower("aws"): filterImagesByHypervisor, // Placeholder for AWS | |
| // strings.ToLower("azure"): filterImagesByGeneration, // Placeholder for Azure | |
| // Add more CSP-specific filters here as needed | |
| } | |
| // applyCspSpecificImageFiltering applies CSP-specific filtering rules based on spec information | |
| func applyCspSpecificImageFiltering(images []model.TbImageInfo, specInfo model.TbSpecInfo) []model.TbImageInfo { | |
| provider := strings.ToLower(specInfo.ProviderName) | |
| if filterFunc, ok := cspImageFilterRegistry[provider]; ok { | |
| return filterFunc(images, specInfo) | |
| } | |
| // No specific filtering for other CSPs | |
| return images |
| log.Info().Msgf("CorrespondingIds filtering: %d corresponding image IDs found, filtered from %d to %d images for provider %s", | ||
| len(correspondingIds), len(images), len(filteredImages), specInfo.ProviderName) | ||
|
|
||
| return filteredImages |
There was a problem hiding this comment.
The filtering logic iterates through all images to match against the validImageIds map. For large image datasets, consider pre-filtering at the database query level using an IN clause with the corresponding image IDs.
Suggested change
| return filteredImages | |
| func applyCspSpecificImageFiltering(db *gorm.DB, specInfo model.TbSpecInfo) ([]model.TbImageInfo, error) { | |
| switch strings.ToLower(specInfo.ProviderName) { | |
| case csp.NCP: | |
| return filterImagesByCorrespondingIds(db, specInfo) | |
| // Add more CSP-specific filtering logic here as needed | |
| // case "aws": | |
| // return filterImagesByHypervisor(db, specInfo) | |
| // case "azure": | |
| // return filterImagesByGeneration(db, specInfo) | |
| default: | |
| // No specific filtering for other CSPs | |
| var images []model.TbImageInfo | |
| if err := db.Find(&images).Error; err != nil { | |
| return nil, err | |
| } | |
| return images, nil | |
| } | |
| } | |
| // filterImagesByCorrespondingIds filters images based on CorrespondingImageIds from spec details using DB query | |
| func filterImagesByCorrespondingIds(db *gorm.DB, specInfo model.TbSpecInfo) ([]model.TbImageInfo, error) { | |
| // Find CorrespondingImageIds from spec details | |
| correspondingIds := extractCorrespondingImageIds(specInfo.Details) | |
| if len(correspondingIds) == 0 { | |
| log.Warn().Msgf("No CorrespondingImageIds found in spec %s for provider %s", specInfo.Id, specInfo.ProviderName) | |
| var images []model.TbImageInfo | |
| if err := db.Find(&images).Error; err != nil { | |
| return nil, err | |
| } | |
| return images, nil | |
| } | |
| var filteredImages []model.TbImageInfo | |
| if err := db.Where("csp_image_name IN ?", correspondingIds).Find(&filteredImages).Error; err != nil { | |
| return nil, err | |
| } | |
| log.Info().Msgf("CorrespondingIds filtering: %d corresponding image IDs found, filtered to %d images for provider %s", | |
| len(correspondingIds), len(filteredImages), specInfo.ProviderName) | |
| return filteredImages, nil |
| "azure+koreacentral+Standard_B1s", | ||
| "gcp+asia-northeast3+e2-micro", | ||
| "ncpvpc+kr+m8-g3a", | ||
| } |
There was a problem hiding this comment.
[nitpick] The fallback hardcoded examples may become outdated. Consider storing these fallback values in a configuration file or constant to make them easier to maintain.
Suggested change
| } | |
| options.MatchedSpecId = fallbackMatchedSpecIds |
Member
Author
|
/approve |
This file contains hidden or 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
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.
No description provided.