-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
New Resource: azurerm_log_profile #1792
Conversation
* Update config and provider. * Add data and resource schemas. * Add docs. * Add acceptance tests.
* Normalise locations. * Set storage account and service bus IDs directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @liemnotliam , I have looked through your code and had several small comments.
azurerm/resource_arm_log_profile.go
Outdated
"name": { | ||
Type: schema.TypeString, | ||
Required: true, | ||
ForceNew: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the non-nil validation similar to this one. #Closed
azurerm/resource_arm_log_profile.go
Outdated
categories := expandLogProfileCategories(d) | ||
locations := expandLogProfileLocations(d) | ||
retentionPolicy, err := expandAzureRmLogProfileRetentionPolicy(d) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually you can combine this line and the next line together. This comment also applies to the several similar cases below. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combine them in which way, could you clarify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, this suggestion doesn't apply anymore. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @liemnotliam , Thanks for your contribution. I took a detailed look at your code and had provided some more detailed comments. And by the way, TestAccAzureRMLogProfile_servicebus
case failed with message After applying this step, the plan was not empty
.
azurerm/data_source_log_profile.go
Outdated
d.Set("storage_account_id", props.StorageAccountID) | ||
d.Set("service_bus_rule_id", props.ServiceBusRuleID) | ||
|
||
locations := make([]string, len(*props.Locations)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although defined as required in Azure REST spec, we might still need to nil
check props.Locations
to prevent potential crash.
if props.Locations != nil {
locations := …
…
d.Set("locations", locations)
}
``` #Closed
"github.com/hashicorp/terraform/helper/resource" | ||
) | ||
|
||
func TestAccAzureRMLogProfile_importBasic(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our latest best practice for import tests is to move them to the normal test cases. Take this action group test
as an example. #Closed
azurerm/resource_arm_log_profile.go
Outdated
locations := []string{} | ||
|
||
for _, location := range logProfileLocations { | ||
locations = append(locations, location.(string)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe azureRMNormalizeLocation(location.(string))
here. #Closed
} | ||
|
||
resource "azurerm_log_profile" "test" { | ||
name = "basic" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Append random number to name
to prevent name conflict when running tests in parallel. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The acceptance tests are combined into one run now. Azure doesn't allow for more than one log profile in a subscription so these tests shouldn't run in parallel.
} | ||
|
||
resource "azurerm_servicebus_namespace" "test" { | ||
name = "a%s" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming convention acctestsbns-%s
#Closed
} | ||
|
||
resource "azurerm_servicebus_namespace_authorization_rule" "test" { | ||
name = "%s" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming convention acctestsbrule-%s
#Closed
* Move import test to normal test case. * Update resource names.
* Make retention policy days optional, which defaults to 0. * Add nil checks. * Add flatten locations function. * Refactor based on conventions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me except for the Retry stuffs, that might be an API issue and should be fixed on their side. All other stuffs just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that is the case, yes @liemnotliam , I'm OK with other code logic here.
Thanks for the comments and feedback @JunyiYi ! Is there scope for this to be included in the v1.14.0 from the Future milestone? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @liemnotliam
Thanks for this PR - apologies for the delayed review here!
I've taken a look through and left some comments in-line - but this is mostly looking good. If we can fix those up we should be able to kick off the tests and see how this looks 👍
Thanks!
azurerm/data_source_log_profile.go
Outdated
d.Set("categories", props.Categories) | ||
|
||
d.Set("locations", flattenAzureRmLogProfileLocations(props.Locations)) | ||
d.Set("retention_policy", flattenAzureRmLogProfileRetentionPolicy(props.RetentionPolicy)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
singe both of these calls sets complex objects; can we check for errors here? e.g.
if err := d.Set("locations", flattenAzureRmLogProfileLocations(props.Locations)); err != nil {
return fmt.Errorf("Error flattening `locations`: %+v", err)
}
if err := d.Set("retention_policy", flattenAzureRmLogProfileRetentionPolicy(props.RetentionPolicy)); err != nil {
return fmt.Errorf("Error flattening `retention_policy `: %+v", err)
}
azurerm/data_source_log_profile.go
Outdated
resp, err := client.Get(ctx, name) | ||
if err != nil { | ||
if utils.ResponseWasNotFound(resp.Response) { | ||
d.SetId("") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove the d.SetId("")
here?
data "azurerm_log_profile" "test" { | ||
name = "${azurerm_log_profile.test.name}" | ||
} | ||
`, rInt, location, rString, rInt, location) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor can we switch over to using spaces rather than tabs for indentations here?
data "azurerm_log_profile" "test" { | ||
name = "${azurerm_log_profile.test.name}" | ||
} | ||
`, rInt, location, rString, rInt, location) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor can we switch over to using spaces rather than tabs for indentations here?
azurerm/provider.go
Outdated
@@ -188,6 +189,7 @@ func Provider() terraform.ResourceProvider { | |||
"azurerm_local_network_gateway": resourceArmLocalNetworkGateway(), | |||
"azurerm_log_analytics_solution": resourceArmLogAnalyticsSolution(), | |||
"azurerm_log_analytics_workspace": resourceArmLogAnalyticsWorkspace(), | |||
"azurerm_log_profile": resourceArmLogProfile(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this azurerm_monitor_log_profile
to match the other monitoring resources?
|
||
* `categories` - List of categories of the logs. | ||
|
||
* `retention_policy`- A retention policy for how long Activity Logs are retained in the storage account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update this to be:
* `retention_policy` - a `retention_policy` block as documented below
"westus", | ||
"northcentralus", | ||
"southcentralus", | ||
"global", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the purposes of this example we should be able to simplify this to just global
?
azurerm/data_source_log_profile.go
Outdated
Type: schema.TypeString, | ||
Computed: true, | ||
}, | ||
"service_bus_rule_id": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this servicebus_rule_id
to match the other resources/Azure branding?
azurerm/data_source_log_profile.go
Outdated
|
||
if props := resp.LogProfileProperties; props != nil { | ||
d.Set("storage_account_id", props.StorageAccountID) | ||
d.Set("service_bus_rule_id", props.ServiceBusRuleID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this servicebus_rule_id
to match the other resources/Azure branding?
azurerm/resource_arm_log_profile.go
Outdated
Optional: true, | ||
ValidateFunc: azure.ValidateResourceIDOrEmpty, | ||
}, | ||
"service_bus_rule_id": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this servicebus_rule_id
to match the other resources/Azure branding?
* Renamed log profile resource to `azurerm_monitor_log_profile` * Renamed service bus rule id field to match azurerm provider branding * Add more detailed error messages * Update locations and categories type to Set * Use alt test location to support tests in Azure China, gov't, etc.
Hey @tombuildsstuff I've made changes based on your comments and this PR is ready to be reviewed again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @liemnotliam,
I've taken a quick look and it mostly LGTM aside from a few comments, none that would block this. However the tests are currently failing:
------- Stdout: -------
=== RUN TestAccAzureRMMonitorLogProfile
=== RUN TestAccAzureRMMonitorLogProfile/basic
=== RUN TestAccAzureRMMonitorLogProfile/basic/basic
=== RUN TestAccAzureRMMonitorLogProfile/basic/servicebus
=== RUN TestAccAzureRMMonitorLogProfile/basic/complete
=== RUN TestAccAzureRMMonitorLogProfile/basic/disappears
--- FAIL: TestAccAzureRMMonitorLogProfile (769.60s)
--- FAIL: TestAccAzureRMMonitorLogProfile/basic (769.60s)
--- FAIL: TestAccAzureRMMonitorLogProfile/basic/basic (93.57s)
testing.go:513: Step 0 error: Error applying: 1 error(s) occurred:
* azurerm_monitor_log_profile.test: 1 error(s) occurred:
* azurerm_monitor_log_profile.test: Error Creating/Updating Log Profile "acctestlp-9142375735542561086": insights.LogProfilesClient#CreateOrUpdate: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> <nil>
--- FAIL: TestAccAzureRMMonitorLogProfile/basic/servicebus (433.14s)
testing.go:513: Step 0 error: Error applying: 1 error(s) occurred:
* azurerm_monitor_log_profile.test: 1 error(s) occurred:
* azurerm_monitor_log_profile.test: Error Creating/Updating Log Profile "acctestlp-2367274785140308499": insights.LogProfilesClient#CreateOrUpdate: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> <nil>
--- FAIL: TestAccAzureRMMonitorLogProfile/basic/complete (159.29s)
testing.go:513: Step 0 error: Error applying: 1 error(s) occurred:
* azurerm_monitor_log_profile.test: 1 error(s) occurred:
* azurerm_monitor_log_profile.test: Error Creating/Updating Log Profile "acctestlp-5022616636028035825": insights.LogProfilesClient#CreateOrUpdate: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> <nil>
--- FAIL: TestAccAzureRMMonitorLogProfile/basic/disappears (83.60s)
testing.go:513: Step 0 error: Error applying: 1 error(s) occurred:
* azurerm_monitor_log_profile.test: 1 error(s) occurred:
* azurerm_monitor_log_profile.test: Error Creating/Updating Log Profile "acctestlp-290585701865438756": insights.LogProfilesClient#CreateOrUpdate: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> <nil>
FAIL
I'll take a closer look later today/tomorrow but could this be related to only one log profile being able to be provisioned per subscription?
|
||
func testAccDataSourceAzureRMMonitorLogProfile_storageaccountConfig(rInt int, rString string, location string) string { | ||
return fmt.Sprintf(` | ||
resource "azurerm_resource_group" "test" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor and not a blocker, but could we left align theres:
return fmt.Sprintf(`
resource "azurerm_resource_group" "test" {
page_title: "Azure Resource Manager: azurerm_monitor_log_profile" | ||
sidebar_current: "docs-azurerm-resource-monitor-log-profile" | ||
description: |- | ||
Creates a Log Profile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be Manages a
|
||
# azurerm_monitor_log_profile | ||
|
||
Creates a [Log Profile](https://docs.microsoft.com/en-us/azure/monitoring-and-diagnostics/monitoring-overview-activity-logs#export-the-activity-log-with-a-log-profile). A Log Profile configures how Activity Logs are exported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As should this
Hey @katbyte , I've made the changes based on your comments. The 409 conflict error typically indicates that there's an existing log profile so it can't create another due to that 1 log profile per subscription limitation. |
Cleaning out our subscription and trying again. However if this is the case having the datasource and resource tests separate is going to cause issues with our nightly build, they will either need to be combined or locked so they don't run at the same time. |
I am still unable to get it to work on our subscription. I suspect there is a log profile kicking around somewhere. Are you aware of where to find this in the portal? However running the tests on another subscription, the test is showing a non-empty plan:
|
The portal doesn't really provide an easy way to view an existing log profile. You can query it via az cli with I've combined the tests and pasted the test results below. ~/go/src/github.com/terraform-providers/terraform-provider-azurerm on log-profile*
TESTARGS="-run TestAccAzureRMMonitorLogProfile -count=1" make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccAzureRMMonitorLogProfile -count=1 -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
? github.com/terraform-providers/terraform-provider-azurerm [no test files]
=== RUN TestAccAzureRMMonitorLogProfile
=== RUN TestAccAzureRMMonitorLogProfile/basic
=== RUN TestAccAzureRMMonitorLogProfile/basic/basic
=== RUN TestAccAzureRMMonitorLogProfile/basic/servicebus
=== RUN TestAccAzureRMMonitorLogProfile/basic/complete
=== RUN TestAccAzureRMMonitorLogProfile/basic/disappears
=== RUN TestAccAzureRMMonitorLogProfile/datasource
=== RUN TestAccAzureRMMonitorLogProfile/datasource/eventhub
=== RUN TestAccAzureRMMonitorLogProfile/datasource/storageaccount
--- PASS: TestAccAzureRMMonitorLogProfile (1070.07s)
--- PASS: TestAccAzureRMMonitorLogProfile/basic (807.06s)
--- PASS: TestAccAzureRMMonitorLogProfile/basic/basic (98.02s)
--- PASS: TestAccAzureRMMonitorLogProfile/basic/servicebus (442.57s)
--- PASS: TestAccAzureRMMonitorLogProfile/basic/complete (174.21s)
--- PASS: TestAccAzureRMMonitorLogProfile/basic/disappears (92.27s)
--- PASS: TestAccAzureRMMonitorLogProfile/datasource (263.01s)
--- PASS: TestAccAzureRMMonitorLogProfile/datasource/eventhub (165.44s)
--- PASS: TestAccAzureRMMonitorLogProfile/datasource/storageaccount (97.56s)
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 1070.106s
testing: warning: no tests to run
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/authentication 0.023s [no tests to run]
testing: warning: no tests to run
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/azure 0.029s [no tests to run]
testing: warning: no tests to run
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/kubernetes 0.010s [no tests to run]
testing: warning: no tests to run
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/response 0.037s [no tests to run]
? github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/set [no test files]
testing: warning: no tests to run
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/suppress 0.032s [no tests to run]
testing: warning: no tests to run
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/validate 0.026s [no tests to run]
testing: warning: no tests to run
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils 0.038s [no tests to run]
? github.com/terraform-providers/terraform-provider-azurerm/version [no test files] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip @liemnotliam! I was able to see there was one kicking around and delete it. Totally didn't think to use the cli.
I was able to get the tests to pass except the complete one, that then succeeded when i ran it after. Seems our subscription doesn't like provisioning and deprovisioning things so rapidly
gofmt -w $(find . -name '*.go' |grep -v vendor)
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMMonitorLogProfile -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN TestAccAzureRMMonitorLogProfile
=== RUN TestAccAzureRMMonitorLogProfile/datasource
=== RUN TestAccAzureRMMonitorLogProfile/datasource/eventhub
=== RUN TestAccAzureRMMonitorLogProfile/datasource/storageaccount
=== RUN TestAccAzureRMMonitorLogProfile/basic
=== RUN TestAccAzureRMMonitorLogProfile/basic/basic
=== RUN TestAccAzureRMMonitorLogProfile/basic/servicebus
=== RUN TestAccAzureRMMonitorLogProfile/basic/complete
=== RUN TestAccAzureRMMonitorLogProfile/basic/disappears
--- FAIL: TestAccAzureRMMonitorLogProfile (1061.77s)
--- PASS: TestAccAzureRMMonitorLogProfile/datasource (283.16s)
--- PASS: TestAccAzureRMMonitorLogProfile/datasource/eventhub (182.56s)
--- PASS: TestAccAzureRMMonitorLogProfile/datasource/storageaccount (100.60s)
--- FAIL: TestAccAzureRMMonitorLogProfile/basic (778.61s)
--- PASS: TestAccAzureRMMonitorLogProfile/basic/basic (95.31s)
--- PASS: TestAccAzureRMMonitorLogProfile/basic/servicebus (449.11s)
--- FAIL: TestAccAzureRMMonitorLogProfile/basic/complete (153.35s)
testing.go:513: Step 0 error: Error applying: 1 error(s) occurred:
* azurerm_monitor_log_profile.test: 1 error(s) occurred:
* azurerm_monitor_log_profile.test: Error retrieving Log Profile "acctestlp-6353106286116217609": insights.LogProfilesClient#Get: Failure responding to request: StatusCode=404 -- Original Error: autorest/azure: Service returned an error. Status=404 Code="ResourceNotFound" Message="Log profile 'acctestlp-6353106286116217609' under scope '/subscriptions/c0a607b2-6372-4ef3-abdb-dbe52a7b56ba' doesn't exist."
--- FAIL: TestAccAzureRMMonitorLogProfile/basic/disappears (80.85s)
testing.go:513: Step 0 error: Error applying: 1 error(s) occurred:
* azurerm_monitor_log_profile.test: 1 error(s) occurred:
* azurerm_monitor_log_profile.test: Error Creating/Updating Log Profile "acctestlp-4445915028616985967": insights.LogProfilesClient#CreateOrUpdate: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> <nil>
FAIL
FAIL github.com/terraform-providers/terraform-provider-azurerm/azurerm 1061.805s
make: *** [testacc] Error 1
[12:42:44] kt@snowbook:~/hashi/..3../terraform-providers/terraform-provider-azurerm$ make fmt; make testacc TEST=./azurerm TESTARGS=-test.run=TestAccAzureRMMonitorLogProfile/basic/complete
gofmt -w $(find . -name '*.go' |grep -v vendor)
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMMonitorLogProfile/basic/complete -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN TestAccAzureRMMonitorLogProfile
=== RUN TestAccAzureRMMonitorLogProfile/basic
=== RUN TestAccAzureRMMonitorLogProfile/basic/complete
--- PASS: TestAccAzureRMMonitorLogProfile (163.17s)
--- PASS: TestAccAzureRMMonitorLogProfile/basic (163.17s)
--- PASS: TestAccAzureRMMonitorLogProfile/basic/complete (163.17s)
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 163.203s
So i'm going to say this LGTM now and merge 🙂
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
New Log Profile resource to address issue #1175