`azurerm_servicebus_topic` - added a `status` field to allow disabling the topic #150

tombuildsstuff · 2017-07-04T11:37:45Z

Allows Service Bus Topic's to be set to a Disabled state.

Fixes #146

Tests pass:

$ TF_ACC=1 envchain azurerm go test ./azurerm -v -timeout 120m -run TestAccAzureRMServiceBusTopic
=== RUN   TestAccAzureRMServiceBusTopic_importBasic
--- PASS: TestAccAzureRMServiceBusTopic_importBasic (206.31s)
=== RUN   TestAccAzureRMServiceBusTopic_importBasicDisabled
--- PASS: TestAccAzureRMServiceBusTopic_importBasicDisabled (207.14s)
=== RUN   TestAccAzureRMServiceBusTopic_basic
--- PASS: TestAccAzureRMServiceBusTopic_basic (202.54s)
=== RUN   TestAccAzureRMServiceBusTopic_basicDisabled
--- PASS: TestAccAzureRMServiceBusTopic_basicDisabled (207.77s)
=== RUN   TestAccAzureRMServiceBusTopic_basicDisableEnable
--- PASS: TestAccAzureRMServiceBusTopic_basicDisableEnable (251.32s)
=== RUN   TestAccAzureRMServiceBusTopic_update
--- PASS: TestAccAzureRMServiceBusTopic_update (219.43s)
=== RUN   TestAccAzureRMServiceBusTopic_enablePartitioningStandard
--- PASS: TestAccAzureRMServiceBusTopic_enablePartitioningStandard (233.38s)
=== RUN   TestAccAzureRMServiceBusTopic_enablePartitioningPremium
--- PASS: TestAccAzureRMServiceBusTopic_enablePartitioningPremium (527.34s)
=== RUN   TestAccAzureRMServiceBusTopic_enableDuplicateDetection
--- PASS: TestAccAzureRMServiceBusTopic_enableDuplicateDetection (250.84s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	2306.085s

radeksimko

This looks overall good. I left you some comments there out of which 1 is probably more significant than others - pointer helper functions. Let me know what you think.

radeksimko · 2017-07-04T17:52:06Z

azurerm/resource_arm_servicebus_topic.go

+			EnablePartitioning:                &enablePartitioning,
+			MaxSizeInMegabytes:                &maxSize,
+			RequiresDuplicateDetection:        &requiresDuplicateDetection,
+			SupportOrdering:                   &supportOrdering,


I think we'd benefit from having a provider-wide helper for creating these pointers here. AWS has its own in the SDK (aws.String(), aws.Bool() etc.) and I built similar ones in K8S: https://github.com/terraform-providers/terraform-provider-kubernetes/blob/master/kubernetes/structures.go#L124-L142

It may look like a nitpick, but you can effectively reduce the number of lines to half here.

👍 good shout - there's a ton built into Riviera - I'll switch this out

radeksimko · 2017-07-04T17:53:06Z

azurerm/resource_arm_servicebus_topic.go

@@ -186,6 +200,7 @@ func resourceArmServiceBusTopicRead(d *schema.ResourceData, meta interface{}) er
 	d.Set("location", azureRMNormalizeLocation(*resp.Location))

 	props := resp.TopicProperties
+	d.Set("status", string(props.Status))


I'd think the Set will take care of the casting, but maybe I'm wrong... 🤷‍♂️

radeksimko · 2017-07-04T17:53:54Z

azurerm/resource_arm_servicebus_topic_test.go

+			{
+				Config: enabledConfig,
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMServiceBusTopicExists(resourceName),


Good thorough testing here, nice 👍

radeksimko · 2017-07-04T17:56:44Z

azurerm/resource_arm_servicebus_namespace.go

+					string(servicebus.Basic),
+					string(servicebus.Standard),
+					string(servicebus.Premium),
+				}, true),


Good job with simplification 👍

radeksimko · 2017-07-04T17:57:27Z

azurerm/resource_arm_servicebus_namespace.go

@@ -134,7 +139,7 @@ func resourceArmServiceBusNamespaceRead(d *schema.ResourceData, meta interface{}

 	resp, err := namespaceClient.Get(resGroup, name)
 	if err != nil {
-		return fmt.Errorf("Error making Read request on Azure ServiceBus Namespace %s: %+v", name, err)
+		return fmt.Errorf("Error making Read request on Azure ServiceBus Namespace '%s': %+v", name, err)


%q would add double quotes around the string for you + escaped it properly here.

TIL, thanks!

tombuildsstuff · 2017-07-05T09:04:56Z

Tests pass:

$ TF_ACC=1 envchain azurerm go test ./azurerm -v -timeout 120m -run TestAccAzureRMServiceBusTopic
=== RUN   TestAccAzureRMServiceBusTopic_importBasic
--- PASS: TestAccAzureRMServiceBusTopic_importBasic (225.37s)
=== RUN   TestAccAzureRMServiceBusTopic_importBasicDisabled
--- PASS: TestAccAzureRMServiceBusTopic_importBasicDisabled (224.31s)
=== RUN   TestAccAzureRMServiceBusTopic_basic
--- PASS: TestAccAzureRMServiceBusTopic_basic (220.86s)
=== RUN   TestAccAzureRMServiceBusTopic_basicDisabled
--- PASS: TestAccAzureRMServiceBusTopic_basicDisabled (226.15s)
=== RUN   TestAccAzureRMServiceBusTopic_basicDisableEnable
--- PASS: TestAccAzureRMServiceBusTopic_basicDisableEnable (252.59s)
=== RUN   TestAccAzureRMServiceBusTopic_update
--- PASS: TestAccAzureRMServiceBusTopic_update (239.40s)
=== RUN   TestAccAzureRMServiceBusTopic_enablePartitioningStandard
--- PASS: TestAccAzureRMServiceBusTopic_enablePartitioningStandard (219.67s)
=== RUN   TestAccAzureRMServiceBusTopic_enablePartitioningPremium
--- PASS: TestAccAzureRMServiceBusTopic_enablePartitioningPremium (524.82s)
=== RUN   TestAccAzureRMServiceBusTopic_enableDuplicateDetection
--- PASS: TestAccAzureRMServiceBusTopic_enableDuplicateDetection (241.51s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	2374.706s

ghost · 2020-04-01T15:34:15Z

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!

tombuildsstuff added the enhancement label Jul 4, 2017

tombuildsstuff mentioned this pull request Jul 4, 2017

Feature Request: Allow Service Bus Topic Subscriptions to be created in a "disabled" state #146

Closed

radeksimko approved these changes Jul 4, 2017

View reviewed changes

tombuildsstuff force-pushed the servicebus-disabled branch from 96e9e26 to 82792ff Compare July 5, 2017 08:56

tombuildsstuff added 6 commits July 5, 2017 10:04

Cleaning up service bus namespaces

409f6c5

Service Bus Topic's can be disabled

1c68d84

Refactoring

20cca85

Actually enabling disabled

da0eb1e

Adding the new field to the docs

55a30f0

Switching to use the azure wrappers

1eebdf4

tombuildsstuff force-pushed the servicebus-disabled branch from 82792ff to 1eebdf4 Compare July 5, 2017 09:04

tombuildsstuff merged commit 894e0d0 into master Jul 5, 2017

tombuildsstuff deleted the servicebus-disabled branch July 5, 2017 09:29

tombuildsstuff added a commit that referenced this pull request Jul 5, 2017

Updating to include #150

806dbf3

sebastus mentioned this pull request Jul 12, 2017

catch up with the team. (#1) #165

Closed

ghost locked and limited conversation to collaborators Apr 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_servicebus_topic` - added a `status` field to allow disabling the topic #150

`azurerm_servicebus_topic` - added a `status` field to allow disabling the topic #150

tombuildsstuff commented Jul 4, 2017

radeksimko left a comment

radeksimko Jul 4, 2017

tombuildsstuff Jul 5, 2017

radeksimko Jul 4, 2017

radeksimko Jul 4, 2017

radeksimko Jul 4, 2017

radeksimko Jul 4, 2017

tombuildsstuff Jul 4, 2017

tombuildsstuff commented Jul 5, 2017

ghost commented Apr 1, 2020

azurerm_servicebus_topic - added a status field to allow disabling the topic #150

azurerm_servicebus_topic - added a status field to allow disabling the topic #150

Conversation

tombuildsstuff commented Jul 4, 2017

radeksimko left a comment

Choose a reason for hiding this comment

radeksimko Jul 4, 2017

Choose a reason for hiding this comment

tombuildsstuff Jul 5, 2017

Choose a reason for hiding this comment

radeksimko Jul 4, 2017

Choose a reason for hiding this comment

radeksimko Jul 4, 2017

Choose a reason for hiding this comment

radeksimko Jul 4, 2017

Choose a reason for hiding this comment

radeksimko Jul 4, 2017

Choose a reason for hiding this comment

tombuildsstuff Jul 4, 2017

Choose a reason for hiding this comment

tombuildsstuff commented Jul 5, 2017

ghost commented Apr 1, 2020

`azurerm_servicebus_topic` - added a `status` field to allow disabling the topic #150

`azurerm_servicebus_topic` - added a `status` field to allow disabling the topic #150