fix(scheduler): extend ambient host-account UUID tagging to Azure/GCP - #725
Conversation
Issue #662: Azure and GCP lacked an ambient-credential fallback path when no cloud accounts were registered. Unlike AWS (which PR #607 already fixed), they silently returned empty rather than falling back to the host identity. This commit extends the #607 pattern to both providers: - Adds resolveAmbientAccountID: a provider-agnostic helper that looks up (provider, externalID) in cloud_accounts and returns the UUID, or "" for truly-orphan deployments. Errors are swallowed (logged at WARN) so a DB blip never fails the collection. - Adds collectAzureAmbient: ambient path via providerFactory for Azure, accepting the subscription ID explicitly to skip the API round-trip that would otherwise auto-discover subscriptions. - Adds collectGCPAmbient: ambient path via providerFactory for GCP (ADC). - Extends collectAzureRecommendations: when no accounts are registered and AZURE_SUBSCRIPTION_ID is set, falls back to ambient credentials and tags recs with the registered subscription UUID if found in cloud_accounts. - Extends collectGCPRecommendations: same pattern using GCP_PROJECT_ID. Eight new tests covering: happy path (UUID tagged), no registered row (nil preserved), store error (nil preserved, collection succeeds), env var absent (skips gracefully), plus two unit tests for resolveAmbientAccountID itself. Closes #662
|
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)
📝 WalkthroughWalkthroughThe PR extends ambient-path host-account UUID tagging to Azure and GCP collectors, mirroring the fix applied to AWS in PR ChangesAmbient account tagging for Azure and GCP
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@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)
internal/scheduler/scheduler_test.go (1)
2202-2202: ⚡ Quick winTighten Azure factory expectations to lock in the subscription pre-set contract.
These Azure ambient tests currently use permissive
mock.Anythingmatchers, so they won’t fail if the code stops passing"azure"or the expectedAzureSubscriptionIDinProviderConfig.Suggested test hardening
- mockFactory.On("CreateAndValidateProvider", mock.Anything, mock.Anything, mock.Anything).Return(mockProvider, nil) + mockFactory.On( + "CreateAndValidateProvider", + mock.Anything, + "azure", + mock.MatchedBy(func(cfg *provider.ProviderConfig) bool { + return cfg != nil && cfg.AzureSubscriptionID == "sub-abc-123" + }), + ).Return(mockProvider, nil)Apply the same pattern to the other Azure ambient tests with their respective subscription IDs.
Also applies to: 2246-2246, 2281-2281
🤖 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 `@internal/scheduler/scheduler_test.go` at line 2202, The test uses permissive mock.Anything for mockFactory.On("CreateAndValidateProvider", ...) which allows regressions; update the expectation to assert the provider type and subscription ID are passed: call mockFactory.On("CreateAndValidateProvider", ctxMatcher/anythingMatcher for context, mock.MatchedBy(func(cfg ProviderConfig) bool { return cfg.Type == "azure" && cfg.AzureSubscriptionID == "<expected-subscription-id>" }), mock.Anything).Return(mockProvider, nil) (or the equivalent matcher in your test framework) and apply the same change for the other Azure ambient test cases (lines referencing CreateAndValidateProvider at the other noted locations) so each test locks to the correct "azure" string and the specific AzureSubscriptionID constant.
🤖 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 `@internal/scheduler/scheduler_test.go`:
- Line 2202: The test uses permissive mock.Anything for
mockFactory.On("CreateAndValidateProvider", ...) which allows regressions;
update the expectation to assert the provider type and subscription ID are
passed: call mockFactory.On("CreateAndValidateProvider",
ctxMatcher/anythingMatcher for context, mock.MatchedBy(func(cfg ProviderConfig)
bool { return cfg.Type == "azure" && cfg.AzureSubscriptionID ==
"<expected-subscription-id>" }), mock.Anything).Return(mockProvider, nil) (or
the equivalent matcher in your test framework) and apply the same change for the
other Azure ambient test cases (lines referencing CreateAndValidateProvider at
the other noted locations) so each test locks to the correct "azure" string and
the specific AzureSubscriptionID constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5143d41-a4c7-4208-aa3d-0c7c82b758fe
📒 Files selected for processing (2)
internal/scheduler/scheduler.gointernal/scheduler/scheduler_test.go
Replace permissive mock.Anything matchers in the three Azure ambient-tagging
tests with mock.MatchedBy checks that assert both the provider type ("azure")
and the exact AzureSubscriptionID each test wires via AZURE_SUBSCRIPTION_ID.
This locks in the field-level contract so a future regression (wrong provider
name or wrong subscription routed to the factory) would be caught immediately.
Resolves CodeRabbit nitpick on PR #725.
|
Addressed the nitpick from the 2026-05-26 review pass. All three Azure ambient-tagging tests now use strict matchers instead of
Commit: daa9af0. All 6 @coderabbitai review |
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Closes #662. Mirrors PR #607's AWS pattern for the two non-AWS providers.
Defect
collectAzureRecommendationsandcollectGCPRecommendationsreturnednil, nil, nilwhen no accounts were registered — no ambient fallback path, breaking downstream attribution. AWS already had the fallback via PR #607.Changes (
internal/scheduler/scheduler.go)resolveAmbientAccountID(ctx, provider, externalID string) string— provider-agnostic helper that looks up(provider, externalID)incloud_accountsand returns the UUID or"". Errors swallowed (logged at WARN), mirroring the AWSresolveAmbientHostAccountIDdesign.collectAzureAmbient(ctx, subscriptionID string)— creates an Azure provider via the injected factory (testable via mock) with the subscription ID pre-set to avoid an unnecessary auto-discovery API call.collectGCPAmbient(ctx)— creates a GCP provider via the injected factory using ADC.collectAzureRecommendationsextended — whenlen(accounts) == 0andAZURE_SUBSCRIPTION_IDis set, falls back to ambient credentials and tags recs with the registered subscription UUID if found.collectGCPRecommendationsextended — same pattern usingGCP_PROJECT_ID.Tests (+10, 92 total in scheduler)
resolveAmbientAccountID: empty externalID guard, store error swallowedgo vet+gofmtclean.Summary by CodeRabbit
New Features
Tests