Skip to content
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

ServiceBus Namespace: polling for the deletion #1908

Merged
merged 2 commits into from
Sep 12, 2018
Merged

ServiceBus Namespace: polling for the deletion #1908

merged 2 commits into from
Sep 12, 2018

Conversation

lrxtom2
Copy link
Contributor

@lrxtom2 lrxtom2 commented Sep 11, 2018

Deleting service bus in tanscation that resource group deletion will be blocked when sku set to premium.
Added WaitForCompletionRef method to make sure service bus was deleted and also added test for premium sku.

Thanks,
Chang

@ghost ghost added the size/M label Sep 11, 2018
Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR Chang, LGTM.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lrxtom2,

Thank you for the bugfix. I've left a few minor comments inline that should be addressed before this is merged.

@@ -212,10 +213,19 @@ func resourceArmServiceBusNamespaceDelete(d *schema.ResourceData, meta interface
resourceGroup := id.ResourceGroup
name := id.Path["namespaces"]

_, err = client.Delete(ctx, resourceGroup, name)
deleteFuture, err := client.Delete(ctx, resourceGroup, name)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we call this future to match the rest of the codebase?

if err != nil {
return err
}

err = deleteFuture.WaitForCompletionRef(ctx, client.Client)
if err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines could be combines as such:

if err := deleteFuture.WaitForCompletionRef(ctx, client.Client); err != nil {

func testAccAzureRMServiceBusNamespace_premium(rInt int, location string) string {
return fmt.Sprintf(`
resource "azurerm_resource_group" "test" {
name = "acctestRG-%d"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we align these assignments?

name = "acctestRG-%d"
location = "%s"
}
resource "azurerm_servicebus_namespace" "test" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And add a space here between the resources

@ghost ghost added the size/M label Sep 12, 2018
@lrxtom2
Copy link
Contributor Author

lrxtom2 commented Sep 12, 2018

@katbyte Thanks for review, updated.

Copy link
Member

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @lrxtom2

Thanks for pushing those changes - this now LGTM 👍

Thanks!

@tombuildsstuff tombuildsstuff dismissed katbyte’s stale review September 12, 2018 15:02

changes have been pushed

@tombuildsstuff
Copy link
Member

Tests pass:

screenshot 2018-09-12 at 17 18 05

@tombuildsstuff tombuildsstuff changed the title Service bus fixed when sku set to premium ServiceBus Namespace: polling for the deletion Sep 12, 2018
@tombuildsstuff tombuildsstuff changed the title Service bus fixed when sku set to premium ServiceBus Namespace: polling for the deletion Sep 12, 2018
@tombuildsstuff tombuildsstuff merged commit cadce83 into hashicorp:master Sep 12, 2018
tombuildsstuff added a commit that referenced this pull request Sep 12, 2018
@lrxtom2 lrxtom2 deleted the serviceBusFix branch September 13, 2018 01:07
@ghost
Copy link

ghost commented Mar 6, 2019

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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants