New Resource: aws_budgets_budget

[xchapter7x]

building out budget resource functionality in order to provide the ability to create/configure budgets from the terraform aws provider

[https://github.com//issues/753]

[radeksimko]

Hi @xchapter7x ,
thanks for the contribution.

Do you mind submitting all vendor changes in a separate PR (we can merge such PR fairly quickly) to make the review of the real code easier?

[xchapter7x]

sure thing @radeksimko .

Here is the PR for the govendor of the budgets package #1898

[xchapter7x]

hi @radeksimko ,
this PR is ready for review. Whenever you have time to provide your thoughts and feedback, that would be much appreciated.

Side question, the docs say for new resources that terraform docs should be updated to reflect the new feature. Since the docs seem to be in a separate repo, I'm not sure what the desired flow is.

Let me know. Thanks.

-John

[xchapter7x]

Hey @radeksimko, just wanted to check in and see if you have some free time to review this PR? Feedback would be greatly appreciated. Let me know thanks.

[xchapter7x]

Hey @radeksimko ,

Any word on this PR? Any feedback you can provide would be much appreciated.
Thanks :)

[nicksantamaria]

I had a few thoughts while reviewing this PR and writing some docs.

• According to the AWS docs, time_period_end defaults to June 15, 2087 when created in the console. It would be good to mirror that behavior, and make the time_period_end argument optional.
• There are 3 include_ arguments provided by this resource, but the AWS CostTypes documentation shows there are 10 possible options. I would like to see support added for the following options:
  • include_credit
  • include_discount
  • include_other_subscription
  • include_recurring
  • include_refund
  • include_credit
  • include_support
  • include_upfront
• Add support for name_prefix as an alternative argument to name.

[xchapter7x]

hi @nicksantamaria ,

thanks for the PR feedback. Let me set aside some time to get some commits together for the suggested enhancements. Stay tuned. Ill put this back to [wip] in the meantime.

[xchapter7x]

hi @nicksantamaria ,

latest changes should reflect your suggested enhancements for name_prefix, cost_types & time_period_end.

any other suggestions or feedback would be most appreciated. Thanks.

• John

[xchapter7x]

hi @radeksimko & @nicksantamaria ,

Happy New Year!

Just wanted to check in and ping for the preferred path forward for this PR.
Any further guidance, thoughts or feedback on the implementation and functionality would be greatly appreciated.

Let me know. Thanks :)

John

[eperdeme]

Been testing this locally looks to work really well.
Is there anyway to also do notifications for budges like you can in the CLI/UI ? Ie, aws budgets create-budget --account-id X --budget X --notifications-with-subscribers X

https://docs.aws.amazon.com/cli/latest/reference/budgets/create-notification.html

[xchapter7x]

@eperdeme I agree the ability to configure notifications would be a valuable addition.

It was intentionally left out of this PR to control the scope. Reason being, I feel adding notifications support is more appropriate to be done as a feature enhancement, after the core value add of the budget resource is merged in.

[xchapter7x]

Hey @bflad ,

after a long pause, i think i've addressed the feedback items.

i did have 1 question to your feedback, which can be found in the comments, but outside of that, the PR should be in a good state.

let me know your thoughts.
thanks:)

-John

[bflad]

We should avoid this indirection -- it adds needless complexity to the resource. We can directly access objects under describeBudgetOutput as necessary and only expand non-scalars (like cost_types) as necessary.

The checks for nil values in the AWS response are also unnecessary (we should handle them gracefully and also d.Set() already appropriately handles them) and the d.GetOk() conditional before d.Set() breaks Terraform drift detection (we should always call d.Set() to refresh the Terraform state unless theres an edge case).

This whole thing can be simplified such as:

	// ... start of read function
	budget := describeBudgetOutput.Budget
	if budget == nil {
		log.Printf("[WARN] Budget %s not found, removing from state", d.Id())
		d.SetId("")
		return nil
	}

	d.Set("account_id", accountID)
	d.Set("budget_type", budget.BudgetType)

	if err := d.Set("cost_filters", convertCostFiltersToStringMap(budget.CostFilters)); err != nil {
		return fmt.Errorf("error setting cost_filters: %s", err)
	}

	if err := d.Set("cost_types", flattenBudgetsCostTypes(budget.CostTypes)); err != nil {
		return fmt.Errorf("error setting cost_filters: %s", err)
	}

	if budget.BudgetLimit != nil {
		d.Set("limit_amount", budget.BudgetLimit.Amount)
		d.Set("limit_unit", budget.BudgetLimit.Unit)
	}

	d.Set("name", budget.BudgetName)

	if budget.TimePeriod != nil {
		d.Set("time_period_end", budget.TimePeriod.End)
		d.Set("time_period_start", budget.TimePeriod.Start)
	}

	d.Set("time_unit", budget.TimeUnit)

	return nil
}

func flattenBudgetsCostTypes(costTypes *budgets.CostTypes) []map[string]interface{} {
	if costTypes == nil {
		return []map[string]interface{}{}
	}

	m := map[string]interface{}{
		"include_credit":             aws.BoolValue(costTypes.IncludeCredit),
		"include_discount":           aws.BoolValue(costTypes.IncludeDiscount),
		"include_other_subscription": aws.BoolValue(costTypes.IncludeOtherSubscription),
		"include_recurring":          aws.BoolValue(costTypes.IncludeRecurring),
		"include_refund":             aws.BoolValue(costTypes.IncludeRefund),
		"include_subscription":       aws.BoolValue(costTypes.IncludeSubscription),
		"include_support":            aws.BoolValue(costTypes.IncludeSupport),
		"include_tax":                aws.BoolValue(costTypes.IncludeTax),
		"include_upfront":            aws.BoolValue(costTypes.IncludeUpfront),
		"use_amortized":              aws.BoolValue(costTypes.UseAmortized),
		"use_blended":                aws.BoolValue(costTypes.UseBlended),
	}

	return []map[string]interface{}{m}
}

[xchapter7x]

@bflad ,

just pushed up changes in line with the last refactor suggestion.
If you could give it a look when you have a moment, and let me know if there are any further refactors, suggestions or thoughts.

thanks for your continued feedback and support in driving this to completion.

-John

[fmasuhr]

While I was using your implementation in a fork already I found some misleading parts in the documentation. It would be good to update them before merge this PR

[fmasuhr]

The documentation here is not fitting to the allowed values for the budget_type in AWS: https://docs.aws.amazon.com/aws-cost-management/latest/APIReference/API_budgets_Budget.html#awscostmanagement-Type-budgets_Budget-BudgetType

[xchapter7x]

👍
thanks for pointing these out @fmasuhr . ill try to push up a change this evening.

[fmasuhr]

This needs to include the time as well: 2017-07-01_00:00

[fmasuhr]

The unit to the cost has to be USD

[bflad]

Thanks so much for your efforts with this PR. Let's get this in. 🚀

2 tests passed (all tests)
=== RUN   TestAccAWSBudgetsBudget_prefix
--- PASS: TestAccAWSBudgetsBudget_prefix (16.61s)
=== RUN   TestAccAWSBudgetsBudget_basic
--- PASS: TestAccAWSBudgetsBudget_basic (18.72s)

[bflad]

This has been released in version 1.17.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[ghost]

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. Thanks!