New Resource: `azurerm_monitor_diagnostic_setting` / New Data Source: `azurerm_monitor_diagnostic_categories` #1291

janboll · 2018-05-24T15:29:34Z

Hi,

I implemented a new resource to be able to configure diagnostic settings on any given resource (fixes #657). I am opening this PR to request feedback as soon as possible.
There are still open points

Documentation
Testing
- Update / delete
- Complete feature
Import
Datasource

Please have a look at the PR and check if I am steering into the right direction.
Thanks,
Jan

tombuildsstuff

hey @janboll

Thanks for this PR :)

I've taken a look through and left some comments in-line and have tried to provide code samples for all of the issues I've raised - but this is off to a good start 👍

Thanks!

azurerm/config.go

azurerm/resource_arm_monitor_diagnostics.go

tombuildsstuff · 2018-05-29T11:04:49Z

azurerm/resource_arm_monitor_diagnostics.go

+				Type:             schema.TypeString,
+				Required:         true,
+				ForceNew:         true,
+				DiffSuppressFunc: ignoreCaseDiffSuppressFunc,


can we remove the DiffSuppressFunc here, since this should force a recreation if the casing changes on the resource we're monitoring?

azurerm/resource_arm_monitor_diagnostics_test.go

azurerm/resource_arm_monitor_diagnostics.go

tombuildsstuff · 2018-05-29T14:05:50Z

azurerm/resource_arm_monitor_diagnostics.go

+		return err
+	}
+	if read.ID == nil {
+		return fmt.Errorf("Cannot read Diagnostic Settings")


can we make this error message clearer to match the other resources? e.g. fmt.Errorf("Cannot read ID for Monitor Diagnostics %q", name)

janboll · 2018-06-12T13:53:59Z

I had to change the schema cause of the way the Azure API behaves. If you omit one of MetricSettings or LogSettings it will just create these loggings with default settings instead of skipping them.
Therefore I built the schema so that it matches this behaviour, it will always enable all diagnostic settings unless you disable them using disabled_settings

janboll · 2018-06-25T12:25:29Z

Hi @tombuildsstuff did you have time to check on my changes yet?

tombuildsstuff · 2018-07-16T12:55:45Z

@janboll apologies for the really delayed re-review here - I'm going to take another look through this ~~tomorrow~~ (sorry I missed that) later this week :)

tombuildsstuff · 2018-07-24T14:01:52Z

@janboll hey - just to let you know that I'm taking a look at this at the moment. As a part of this I've pulled the code locally to read through properly, and have rebased this on top of master/switched to using the same API as the rest of the resources which use the insights SDK - I hope you don't mind!

tombuildsstuff

hey @janboll

Thanks for pushing those updates / apologies for the delayed review here!

I've pushed a couple of commits to rebase this and then fix the ID as mentioned in the comments/add some TODO's. Taking a look through this I think the schema needs some tweaking to get it right, in particular I think we need to add a data source to return the available (Metrics/Logs) Categories for a particular Diagnostic Setting, and probably look at the schema for the Metric and Log fields, since I think these want to be nested blocks; what do you think?

Thanks!

azurerm/resource_arm_monitor_diagnostics.go

tombuildsstuff · 2018-07-24T14:10:29Z

azurerm/resource_arm_monitor_diagnostics.go

+				Type:             schema.TypeString,
+				Required:         true,
+				ForceNew:         true,
+				DiffSuppressFunc: ignoreCaseDiffSuppressFunc,


can we remove the DiffSuppressFunc from this? in addition we can validate this using ValidateFunc: azure.ValidateResourceID (rather than the azure.ValidateResourceIDOrEmpty which is intended for optional fields)

tombuildsstuff · 2018-07-24T14:37:34Z

azurerm/monitor_diagnostics.go

+func retrieveDiagnosticName(diagnosticSettingId string) string {
+	substring := "/providers/microsoft.insights/diagnosticSettings/"
+	return diagnosticSettingId[len(substring)+strings.Index(diagnosticSettingId, substring):]
+}


on reflection I feel like a less brittle way of doing this would be to compose our own resource ID of {targetResourceID}|{name} since some of the Azure API's are case sensitive - I'm going to push a commit to fix this.

azurerm/monitor_diagnostics.go

azurerm/resource_arm_monitor_diagnostics.go

tombuildsstuff · 2018-11-02T19:03:05Z

@janboll apologies this has sat for a little while - I hope you don't mind but I've pushed a commit to rebase this on top of master so we can proceed with this :)

Add testCheckAzureRMMonitorDiagnosticsDestroy function

… etc Adding some TODO's around the Schema Ensuring fields are always set

… file

Tests pass: ``` $ acctests azurerm TestAccDataSourceArmMonitorDiagnosticCategories_ === RUN TestAccDataSourceArmMonitorDiagnosticCategories_appService --- PASS: TestAccDataSourceArmMonitorDiagnosticCategories_appService (150.71s) === RUN TestAccDataSourceArmMonitorDiagnosticCategories_storageAccount --- PASS: TestAccDataSourceArmMonitorDiagnosticCategories_storageAccount (108.97s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 261.573s ```

dismissing since changes have been pushed

``` $ acctests azurerm TestAccAzureRMMonitorDiagnosticSetting_ === RUN TestAccAzureRMMonitorDiagnosticSetting_eventhub === PAUSE TestAccAzureRMMonitorDiagnosticSetting_eventhub === RUN TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspace === PAUSE TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspace === RUN TestAccAzureRMMonitorDiagnosticSetting_storageAccount === PAUSE TestAccAzureRMMonitorDiagnosticSetting_storageAccount === CONT TestAccAzureRMMonitorDiagnosticSetting_eventhub --- PASS: TestAccAzureRMMonitorDiagnosticSetting_eventhub (282.77s) === CONT TestAccAzureRMMonitorDiagnosticSetting_storageAccount --- PASS: TestAccAzureRMMonitorDiagnosticSetting_storageAccount (219.99s) === CONT TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspace --- PASS: TestAccAzureRMMonitorDiagnosticSetting_logAnalyticsWorkspace (215.82s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 719.066s ```

katbyte

@janboll, @tombuildsstuff,

I have left some comments inline, most pretty innocuous. The one thing that stands out to me is what will happen if one or more of eventhub_authorization_rule_id, log_analytics_workspace_id or storage_account_id is set?

azurerm/data_source_monitor_diagnostic_categories.go

azurerm/resource_arm_monitor_diagnostic_setting.go

azurerm/utils/utils.go

correct log string

tombuildsstuff · 2018-11-14T10:35:00Z

@janboll thanks for pushing those changes 👍

Apologies (as I'd meant to post this yesterday) / for the delayed re-review here - I hope you don't mind, but I pushed some commits to update this resource (and added a Data Source) to better match the representation in Azure so that we can get this merged :)

Thanks!

janboll · 2018-11-14T15:44:04Z

@tombuildsstuff Thanks, really appreciate help on this PR! 👍

katbyte

LGTM @tombuildsstuff 👍

twem · 2018-12-04T16:12:53Z

The examples appear to be missing from the documentation. Has this been logged as an issue?

tombuildsstuff · 2018-12-04T17:20:30Z

@twem thanks for the heads up - that's been reported/fixed in #2391 and will be updated as a part of the next release :)

ghost · 2019-03-05T14:30:02Z

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 👉 [email protected]. Thanks!

tombuildsstuff added new-resource service/monitor labels May 25, 2018

tombuildsstuff self-assigned this May 25, 2018

tombuildsstuff self-requested a review May 25, 2018 06:52

tombuildsstuff suggested changes May 29, 2018

View reviewed changes

tombuildsstuff added the waiting-response label May 29, 2018

janboll force-pushed the feature/diagnostics-monitoring branch from 1ffa1f6 to a70de22 Compare June 4, 2018 15:30

This comment has been minimized.

Sign in to view

janboll force-pushed the feature/diagnostics-monitoring branch from 6ed0caf to 5c0ae0c Compare July 9, 2018 07:17

tombuildsstuff removed the waiting-response label Jul 11, 2018

tombuildsstuff force-pushed the feature/diagnostics-monitoring branch from 5c0ae0c to a433d5e Compare July 24, 2018 14:02

tombuildsstuff previously requested changes Jul 24, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Jul 24, 2018

metacpp requested a review from JunyiYi August 3, 2018 00:56

tombuildsstuff mentioned this pull request Aug 28, 2018

Automatically enable Diagnostic Settings at resource creation using a Resource Manager template #1834

Closed

tombuildsstuff removed the waiting-response label Sep 30, 2018

metacpp assigned JunyiYi Oct 19, 2018

katbyte unassigned JunyiYi Oct 25, 2018

katbyte added the waiting-response label Oct 25, 2018

tombuildsstuff added this to the 1.19.0 milestone Oct 25, 2018

tombuildsstuff mentioned this pull request Oct 25, 2018

No obious way to control "Basic Metrics" in "Diagnostics settings" in Azure VM #317

Closed

tombuildsstuff force-pushed the feature/diagnostics-monitoring branch from 0b9a331 to 65e8b01 Compare November 2, 2018 19:02

ghost added the size/XL label Nov 2, 2018

Jan-Hendrik Boll and others added 5 commits November 13, 2018 12:17

Rename Create/Update function

d9683f6

Add testCheckAzureRMMonitorDiagnosticsDestroy function

Switching to a custom ID format, to work around differences in casing…

ce53320

… etc Adding some TODO's around the Schema Ensuring fields are always set

refactoring/adding some validation

7ac61a7

refactor: moving the diagnostics settings category func into the main…

06ae887

… file

tombuildsstuff force-pushed the feature/diagnostics-monitoring branch from 6c0c2fa to 83f9ff4 Compare November 13, 2018 12:18

ghost added size/XXL and removed size/XL labels Nov 13, 2018

tombuildsstuff changed the title ~~[WIP] New resource: monitor diagnostics~~ New Resource: azurerm_monitor_diagnostic_setting / New Data Source: azurerm_monitor_diagnostic_categories Nov 13, 2018

tombuildsstuff force-pushed the feature/diagnostics-monitoring branch from b344abf to c3d9d1c Compare November 13, 2018 16:59

updating a todo

b909a7a

katbyte requested changes Nov 13, 2018

View reviewed changes

Use ValidateFunc for eventhub_name

125cb4f

correct log string

tombuildsstuff added 3 commits November 14, 2018 11:42

Adding a wait check func to the delete, to ensure it's gone

5befe55

removing unused code

bf61b9a

refactoring

ea431b1

tombuildsstuff assigned katbyte and unassigned tombuildsstuff Nov 14, 2018

tombuildsstuff mentioned this pull request Nov 14, 2018

Several Feature Enhancement Requests #2312

Closed

katbyte approved these changes Nov 15, 2018

View reviewed changes

katbyte merged commit 2154108 into hashicorp:master Nov 15, 2018

katbyte added a commit that referenced this pull request Nov 15, 2018

Update CHANGELOG.md to include #1291

8a3270e

ghost locked and limited conversation to collaborators Mar 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: `azurerm_monitor_diagnostic_setting` / New Data Source: `azurerm_monitor_diagnostic_categories` #1291

New Resource: `azurerm_monitor_diagnostic_setting` / New Data Source: `azurerm_monitor_diagnostic_categories` #1291

janboll commented May 24, 2018

tombuildsstuff left a comment

tombuildsstuff May 29, 2018

tombuildsstuff May 29, 2018

janboll commented Jun 12, 2018

This comment has been minimized.

janboll commented Jun 25, 2018

This comment has been minimized.

This comment has been minimized.

tombuildsstuff commented Jul 16, 2018 •

edited

Loading

tombuildsstuff commented Jul 24, 2018

tombuildsstuff left a comment

tombuildsstuff Jul 24, 2018

tombuildsstuff Jul 24, 2018

tombuildsstuff commented Nov 2, 2018

katbyte left a comment

tombuildsstuff commented Nov 14, 2018 •

edited

Loading

janboll commented Nov 14, 2018

katbyte left a comment

twem commented Dec 4, 2018

tombuildsstuff commented Dec 4, 2018

ghost commented Mar 5, 2019

New Resource: azurerm_monitor_diagnostic_setting / New Data Source: azurerm_monitor_diagnostic_categories #1291

New Resource: azurerm_monitor_diagnostic_setting / New Data Source: azurerm_monitor_diagnostic_categories #1291

Conversation

janboll commented May 24, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff May 29, 2018

Choose a reason for hiding this comment

tombuildsstuff May 29, 2018

Choose a reason for hiding this comment

janboll commented Jun 12, 2018

This comment has been minimized.

janboll commented Jun 25, 2018

This comment has been minimized.

This comment has been minimized.

tombuildsstuff commented Jul 16, 2018 • edited Loading

tombuildsstuff commented Jul 24, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff Jul 24, 2018

Choose a reason for hiding this comment

tombuildsstuff Jul 24, 2018

Choose a reason for hiding this comment

tombuildsstuff commented Nov 2, 2018

katbyte left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Nov 14, 2018 • edited Loading

janboll commented Nov 14, 2018

katbyte left a comment

Choose a reason for hiding this comment

twem commented Dec 4, 2018

tombuildsstuff commented Dec 4, 2018

ghost commented Mar 5, 2019

New Resource: `azurerm_monitor_diagnostic_setting` / New Data Source: `azurerm_monitor_diagnostic_categories` #1291

New Resource: `azurerm_monitor_diagnostic_setting` / New Data Source: `azurerm_monitor_diagnostic_categories` #1291

tombuildsstuff commented Jul 16, 2018 •

edited

Loading

tombuildsstuff commented Nov 14, 2018 •

edited

Loading