-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Support locale in consumption budgets #23509
base: main
Are you sure you want to change the base?
Support locale in consumption budgets #23509
Conversation
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.
@stawik-mesa Thanks for opening this PR! This is mostly looking good, but we'll want to ensure that we have test coverage for this property for each of the updated resources. These can be added to the "complete" test config where one exists, or a test config where this block is already specified. Additionally, new optional properties in most cases require a default value to be specified in the schema, so that we don't incorrectly send blank values.
If you can look at adding these, I'll review again and run the tests and we should be in a good position to merge this. Please feel free to reach out if you'd like some additional pointers. Thanks!
"locale": { | ||
Type: pluginsdk.TypeString, | ||
Optional: true, | ||
ValidateFunc: validation.StringInSlice(budgets.PossibleValuesForCultureCode(), false), | ||
}, |
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.
As above we will probably need a default value here.
internal/services/consumption/migration/consumption_budget_subscription.go
Outdated
Show resolved
Hide resolved
internal/services/consumption/migration/consumption_budget_subscription.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Tom Bamford <tom@bamford.io>
@manicminer Thank you for the reviews. If the default value is used, a GET request doesn't have the property so its absent. |
@stawik-mesa Sorry for the delay in replying, I've commented inline (above) regarding the default value. |
Co-authored-by: Tom Harvey <tombuildsstuff@users.noreply.github.com>
internal/services/consumption/consumption_budget_management_group_resource.go
Show resolved
Hide resolved
@manicminer @tombuildsstuff Seems legit to make it computed. I have added it now. Is there anything else missing from my side? |
This PR is being labeled as "stale" because it has not been updated for 30 or more days. If this PR is still valid, please remove the "stale" label. If this PR is blocked, please add it to the "Blocked" milestone. If you need some help completing this PR, please leave a comment letting us know. Thank you! |
Can someone please take a look? @manicminer @tombuildsstuff ? |
Hi @stawik-mesa @manicminer @tombuildsstuff . |
This comment was marked as off-topic.
This comment was marked as off-topic.
Hi @manicminer & @tombuildsstuff any change to get this merged? Best regards |
Hey @stawik-mesa - i was looking at this today and while the PR looks good it appears the test are all failing now with:
should we be using en-us instead of en-US? |
internal/services/consumption/consumption_budget_subscription_resource_test.go
Outdated
Show resolved
Hide resolved
Hi @katbyte, I have changed it to en-us. Can you please run the workflows again to check if all issues are resolved now? Thank you ;) |
That does not appear to fix it :(
|
@stawik-mesa possibly forgotten to adjust within internal/services/consumption/consumption_budget_resource_group_resource_test.go |
@AndreasAugustin @katbyte Can you check again. hopefully it's fixed now. |
Resolves #22025