Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

azurerm_log_analytics_linked_storage_account - fix data_source_type case sensitivity #18116

Merged

Conversation

teowa
Copy link
Contributor

@teowa teowa commented Aug 25, 2022

Resolves #16234

Use strings.ToLower(), like in the schema below

Schema: map[string]*pluginsdk.Schema{
"data_source_type": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: validation.StringInSlice([]string{
strings.ToLower(string(operationalinsights.CustomLogs)),
strings.ToLower(string(operationalinsights.AzureWatson)),
strings.ToLower(string(operationalinsights.Query)),
strings.ToLower(string(operationalinsights.Alerts)),
// Value removed from enum in 2020-08-01, but effectively still works
"ingestion",
}, false),
},

Test Result
make acctests SERVICE='loganalytics' TESTARGS='-run=TestAcclogAnalyticsLinkedStorageAccount_' TESTTIMEOUT='60m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/loganalytics -run=TestAcclogAnalyticsLinkedStorageAccount_ -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAcclogAnalyticsLinkedStorageAccount_basic
=== PAUSE TestAcclogAnalyticsLinkedStorageAccount_basic
=== RUN   TestAcclogAnalyticsLinkedStorageAccount_requiresImport
=== PAUSE TestAcclogAnalyticsLinkedStorageAccount_requiresImport
=== RUN   TestAcclogAnalyticsLinkedStorageAccount_complete
=== PAUSE TestAcclogAnalyticsLinkedStorageAccount_complete
=== RUN   TestAcclogAnalyticsLinkedStorageAccount_update
=== PAUSE TestAcclogAnalyticsLinkedStorageAccount_update
=== RUN   TestAcclogAnalyticsLinkedStorageAccount_ingestion
=== PAUSE TestAcclogAnalyticsLinkedStorageAccount_ingestion
=== CONT  TestAcclogAnalyticsLinkedStorageAccount_basic
=== CONT  TestAcclogAnalyticsLinkedStorageAccount_update
=== CONT  TestAcclogAnalyticsLinkedStorageAccount_ingestion
=== CONT  TestAcclogAnalyticsLinkedStorageAccount_complete
=== CONT  TestAcclogAnalyticsLinkedStorageAccount_requiresImport
--- PASS: TestAcclogAnalyticsLinkedStorageAccount_basic (238.75s)
--- PASS: TestAcclogAnalyticsLinkedStorageAccount_ingestion (243.31s)
--- PASS: TestAcclogAnalyticsLinkedStorageAccount_complete (246.39s)
--- PASS: TestAcclogAnalyticsLinkedStorageAccount_requiresImport (263.46s)
--- PASS: TestAcclogAnalyticsLinkedStorageAccount_update (434.67s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/loganalytics  434.698s

@teowa teowa requested a review from katbyte September 9, 2022 04:42
@teowa teowa marked this pull request as draft September 15, 2022 12:54
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ♻️ is this supposed to still be a drafT?

@teowa teowa force-pushed the fix_log_analytics_linked_storage_account_dst branch from 3336a02 to 4e65e45 Compare September 16, 2022 06:39
@teowa
Copy link
Contributor Author

teowa commented Sep 16, 2022

LGTM ♻️ is this supposed to still be a drafT?

This is really a special case, a Enum is used in request path to create:

id := linkedstorageaccounts.NewDataSourceTypeID(workspace.SubscriptionId, d.Get("resource_group_name").(string), workspace.WorkspaceName, linkedstorageaccounts.DataSourceType(d.Get("data_source_type").(string)))
if d.IsNewResource() {
existing, err := client.Get(ctx, id)
if err != nil {
if !response.WasNotFound(existing.HttpResponse) {
return fmt.Errorf("checking for presence of existing %s: %+v", id, err)
}
}
if !response.WasNotFound(existing.HttpResponse) {
return tf.ImportAsExistsError("azurerm_log_analytics_linked_storage_account", id.ID())
}
}
parameters := linkedstorageaccounts.LinkedStorageAccountsResource{
Properties: linkedstorageaccounts.LinkedStorageAccountsProperties{
StorageAccountIds: utils.ExpandStringSlice(d.Get("storage_account_ids").(*pluginsdk.Set).List()),
},
}
if _, err := client.CreateOrUpdate(ctx, id, parameters); err != nil {
return fmt.Errorf("creating/updating %s: %+v", id, err)
}

I think in Read we should also set data_source_type using id.DataSourceType instead of using resp.Model.Properties.DataSourceType , align with what did in Create. Another question is the id.DataSourceType is always the Camel Case because SDK parse it (see source code), so the case diff suppression is added to schema of 3.x versions (since we ignore Enum case in 3.x).

below is a example response (id in response matches the request case, but DataSourceType in response.properties does not):

PUT https://management.azure.com/subscriptions/{{subscriptionId}}/resourceGroups/wt-test-app_ins/providers/Microsoft.OperationalInsights/workspaces/exampleworkspace/linkedStorageAccounts/CustomLogs?api-version=2020-08-01
{
    "properties": {
        "storageAccountIds": [
            "/subscriptions/{{subscriptionId}}/resourceGroups/wt-test-app_ins/providers/Microsoft.Storage/storageAccounts/wantatexamplesa"
        ]
    }
}

response:
{
    "properties": {
        "dataSourceType": "customlogs",
        "storageAccountIds": [
            "/subscriptions/{{subscriptionId}}/resourceGroups/wt-test-app_ins/providers/Microsoft.Storage/storageAccounts/wantatexamplesa"
        ]
    },
    "id": "/subscriptions/{{subscriptionId}}/resourceGroups/wt-test-app_ins/providers/Microsoft.OperationalInsights/workspaces/exampleworkspace/linkedStorageAccounts/CustomLogs",
    "name": "customlogs",
    "type": "Microsoft.OperationalInsights/workspaces/linkedStorageAccounts"
}

@teowa teowa marked this pull request as ready for review September 16, 2022 07:10
@teowa teowa requested a review from katbyte September 16, 2022 07:30
@teowa
Copy link
Contributor Author

teowa commented Sep 16, 2022

image

@katbyte
Copy link
Collaborator

katbyte commented Sep 24, 2022

@teowa - yes we should use the correct casing always

@@ -143,7 +144,8 @@ resource "azurerm_storage_account" "test" {
}

func (r LogAnalyticsLinkedStorageAccountResource) basic(data acceptance.TestData) string {
return fmt.Sprintf(`
if !features.FourPointOh() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this shouldn't be required because we're ignoring case till 4.0?

@@ -131,6 +137,7 @@ func resourceLogAnalyticsLinkedStorageAccountRead(d *pluginsdk.ResourceData, met

d.Set("resource_group_name", id.ResourceGroupName)
d.Set("workspace_resource_id", linkedstorageaccounts.NewWorkspaceID(id.SubscriptionId, id.ResourceGroupName, id.WorkspaceName).ID())
d.Set("data_source_type", string(id.DataSourceType))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is part of the id, we might want to do a state migration so we can remove the parse insensitively above?

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @teowa - LGTM now 🍄

@katbyte katbyte merged commit ab511d2 into hashicorp:main Sep 28, 2022
katbyte added a commit that referenced this pull request Sep 28, 2022
@teowa
Copy link
Contributor Author

teowa commented Sep 28, 2022

Sorry @katbyte , it's my fault, I forget to add the StateUpgraders in resource schema, I have submitted another PR #18556 to amend this.

@github-actions
Copy link

This functionality has been released in v3.25.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

azurerm_log_analytics_linked_storage_account - data_storage_type - case validation
3 participants