fix(providers/azure): wire Azure Search client into provider (closes #473 partially) - #560
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReclassifies Azure SearchClient and converted artifacts to use ChangesAzure Search Service Classification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
providers/azure/provider_test.go (1)
406-413:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude
common.ServiceNoSQLin the "all service types" test matrix.Line 412 adds Search, but this table-driven test still skips
common.ServiceNoSQL, soGetServiceClientNoSQL routing is not covered inTestAzureProvider_GetServiceClient_AllServiceTypes.Suggested patch
testCases := []struct { service common.ServiceType }{ {common.ServiceCompute}, {common.ServiceRelationalDB}, + {common.ServiceNoSQL}, {common.ServiceCache}, {common.ServiceSearch}, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/provider_test.go` around lines 406 - 413, The table-driven test TestAzureProvider_GetServiceClient_AllServiceTypes omits common.ServiceNoSQL from the testCases slice, so GetServiceClient's NoSQL routing is not exercised; update the testCases declaration (the slice of common.ServiceType) to include {common.ServiceNoSQL} alongside ServiceCompute, ServiceRelationalDB, ServiceCache, and ServiceSearch so the NoSQL branch is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@providers/azure/provider_test.go`:
- Around line 406-413: The table-driven test
TestAzureProvider_GetServiceClient_AllServiceTypes omits common.ServiceNoSQL
from the testCases slice, so GetServiceClient's NoSQL routing is not exercised;
update the testCases declaration (the slice of common.ServiceType) to include
{common.ServiceNoSQL} alongside ServiceCompute, ServiceRelationalDB,
ServiceCache, and ServiceSearch so the NoSQL branch is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a91fd524-c03b-4642-9554-64b61345c2fe
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
go.modproviders/azure/provider.goproviders/azure/provider_test.goproviders/azure/services.goproviders/azure/services/search/client.goproviders/azure/services/search/client_test.goproviders/azure/services_test.go
The table-driven test TestAzureProvider_GetServiceClient_AllServiceTypes
was missing common.ServiceNoSQL, leaving the GetServiceClient NoSQL
routing branch uncovered. Add {common.ServiceNoSQL} to testCases so
NewCosmosDBClient construction is verified alongside other service types.
Fixes CodeRabbit finding on PR #560.
CodeRabbit Review - Addressed
Commit: @coderabbitai review |
|
Triggering a fresh review of the latest changes now. ✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
The table-driven test TestAzureProvider_GetServiceClient_AllServiceTypes
was missing common.ServiceNoSQL, leaving the GetServiceClient NoSQL
routing branch uncovered. Add {common.ServiceNoSQL} to testCases so
NewCosmosDBClient construction is verified alongside other service types.
Fixes CodeRabbit finding on PR #560.
00e445a to
56306a5
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
The table-driven test TestAzureProvider_GetServiceClient_AllServiceTypes
was missing common.ServiceNoSQL, leaving the GetServiceClient NoSQL
routing branch uncovered. Add {common.ServiceNoSQL} to testCases so
NewCosmosDBClient construction is verified alongside other service types.
Fixes CodeRabbit finding on PR #560.
56306a5 to
0f78096
Compare
The table-driven test TestAzureProvider_GetServiceClient_AllServiceTypes
was missing common.ServiceNoSQL, leaving the GetServiceClient NoSQL
routing branch uncovered. Add {common.ServiceNoSQL} to testCases so
NewCosmosDBClient construction is verified alongside other service types.
Fixes CodeRabbit finding on PR #560.
|
Addressed the CodeRabbit finding on Finding: Fix: Added Additionally: Rebased onto Tests: @coderabbitai review |
0f78096 to
ced589e
Compare
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…473 partially) Add common.ServiceSearch to GetSupportedServices() and a matching case in GetServiceClient() that returns NewSearchClient(). Also add the NewSearchClient factory function to services.go and fix the pre-existing ServiceOther misclassification in the search client's GetServiceType(), convertSearchReservation(), and convertAzureSearchRecommendation() to return ServiceSearch. Update tests in provider_test.go, services_test.go, and services/search/client_test.go to cover the new wiring and correct service type. Add armsearch v1.4.0 to the root go.mod so the root module's build picks up the transitive dependency. Refs #473
The table-driven test TestAzureProvider_GetServiceClient_AllServiceTypes
was missing common.ServiceNoSQL, leaving the GetServiceClient NoSQL
routing branch uncovered. Add {common.ServiceNoSQL} to testCases so
NewCosmosDBClient construction is verified alongside other service types.
Fixes CodeRabbit finding on PR #560.
…t test
During rebase conflict resolution a second {common.ServiceNoSQL} entry was
introduced in TestAzureProvider_GetServiceClient_AllServiceTypes. Remove the
duplicate so each service type appears exactly once in the test matrix.
ced589e to
a4529bf
Compare
|
Rebased onto Verification:
New HEAD: @coderabbitai review |
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
providers/azure/provider_test.go (1)
1159-1168: 💤 Low valueReorder service types to match canonical order.
The service list has
ServiceCachebeforeServiceNoSQL, but the canonical order inGetSupportedServices()(and the order used inTestAzureProvider_GetServiceClient_AllServiceTypeson lines 414-419) isServiceNoSQLbeforeServiceCache. While this doesn't affect test correctness, maintaining consistent ordering across tests improves maintainability.♻️ Proposed reordering to match canonical order
services := []common.ServiceType{ common.ServiceCompute, common.ServiceRelationalDB, - common.ServiceCache, common.ServiceNoSQL, + common.ServiceCache, common.ServiceMemoryDB, common.ServiceSavingsPlans, common.ServiceSearch, common.ServiceDataWarehouse, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/provider_test.go` around lines 1159 - 1168, The services slice in the test is out of canonical order: swap common.ServiceCache and common.ServiceNoSQL so the slice order matches GetSupportedServices() and TestAzureProvider_GetServiceClient_AllServiceTypes; update the services literal used in the test (the variable named services) so common.ServiceNoSQL appears before common.ServiceCache.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@providers/azure/provider_test.go`:
- Around line 1159-1168: The services slice in the test is out of canonical
order: swap common.ServiceCache and common.ServiceNoSQL so the slice order
matches GetSupportedServices() and
TestAzureProvider_GetServiceClient_AllServiceTypes; update the services literal
used in the test (the variable named services) so common.ServiceNoSQL appears
before common.ServiceCache.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 959b4779-ded5-4f7c-8b7f-730fc9d8bf9c
📒 Files selected for processing (4)
providers/azure/provider_test.goproviders/azure/services/search/client.goproviders/azure/services/search/client_test.goproviders/azure/services_test.go
💤 Files with no reviewable changes (3)
- providers/azure/services/search/client.go
- providers/azure/services_test.go
- providers/azure/services/search/client_test.go
|
Addressed the canonical-order nit in commit @coderabbitai review |
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Already addressed in cbfb708: |
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
providers/azure/services/search/has been a fully-implemented Azure Cognitive Search reserved capacity client since it was first added, but it was unreachable becauseGetSupportedServices()did not listServiceSearchandGetServiceClient()had no matching case. This PR wires the client into the provider, making Azure Search visible to every caller that enumerates supported services or constructs service clients (the UI recommendations table, the CLI scan loop, and the purchase flow).What changed
providers/azure/provider.go: addedcommon.ServiceSearchtoGetSupportedServices()return slice; addedcase common.ServiceSearch: return NewSearchClient(...)toGetServiceClient().providers/azure/services.go: addedNewSearchClientfactory function and thesearchpackage import, mirroring the pattern used for compute/database/cache/cosmosdb.providers/azure/services/search/client.go: fixed pre-existing misclassification -GetServiceType(),convertSearchReservation(), andconvertAzureSearchRecommendation()all returned/setcommon.ServiceOtherinstead ofcommon.ServiceSearch.go.mod/go.sum: addedarmsearch v1.4.0to the root module so the root-levelgo build ./...resolves the transitive dependency that theservices.goimport now pulls in.providers/azure/provider_test.go: extendedTestAzureProvider_GetSupportedServicesandTestAzureProvider_GetServiceClient_AllServiceTypesto coverServiceSearch.providers/azure/services_test.go: addedTestNewSearchClientfactory test.providers/azure/services/search/client_test.go: updated two assertions that expectedServiceOtherto expectServiceSearch.Why
Feature parity gap: every other Azure service client (compute, database, cache, cosmosdb) is wired into the provider. The search client existed but was dead code because the dispatch tables did not reference it.
Test plan
cd providers/azure && go test ./... -count=1 -shortpasses (348 tests, all green)go build ./...from repo root passes with armsearch dep addedTestAzureProvider_GetSupportedServicesassertsServiceSearchis presentTestAzureProvider_GetServiceClient_AllServiceTypes/searchconstructs a non-nil clientTestNewSearchClientassertsGetServiceType() == ServiceSearchTestSearchClient_GetServiceTypeassertsServiceSearchRefs #473
Summary by CodeRabbit
Bug Fixes
Tests