-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
consumption
- remove 4.0 flag and fix tests
#27511
Conversation
@teowa - how come this is still in draft? |
…m into fix-consumption-test
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.
Thanks @teowa. Could you take a look at the comments?
internal/services/consumption/consumption_budget_resource_group_resource_test.go
Show resolved
Hide resolved
@@ -153,13 +152,10 @@ func (br consumptionBudgetBaseResource) arguments(fields map[string]*pluginsdk.S | |||
ValidateFunc: validation.IntBetween(0, 1000), | |||
}, | |||
// Issue: https://github.com/Azure/azure-rest-api-specs/issues/16240 |
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.
Looks like this comment should also be removed?
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.
updated
if resp, err := client.Delete(ctx, *id); err != nil { | ||
if !response.WasNotFound(resp.HttpResponse) { | ||
return fmt.Errorf("deleting %s: %+v", *id, err) | ||
} |
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.
We don't usually need to specifically check for a 404
when deleting. If the resource is disappearing before we're able to delete it then that would imply we're not marking this as gone in the read or something is going wrong before we're calling the delete.
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.
Actually this is to fix the below TestAccConsumptionBudgetResourceGroup_disappears
failure, the testcase tries to perform post-test destroy even the resource has been deleted and seems SDK will fire error if delete returns 404. There are many other _disappears
testcase but it won't fail if API returns 204 for deleting an non-existing resource. Or should we remove the below testcase?
------- Stdout: -------
=== RUN TestAccConsumptionBudgetResourceGroup_disappears
=== PAUSE TestAccConsumptionBudgetResourceGroup_disappears
=== CONT TestAccConsumptionBudgetResourceGroup_disappears
testcase.go:173: Error running post-test destroy, there may be dangling resources: exit status 1
Error: deleting Scoped Budget (Scope: "/subscriptions/*******/resourceGroups/acctestRG-241023001722601091"
Budget Name: "acctestconsumptionbudgetresourcegroup-241023001722601091"): unexpected status 404 (404 Not Found) with error: 404: No budget found matching budgetName: acctestconsumptionbudgetresourcegroup-241023001722601091, under storageScope: EntityType = Subscription, EntityId = *******, ChildScope = EntityType = ResourceGroup, EntityId = acctestRG-241023001722601091, ChildScope = , MetaData = , MetaData = (Request ID: 669a79c6-e268-4a25-bed9-78f2cc7d54ab)
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.
These _disappears
test cases are an old pattern that we don't really test for in the provider anymore. I believe @jackofallops has begun removing instances of these in the provider, so this test case should be removed.
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.
That makes sense!
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.
Thanks @teowa! LGTM 💯
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. |
Community Note
Description
remove 4.0 flag and fix tests
PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Testing
Need to run test
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
This is a (please select all that apply):
Related Issue(s)
Fixes #0000
Note
If this PR changes meaningfully during the course of review please update the title and description as required.