New Resource: `azure_consumption_budget_*` #9201

marc-sensenich · 2020-11-06T16:49:19Z

Adds resources

azurerm_consumption_budget_subscription
azurerm_consumption_budget_resource_group

Test status as of 60f9fd3

--- PASS: TestAccConsumptionBudgetSubscription_basic (82.18s)
--- PASS: TestAccConsumptionBudgetSubscription_basicUpdate (136.08s)
--- PASS: TestAccConsumptionBudgetSubscription_complete (132.56s)
--- PASS: TestAccConsumptionBudgetSubscription_completeUpdate (190.34s)
--- PASS: TestAccConsumptionBudgetSubscription_requiresImport (90.68s)

--- PASS: TestAccConsumptionBudgetResourceGroup_basic (128.89s)
--- PASS: TestAccConsumptionBudgetResourceGroup_complete (128.99s)
--- PASS: TestAccConsumptionBudgetResourceGroup_completeUpdate (190.29s)
--- PASS: TestAccConsumptionBudgetResourceGroup_requiresImport (131.36s)
--- PASS: TestAccConsumptionBudgetResourceGroup_basicUpdate (170.65s)

Closes #2677

marc-sensenich · 2020-11-10T19:56:09Z

website/azurerm.erb

@@ -1248,6 +1248,15 @@
              </ul>
            </li>

+            <li>


Is there a way to generate these sections via make? I did this one manually and it appeared on make website

marc-sensenich · 2020-11-10T19:57:08Z

website/docs/r/consumption_budget_subscription.html.markdown

@@ -0,0 +1,178 @@
+---
+subcategory: "Consumption"


I created a Consumption subcategory based on API spec, it could also go under the existing Cost Management subcategory as well based on the goal of budgets

sebastianreloaded · 2021-01-31T12:36:25Z

Will this be merged at some point?

marc-sensenich · 2021-02-05T22:27:06Z

@sebastianreloaded this PR should be updated to the new service layout and testing structure based on the changes that have happened since this PR was originally opened as it is greenfield. I'll try to take a look at updating this PR early next week with those changes and work with the maintainers to get this PR merged in

mbfrahry

Hey @marc-sensenich! Thanks for your patience here! I've given it a cursory look and things look generally good! It's just a bit behind (through no fault of your own) so I haven't been able to extensively test it yet. Do you mind addressing what I've called out and bringing it back up to date.

Once that's done, I can give it another review to confirm everything is in place. If everything looks good, we can get it merged quickly.

mbfrahry · 2021-05-06T20:25:25Z

azurerm/internal/services/consumption/helpers.go

+
+func FlattenConsumptionBudgetNotifications(input map[string]*consumption.Notification) []interface{} {
+	if input == nil {
+		return nil


Sometimes we get weird diffing when returning nil in flatten methods so we'd rather return an empty []interface{}{}

mbfrahry · 2021-05-06T20:25:45Z

azurerm/internal/services/consumption/helpers.go

+	for _, v := range input {
+		notificationBlock := make(map[string]interface{})
+
+		notificationBlock["enabled"] = *v.Enabled


We should make sure this isn't nil before pointing to it

mbfrahry · 2021-05-06T20:46:25Z

azurerm/internal/services/consumption/helpers.go

+	notifications := make(map[string]*consumption.Notification)
+
+	for _, v := range input {
+		notificationRaw := v.(map[string]interface{})


We'll want to make sure that v isn't nil before casting to map[string]interface{}

mbfrahry · 2021-05-06T21:16:52Z

azurerm/internal/services/consumption/consumption_budget_common.go

+	amount := decimal.NewFromFloat(d.Get("amount").(float64))
+	timePeriod, err := ExpandConsumptionBudgetTimePeriod(d.Get("time_period").([]interface{}))
+	if err != nil {
+		return fmt.Errorf("error in expanding`time_period`: %+v", err)


Suggested change

return fmt.Errorf("error in expanding`time_period`: %+v", err)

return fmt.Errorf("error expanding`time_period`: %+v", err)

mbfrahry · 2021-05-06T21:48:59Z

azurerm/internal/services/consumption/consumption_budget_common.go

+		return err
+	}
+
+	if read.ID == nil {


Rather than setting the ID through this check. We've been generating the ID and then setting that. That lets us avoid issues where ID is nil.

mbfrahry · 2021-05-06T22:47:50Z

azurerm/internal/services/consumption/schema.go

+			ValidateFunc: validation.FloatAtLeast(1.0),
+		},
+
+		"category": {


Do you know if there is plan to expand values that are accepted here? If not, we can leave it out

I don't have insight into if this will be expanded, I added it for completeness just in case. If preferred, I'll remove it from the code path

I'd go ahead and remove it since only one value can be specified. We can add it back in later if more categories get added.

Removed in 25e2561

mbfrahry · 2021-05-06T22:50:06Z

azurerm/internal/services/consumption/helpers.go

+}
+
+func ExpandConsumptionBudgetComparisonExpression(input interface{}) *consumption.BudgetComparisonExpression {
+	v := input.(map[string]interface{})


We'll want to confirm that v isn't nil before casting.

mbfrahry · 2021-05-06T23:07:37Z

azurerm/internal/services/consumption/helpers.go

+}
+
+// DiffSuppressFuncs
+func DiffSuppressFuncConsumptionBudgetTimePeriodEndDate(k, old, new string, d *schema.ResourceData) bool {


I may be mistaken but I thought having Computed: true on end_date would catch this. Is that not the case?

I'll need to double check this as I can't recall now.

You were right, I can't recall exactly why I felt this was needed at the time or if it was a user error. Refactored out in 7da9f12

mbfrahry · 2021-05-06T23:15:15Z

azurerm/internal/services/consumption/registration.go

+)
+
+const (
+	consumptionBudgetResourceGroupName = "azurerm_consumption_budget_resource_group"


It feels weird to use this format but I can see the value after thinking it through. Do you mind leaving a comment about why we're using this format in case we come back and forget why it's done this way

@mbfrahry when you say "format" do you mean the actual resource name of azurerm_consumption_budget_$RESOURCE_TYPE? I usually have a tendency to be a bit verbose with naming, so if azurerm_budget_$RESOURCE_TYPE makes more sense it is cleaner and I can also refactor ConsumptionBudget to Budget in the code

Sorry for the added confusion. Normally we don't call out resource names into their own variables but there is a use case here because of how you're passing the names along in various methods throughout this package. I just want us to call out that in a comment so we don't forget why we're doing this differently than every other package.

Clarification added in 2a6d556

website/azurerm.erb

…tion

…up Consumption Budget

mbfrahry

Things are looking good! I just have concerns about the way we're doing ResourceIDs. Please let me know if what I'm suggesting makes sense!

mbfrahry · 2021-05-10T18:04:36Z

azurerm/internal/services/consumption/consumption_budget_common.go

+
+	d.Set("name", resp.Name)
+	d.Set("category", resp.Category)
+	amount, _ := resp.Amount.Float64()


I believe this could panic if the value is nil for whatever reason. Let's check that it isn't nil just to be sure

Nil checked in 926f618

mbfrahry · 2021-05-10T18:13:39Z

azurerm/internal/services/consumption/consumption_budget_resource_group_resource.go

+		Update: resourceArmConsumptionBudgetResourceGroupCreateUpdate,
+		Delete: resourceArmConsumptionBudgetDelete,
+		Importer: pluginsdk.ImporterValidatingResourceId(func(id string) error {
+			_, err := parse.ConsumptionBudgetID(id)


I have concerns with this generic ConsumptionBudgetID. Someone could pass any consumption budget id as if it was a Consumption Budget ResourceGroup and Terraform would manage it. I saw that in the id tests, you were testing various consumption budget ids from different resources. I'd like to see that expanded out so we have ConsumptionBudgetResourceGroupID so we have 100% certainty that the resource is in fact a Consumption Budget Resource Group. Do you think that's possible?

That's a good call out and for details I pass in the the various consumption budget IDs to test the ID across the 8 various scopes that a budget could be applied to. I do believe it's possible to expand these out into per consumption budget scope ID structs with a generator. My thought process is to make a generator for Consumption Budget ID structs that will generate the code for each ID and it could leverage existing validators to verify that that scope provided is in the format expected for that ID type

That ID path you laid out sounds great! Have you seen how we generate IDs in other services? Do you think a format like this could be used for Consumption Budget? Hopefully it won't be anymore issue then that for generating the ids.

@mbfrahry I was able to use the available resource ID generator for this as the new features that have been added since my initial opening of this PR now support resource IDs such as the ones for Consumption Budgets. Thanks for pointing them out!

azurerm/utils/common_marshal.go

These were used with the AzureRM Consumption Budget 2019-01-01 API and are no longer required

…up Consumption Budget

…date is computed Remove DiffSuppressFuncConsumptionBudgetTimePeriodEndDate as the end_date is computed due to the Azure RM API setting this to be 10 years in the future from start_date if not specified in the config.

This is due to these being passed into generic Consumption Budget functions containing the core logic for Consumption Budget resources

…mount on resource read

…ble value

…up Consumption Budget

Now that the resource ID generator supports IDs with nested resources, leverage this for generating Consumption Budget resource IDs over a custom implementation

mbfrahry

LGTM! I went ahead and changed the way we grab the scope for the delete methods because we really shouldn't do d.Get inside of delete methods because it may have been removed from the statefile and the config file so we won't know what that value is. I went ahead and got the scope from the id (exactly the way you did it in the Read methods).

Other than that we're good to go! Thank you for getting this done!

ghost · 2021-05-14T03:05:52Z

This has been released in version 2.59.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.59.0"
}
# ... other configuration ...

marc-sensenich · 2021-05-14T17:13:12Z

@mbfrahry thanks for the details, cleanup, and review!

jebaraa · 2021-05-19T01:49:17Z

Hilp

…m_consumption_budget_subscription` hashicorp#9201

github-actions · 2021-06-18T02:12:38Z

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.

ghost added size/XXL dependencies labels Nov 6, 2020

marc-sensenich mentioned this pull request Nov 6, 2020

New Resource: Consumption Budgets #2677

Closed

marc-sensenich commented Nov 10, 2020

View reviewed changes

marc-sensenich marked this pull request as ready for review December 2, 2020 04:35

katbyte added new-resource service/consumption labels Apr 6, 2021

katbyte assigned mbfrahry Apr 22, 2021

mbfrahry requested changes May 6, 2021

View reviewed changes

mbfrahry added the waiting-response label May 6, 2021

marc-sensenich added 17 commits May 7, 2021 16:54

Add consumption budgets client

c627fab

Add expand and flatten for UUID slices

4ff0ac7

Add consumption budget name validation and ID parsing

9336775

Add consumption budget expand and flatten funcs

67d5d27

Add DiffSuppressFunc for Azure Consumption time_period.end_date

b1b08e4

Add validation for time_period.start_date

bfaa9a6

fixup! Add expand and flatten for UUID slices

c86db60

Add resource azurerm_consumption_budget_subscription

42d33bb

Add resource azurerm_consumption_budget_subscription documentation

d11a4b7

fixup! Add resource azurerm_consumption_budget_subscription documenta…

a33d4d8

…tion

fixup! Add resource azurerm_consumption_budget_subscription

3e19ab8

fixup! Add resource azurerm_consumption_budget_subscription documenta…

e4db287

…tion

Add resource azurerm_consumption_budget_resource_group

0f68fd7

fixup! Add resource azurerm_consumption_budget_subscription

f658b89

fixup! Add resource azurerm_consumption_budget_resource_group

cde0853

Add Computed to end_date

d782c9d

Move Consumption Budget tests

cf64e1e

marc-sensenich added 4 commits May 7, 2021 18:19

fixup! Use Resource Group ID instead of name and sub for Resource Gro…

fd6074e

…up Consumption Budget

fixup! Add ID method to ConsumptionBudgetId

7d6da36

Run go mod vendor

884f53b

Run terrafmt fmt on consumption_budget_resource_group_resource_test.go

fab7a12

mbfrahry requested changes May 10, 2021

View reviewed changes

marc-sensenich added 6 commits May 11, 2021 05:48

Remove UUID slice expand and flatten functions

8eb5683

These were used with the AzureRM Consumption Budget 2019-01-01 API and are no longer required

fixup! Use Resource Group ID instead of name and sub for Resource Gro…

61d4529

…up Consumption Budget

Add comments around why Consumption Budget resource names are consts

2a6d556

This is due to these being passed into generic Consumption Budget functions containing the core logic for Consumption Budget resources

Check that resp.Amount is not nil before setting Consumption Budget a…

926f618

…mount on resource read

Remove category from Consumption Budget schema as it has only 1 possi…

25e2561

…ble value

marc-sensenich requested a review from mbfrahry May 11, 2021 11:02

marc-sensenich and others added 4 commits May 12, 2021 21:38

fixup! Use Resource Group ID instead of name and sub for Resource Gro…

21c34bc

…up Consumption Budget

Use generated resource IDs for Consumption Budget resources

60f9fd3

Now that the resource ID generator supports IDs with nested resources, leverage this for generating Consumption Budget resource IDs over a custom implementation

Change id generation in delete for resource group

cae3cdb

Update id generation for delete with subscription

98f948e

mbfrahry approved these changes May 13, 2021

View reviewed changes

mbfrahry changed the title Add azure_consumption_budget_* New Resource: azure_consumption_budget_* May 13, 2021

mbfrahry merged commit f4b5edd into hashicorp:master May 13, 2021

mbfrahry added a commit that referenced this pull request May 13, 2021

Update changelog for #9201

5099e03

mbfrahry added this to the v2.59.0 milestone May 13, 2021

marc-sensenich deleted the gh-2677 branch May 14, 2021 17:12

ghost removed the waiting-response label May 14, 2021

favoretti pushed a commit to gro1m/terraform-provider-azurerm that referenced this pull request May 26, 2021

New Resource: azurerm_consumption_budget_resource_group and `azurer…

2138bc4

…m_consumption_budget_subscription` hashicorp#9201

favoretti pushed a commit to gro1m/terraform-provider-azurerm that referenced this pull request May 26, 2021

Update changelog for hashicorp#9201

dc84bcd

github-actions bot locked as resolved and limited conversation to collaborators Jun 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: `azure_consumption_budget_*` #9201

New Resource: `azure_consumption_budget_*` #9201

marc-sensenich commented Nov 6, 2020 •

edited

Loading

marc-sensenich Nov 10, 2020

marc-sensenich Nov 10, 2020

sebastianreloaded commented Jan 31, 2021

marc-sensenich commented Feb 5, 2021

mbfrahry left a comment

mbfrahry May 6, 2021

mbfrahry May 6, 2021

mbfrahry May 6, 2021

mbfrahry May 6, 2021

mbfrahry May 6, 2021

mbfrahry May 6, 2021

marc-sensenich May 7, 2021

mbfrahry May 10, 2021

marc-sensenich May 11, 2021

mbfrahry May 6, 2021

mbfrahry May 6, 2021

marc-sensenich May 7, 2021

marc-sensenich May 11, 2021

mbfrahry May 6, 2021

marc-sensenich May 7, 2021

mbfrahry May 10, 2021

marc-sensenich May 11, 2021

mbfrahry left a comment

mbfrahry May 10, 2021

marc-sensenich May 11, 2021

mbfrahry May 10, 2021

marc-sensenich May 11, 2021

mbfrahry May 11, 2021

marc-sensenich May 13, 2021

mbfrahry left a comment

ghost commented May 14, 2021

marc-sensenich commented May 14, 2021

jebaraa commented May 19, 2021

github-actions bot commented Jun 18, 2021

@@ @@ -1248,6 +1248,15 @@ @@
                             </ul>
                           </li>
+                          <li>

	return fmt.Errorf("error in expanding`time_period`: %+v", err)
	return fmt.Errorf("error expanding`time_period`: %+v", err)

New Resource: azure_consumption_budget_* #9201

New Resource: azure_consumption_budget_* #9201

Conversation

marc-sensenich commented Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastianreloaded commented Jan 31, 2021

marc-sensenich commented Feb 5, 2021

mbfrahry 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

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

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

mbfrahry 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

mbfrahry left a comment

Choose a reason for hiding this comment

ghost commented May 14, 2021

marc-sensenich commented May 14, 2021

jebaraa commented May 19, 2021

github-actions bot commented Jun 18, 2021

New Resource: `azure_consumption_budget_*` #9201

New Resource: `azure_consumption_budget_*` #9201

marc-sensenich commented Nov 6, 2020 •

edited

Loading