Azure: Populate cloud.account.name with DisplayName #1396
Conversation
orestisfl
force-pushed
the
1395/sub-name
branch
from
a9f0e9e to
2a5a364
Compare
📊 Allure Report - 💚 No failures were reported.
|
| @@ -108,10 +110,10 @@ func (p *ProviderInitializer) Init(ctx context.Context, log *logp.Logger, azureC | |||
| }, nil | |||
| } | |||
|
|
|||
| func (p *ProviderInitializer) getSubscriptionIds(ctx context.Context, azureConfig auth.AzureFactoryConfig) ([]*string, error) { | |||
| func (p *ProviderInitializer) getSubscriptionIds(ctx context.Context, azureConfig auth.AzureFactoryConfig) (map[string]string, error) { | |||
| // TODO: mockable | |||
There was a problem hiding this comment.
is this TODO still relevant?
There was a problem hiding this comment.
yes 👍 we are missing tests for these paths (whole Init())
|
|
||
| func (p *Provider) getAssetFromData(data map[string]any) AzureAsset { | ||
| subId := getString(data, "subscriptionId") | ||
| properties, _ := data["properties"].(map[string]any) |
There was a problem hiding this comment.
Should we have error handling if type assertions fail here?
There was a problem hiding this comment.
Not much we can do if these are missing since that would either mean the specific asset doesn't have "properties" (I don't know if that's possible) or the output was malformed. Since the code was just moved around, I'd argue to keep the code as is for now and decide on what to do for these cases (also for getString which ignores missing values / wrong types).
The code is safe from panics.
| if subscription != nil { | ||
| result = append(result, subscription.SubscriptionID) | ||
| if subscription != nil && subscription.SubscriptionID != nil { | ||
| result[*subscription.SubscriptionID] = strings.Dereference(subscription.DisplayName) |
There was a problem hiding this comment.
Can this lead to a panic?
There was a problem hiding this comment.
No, I check the subscription.SubscriptionID for nilness in the if above
| @@ -28,6 +28,7 @@ import ( | |||
| "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions" | |||
| "github.com/elastic/elastic-agent-libs/logp" | |||
| "github.com/samber/lo" | |||
| "golang.org/x/exp/maps" | |||
There was a problem hiding this comment.
orestisfl
force-pushed
the
1395/sub-name
branch
from
a6b38af to
b32dd03
Compare
Changes provider initialization to map subscription ids to names (cherry picked from commit 5d8738c)
Summary of your changes
Changes provider initialization to map subscription ids to names
Screenshot/Data
Related Issues
Checklist