log_analytics_workspace: support for the `daily_quota_gb` property #8861

Lucretius · 2020-10-13T00:15:29Z

Resolves #3288

The daily volume cap is off by default as per the docs, so I've set the default value to -1 which means unlimited (no cap). Validation is set to either be -1 or some non-negative value.

One new test per resource/data source added, and one old test updated to confirm default value.

magodo

Thanks for this PR!
I have left some additional mostly minor comments that once addressed this should be good to merge 👍

magodo · 2020-10-13T03:18:49Z

azurerm/internal/services/loganalytics/tests/log_analytics_workspace_resource_test.go

@@ -146,6 +146,27 @@ func TestAccAzureRMLogAnalyticsWorkspace_withDefaultSku(t *testing.T) {
 				Config: testAccAzureRMLogAnalyticsWorkspace_withDefaultSku(data),
 				Check: resource.ComposeTestCheckFunc(
 					testCheckAzureRMLogAnalyticsWorkspaceExists(data.ResourceName),
+					resource.TestCheckResourceAttr(data.ResourceName, "daily_quota_gb", "-1"),


This check is redundent here as it will be checked in the data.ImportStep().

magodo · 2020-10-13T03:19:18Z

azurerm/internal/services/loganalytics/tests/log_analytics_workspace_resource_test.go

+				Config: testAccAzureRMLogAnalyticsWorkspace_withVolumeCap(data, 4.5),
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMLogAnalyticsWorkspaceExists(data.ResourceName),
+					resource.TestCheckResourceAttr(data.ResourceName, "daily_quota_gb", "4.5"),


This check is redundent here as it will be checked in the data.ImportStep().

azurerm/internal/services/loganalytics/tests/log_analytics_workspace_resource_test.go

magodo · 2020-10-13T03:21:47Z

website/docs/r/log_analytics_workspace.html.markdown

@@ -43,6 +43,8 @@ The following arguments are supported:

 * `retention_in_days` - (Optional) The workspace data retention in days. Possible values are either 7 (Free Tier only) or range between 30 and 730.

+* `daily_quota_gb` - (Optional) The workspace of the volume cap in gb.  Defaults to -1 (unlimited).


Can we adjust the statement a bit?

Suggested change

* `daily_quota_gb` - (Optional) The workspace of the volume cap in gb. Defaults to -1 (unlimited).

* `daily_quota_gb` - The workspace daily quota for ingestion in GB. Defaults to -1 (unlimited).

Do we not want the (Optional) in the resource?

magodo · 2020-10-13T03:22:04Z

website/docs/d/log_analytics_workspace.html.markdown

@@ -46,6 +46,8 @@ The following attributes are exported:

 * `retention_in_days` - The workspace data retention in days.

+* `daily_quota_gb` - The workspace of the volume cap in gb.  Defaults to -1 (unlimited).


Can we adjust the statement a bit? And no need to mention the default value for data source.

Suggested change

* `daily_quota_gb` - The workspace of the volume cap in gb. Defaults to -1 (unlimited).

* `daily_quota_gb` - The workspace daily quota for ingestion in GB.

…kspace_resource_test.go Co-authored-by: magodo <[email protected]>

Lucretius · 2020-10-13T06:38:11Z

Thanks for the response @magodo. If I understand correctly then, the data.ImportStep() checks that every attribute for a resource is set to the values defined in the test terraform snippets which should be written to state? Is there any reason to ever use TestCheckResourceAttr on individual attributes instead of just using data.ImportStep()? I have a few other PRs I will want to go back and alter if a single data.ImportStep() is the desired way to verify attributes match terraform test snippets.

jackofallops

Hi @Lucretius - I was running through a review, but I see you're updating while I'm part done, so I'll leave this partial review here and come back later :)

jackofallops · 2020-10-13T07:08:12Z

azurerm/internal/services/loganalytics/tests/log_analytics_workspace_resource_test.go

+  resource_group_name = azurerm_resource_group.test.name
+  sku                 = "PerGB2018"
+  retention_in_days   = 30
+  daily_quota_gb      = %d


Incorrect type here for float:

Suggested change

daily_quota_gb = %d

daily_quota_gb = %f

jackofallops · 2020-10-13T07:12:23Z

azurerm/internal/services/loganalytics/log_analytics_workspace_resource.go

@@ -130,6 +137,7 @@ func resourceArmLogAnalyticsWorkspaceCreateUpdate(d *schema.ResourceData, meta i
 	}

 	retentionInDays := int32(d.Get("retention_in_days").(int))
+	dailyQuotaGb := float64(d.Get("daily_quota_gb").(float64))


I don't think you need the cast here as you're asserting a float64, rather than converting between types?

magodo · 2020-10-14T01:26:27Z

@Lucretius iirc the only moments you willl need to use TestCheckResourceAttr/TestCheckResourceAttrSet is when the attributes you are checking are computed, e.g. when checking against a data source.

Lucretius · 2020-10-20T15:11:26Z

I think I just need to kick the tests off for this one again, as it appears one of the linter tools had an error on downloading as opposed to breaking on an individual step. @jackofallops you are free to continue your review, I fixed the two things you pointed out. Thanks for bearing with me as I refresh my Go :)

katbyte

Thanks @Lucretius - this LGTM 👍

ghost · 2020-10-29T19:06:37Z

This has been released in version 2.34.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.34.0"
}
# ... other configuration ...

ghost · 2020-11-22T14:50:55Z

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!

added property, docs, tests

851ae0f

ghost added size/M documentation labels Oct 13, 2020

Lucretius changed the title ~~added property, docs, tests~~ log_analytics_workspace: enable daily volume cap setting Oct 13, 2020

magodo added service/log-analytics enhancement labels Oct 13, 2020

magodo requested changes Oct 13, 2020

View reviewed changes

Lucretius and others added 2 commits October 12, 2020 23:17

Update azurerm/internal/services/loganalytics/tests/log_analytics_wor…

5073478

…kspace_resource_test.go Co-authored-by: magodo <[email protected]>

commit suggestions, dedupe test name

c2161fb

fix test

eae1223

jackofallops requested changes Oct 13, 2020

View reviewed changes

Lucretius added 2 commits October 13, 2020 10:33

some bug fixes

40dd7bd

run terrafmt locally again

af839b9

katbyte added the waiting-response label Oct 15, 2020

-1.0 instead of casting to float

85cdba6

ghost removed the waiting-response label Oct 20, 2020

katbyte approved these changes Oct 22, 2020

View reviewed changes

katbyte added this to the v2.34.0 milestone Oct 22, 2020

katbyte changed the title ~~log_analytics_workspace: enable daily volume cap setting~~ log_analytics_workspace: support for the daily_quota_gb property Oct 22, 2020

katbyte merged commit 539b9e3 into hashicorp:master Oct 22, 2020

katbyte added a commit that referenced this pull request Oct 22, 2020

update CHANGELOG.md for #8861

55eecf9

ghost locked as resolved and limited conversation to collaborators Nov 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

log_analytics_workspace: support for the `daily_quota_gb` property #8861

log_analytics_workspace: support for the `daily_quota_gb` property #8861

Lucretius commented Oct 13, 2020

magodo left a comment

magodo Oct 13, 2020

magodo Oct 13, 2020

magodo Oct 13, 2020

Lucretius Oct 13, 2020

katbyte Oct 13, 2020

magodo Oct 13, 2020

Lucretius commented Oct 13, 2020

jackofallops left a comment

jackofallops Oct 13, 2020

jackofallops Oct 13, 2020

magodo commented Oct 14, 2020

Lucretius commented Oct 20, 2020

katbyte left a comment

ghost commented Oct 29, 2020

ghost commented Nov 22, 2020

		@@ -43,6 +43,8 @@ The following arguments are supported:

		* `retention_in_days` - (Optional) The workspace data retention in days. Possible values are either 7 (Free Tier only) or range between 30 and 730.

		* `daily_quota_gb` - (Optional) The workspace of the volume cap in gb. Defaults to -1 (unlimited).

	* `daily_quota_gb` - (Optional) The workspace of the volume cap in gb. Defaults to -1 (unlimited).
	* `daily_quota_gb` - The workspace daily quota for ingestion in GB. Defaults to -1 (unlimited).

		@@ -46,6 +46,8 @@ The following attributes are exported:

		* `retention_in_days` - The workspace data retention in days.

		* `daily_quota_gb` - The workspace of the volume cap in gb. Defaults to -1 (unlimited).

log_analytics_workspace: support for the daily_quota_gb property #8861

log_analytics_workspace: support for the daily_quota_gb property #8861

Conversation

Lucretius commented Oct 13, 2020

magodo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lucretius commented Oct 13, 2020

jackofallops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magodo commented Oct 14, 2020

Lucretius commented Oct 20, 2020

katbyte left a comment

Choose a reason for hiding this comment

ghost commented Oct 29, 2020

ghost commented Nov 22, 2020

log_analytics_workspace: support for the `daily_quota_gb` property #8861

log_analytics_workspace: support for the `daily_quota_gb` property #8861