-
Notifications
You must be signed in to change notification settings - Fork 79
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
feat(subscription): add subscription budgets. #352
Conversation
modules/subscription/variables.tf
Outdated
enabled = bool | ||
operator = string # EqualTo, GreaterThan, GreaterThanOrEqualTo | ||
threshold = number # 0-1000 percent | ||
thresholdType = string # Actual, Forecasted |
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.
Question for the team - does it make sense to have this interface "look" like azurerm - e.g. the input name would be camel-cased, like "threshold_type", and the ones below like "contact_emails" etc.
My thoughts are if we do switch out the implementation for an AzureRM version, the interface will remain the same.
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.
Agreed, the inputs for all terraform modules should be snake case.
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 will add unnecessary work (in my opinion) and requires re-assembling all notifications just to bring them into the correct format for the azapi body
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.
I don't consider it unnecessary, it is something that needs to be once in the module to support creating a more consistent interface should the implementation change. It doesnt put burden on the module consumers.
It is similar to what has been done for container app managed envs, for example
https://github.com/Azure/terraform-azurerm-avm-res-app-managedenvironment/blob/main/main.tf
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.
added local to transform the notification map.
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.
Thank you very much for submitting this PR. I appreciate this ask is extra work, but the consistency of naming conventions will be beneficial in the longer term. Thanks
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 code looks really good, just think we can improve the user experience a bit.
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.
LGTM - one comment!
Overview/summary
This PR adds the ability to define cost management budgets for a subscription, excluding filters.
This PR fixes/adds/changes/removes
Breaking changes
None
Testing evidence
I am not familiar enough with Go to have added tests. I tested instead in my environment.
Apply was successfully completed, e.g.:
As part of this pull request I have
make fmt
&make docs
to format your code and update documentation.