Add AzureRM Service Bus Queue resource #14631

iljaskevic · 2017-05-18T15:45:47Z

This PR adds Azure RM - Service Bus Queue resource. Any ideas on how to improve this are welcome.

stack72

hi @iljaskevic

Thanks for all the work here - I have left you some comments to think about

Thanks

Paul

stack72 · 2017-05-19T10:29:39Z

builtin/providers/azurerm/resource_arm_servicebus_queue.go

+
+			"enable_batched_operations": {
+				Type:     schema.TypeBool,
+				Optional: true,


We should provide a default here. We have a known bug in Terraform that if you specify a value of true then change back to false then terraform won't get that change

this is because of d.GetOk("false") believing that the value doesn't exist as the default value for a bool is false

stack72 · 2017-05-19T10:29:45Z

builtin/providers/azurerm/resource_arm_servicebus_queue.go

+			},
+
+			"enable_express": {
+				Type:     schema.TypeBool,


Same default value point

stack72 · 2017-05-19T10:29:55Z

builtin/providers/azurerm/resource_arm_servicebus_queue.go

+
+			"enable_partitioning": {
+				Type:     schema.TypeBool,
+				Optional: true,


Same default value point

stack72 · 2017-05-19T10:30:01Z

builtin/providers/azurerm/resource_arm_servicebus_queue.go

+			},
+
+			"requires_duplicate_detection": {
+				Type:     schema.TypeBool,


Same default value point

stack72 · 2017-05-19T10:30:39Z

builtin/providers/azurerm/resource_arm_servicebus_queue.go

+			},
+
+			"support_ordering": {
+				Type:     schema.TypeBool,


Same default value point

stack72 · 2017-05-19T10:32:19Z

builtin/providers/azurerm/resource_arm_servicebus_queue.go

+	d.Set("namespace_name", namespaceName)
+	d.Set("location", azureRMNormalizeLocation(*resp.Location))
+
+	props := resp.QueueProperties


I think we should check that QueueProperties is not nil before trying to get values from it - otherwise this could panic

stack72 · 2017-05-19T10:32:45Z

builtin/providers/azurerm/resource_arm_servicebus_queue.go

+	d.Set("auto_delete_on_idle", props.AutoDeleteOnIdle)
+	d.Set("default_message_ttl", props.DefaultMessageTimeToLive)
+
+	if props.DuplicateDetectionHistoryTimeWindow != nil && *props.DuplicateDetectionHistoryTimeWindow != "" {


d.Set will safely do this check for you

stack72 · 2017-05-19T10:33:27Z

builtin/providers/azurerm/resource_arm_servicebus_queue.go

+	// if the queue is in a premium namespace and partitioning is enabled then the
+	// max size returned by the API will be 16 times greater than the value set
+	if *props.EnablePartitioning {
+		namespace, err := meta.(*ArmClient).serviceBusNamespacesClient.Get(resGroup, namespaceName)


Why do you need to make another Get request here?

It needs to retrieve the parent Namespace resource where the queue is running to get the size (Basic, Standard or Premium) to be able to re-calculate the max size property differently for Basic and Standard sizes than for Premium size, when getting the queue info from Azure.

In Azure Service Bus, if partitioning is enabled and maximum queue size is set to N megabytes, when getting the max size property of an existing queue we get N MB for Premium and N*16 MB for Basic and Standard. That is because Basic and Standard are running on shared resources and it creates 16 copies/partitions, which means that total max size is 16 times the value that was set. However, Premium runs on dedicated resources and partitioning is always enabled with 2 partitions with max size split between the them. This means that each partition has max size of N/2 MB and the total max size remains N MB.

So to avoid a problem of discrepancy between the value of the property we set when creating the queue and the value of that property we get when we retrieve it from Azure after it was created, it needs to divide the property value by 16 for queues running in Basic and Standard namespaces.

stack72 · 2017-05-24T10:35:09Z

The extra API call makes sense now - thanks for explaining that. Please can you just touch up the other points I made around d.Set and default values for bools and then we can get this merged

Thanks

Paul

…erties for Premium queues, as well as, updated acceptance tests and docs.

…m-servicebus-queue

…es from upstream

stack72 · 2017-06-09T14:04:35Z

Hi @iljaskevic

The code LGTM to me, looks like there is a test failure:

% make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccAzureRMServiceBusQueue'                                                ✭
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/06/09 16:57:53 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/azurerm -v -run=TestAccAzureRMServiceBusQueue -timeout 120m
=== RUN   TestAccAzureRMServiceBusQueue_importBasic
--- FAIL: TestAccAzureRMServiceBusQueue_importBasic (229.58s)
	testing.go:428: Step 0 error: After applying this step, the plan was not empty:

		DIFF:

		UPDATE: azurerm_servicebus_queue.test
		  duplicate_detection_history_time_window: "00:10:00" => ""

		STATE:

		azurerm_resource_group.test:
		  ID = /subscriptions/c0a607b2-6372-4ef3-abdb-dbe52a7b56ba/resourceGroups/acctestRG-6423544310546596076
		  location = westus
		  name = acctestRG-6423544310546596076
		  tags.% = 0
		azurerm_servicebus_namespace.test:
		  ID = /subscriptions/c0a607b2-6372-4ef3-abdb-dbe52a7b56ba/resourceGroups/acctestRG-6423544310546596076/providers/Microsoft.ServiceBus/namespaces/acctestservicebusnamespace-6423544310546596076
		  capacity = 1
		  default_primary_connection_string = Endpoint=sb://acctestservicebusnamespace-6423544310546596076.servicebus.windows.net/;SharedAccessKeyName=RootManageSharedAccessKey;SharedAccessKey=o1910MlqSDp7bECRiHm/EZhb0aPuWbt5qN4cry4JeRw=
		  default_primary_key = o1910MlqSDp7bECRiHm/EZhb0aPuWbt5qN4cry4JeRw=
		  default_secondary_connection_string = Endpoint=sb://acctestservicebusnamespace-6423544310546596076.servicebus.windows.net/;SharedAccessKeyName=RootManageSharedAccessKey;SharedAccessKey=IbP87TaRXjKDCPVYuuSj3wGefCSf11UZOcFCecWDBxI=
		  default_secondary_key = IbP87TaRXjKDCPVYuuSj3wGefCSf11UZOcFCecWDBxI=
		  location = westus
		  name = acctestservicebusnamespace-6423544310546596076
		  resource_group_name = acctestRG-6423544310546596076
		  sku = standard
		  tags.% = 0

		  Dependencies:
		    azurerm_resource_group.test
		azurerm_servicebus_queue.test:
		  ID = /subscriptions/c0a607b2-6372-4ef3-abdb-dbe52a7b56ba/resourceGroups/acctestRG-6423544310546596076/providers/Microsoft.ServiceBus/namespaces/acctestservicebusnamespace-6423544310546596076/queues/acctestservicebusqueue-6423544310546596076
		  auto_delete_on_idle = 10675199.02:48:05.4775807
		  default_message_ttl = 10675199.02:48:05.4775807
		  duplicate_detection_history_time_window = 00:10:00
		  enable_batched_operations = false
		  enable_express = false
		  enable_partitioning = false
		  location = westus
		  max_size_in_megabytes = 5120
		  name = acctestservicebusqueue-6423544310546596076
		  namespace_name = acctestservicebusnamespace-6423544310546596076
		  requires_duplicate_detection = false
		  resource_group_name = acctestRG-6423544310546596076
		  support_ordering = false

		  Dependencies:
		    azurerm_resource_group.test
		    azurerm_servicebus_namespace.test

Please can you look into the tests?

Thanks

Paul

iljaskevic · 2017-06-09T14:26:06Z

Hi @stack72
Hmmm. That's interesting...
I've ran all the tests locally and they all passed. I'll take a look into why it's failing.
Thanks,

Igor

tombuildsstuff · 2017-07-04T16:00:19Z

Hey @iljaskevic

Since this PR was opened the AzureRM Provider has been moved out of this repository as part of the Provider Split in Terraform 0.10. Due to this the PR needed to be moved to the new split out repository - I hope you don't mind but I've gone ahead and done this (and retained your authorship for the original commit), and fixed the failing test that @stack72 identified and opened PR hashicorp/terraform-provider-azurerm#151

As such - I'm going to close this PR in favour of the split-out one - but I'd like to thank you for your contribution :)

Thanks!

@iljaskevic

* Importing the work from hashicorp/terraform#14631 by @iljaskevic * Refactoring the tests * Formatting * Importing the docs * Refactoring * Making `duplicate_detection_history_time_window` computed * Refactoring the tests * Fixing the formatting

ghost · 2020-04-08T02:18:08Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

Added AzureRM Service Bus Queue resource

2cd8759

grubernaut added enhancement new-resource provider/azurerm labels May 18, 2017

stack72 suggested changes May 19, 2017

View reviewed changes

iljaskevicarchitech added 5 commits May 31, 2017 13:43

Added default values for booleans to avoid existing Terraform bug

00ff975

Added sanity check and removed redundant check for nil or empty property

6c8507a

Added checks for enforcing ‘partitioning’ and ‘express entities’ prop…

9da819e

…erties for Premium queues, as well as, updated acceptance tests and docs.

Merge branch 'master' of github.com:hashicorp/terraform into f-azurer…

14696c9

…m-servicebus-queue

Updated PR to match Service Bus client UserAgent and Authorizer updat…

62d14ce

…es from upstream

tombuildsstuff self-assigned this Jul 4, 2017

tombuildsstuff added a commit to hashicorp/terraform-provider-azurerm that referenced this pull request Jul 4, 2017

Importing the work from hashicorp/terraform#14631 by @iljaskevic

5c60f7c

tombuildsstuff mentioned this pull request Jul 4, 2017

New Resource: azurerm_servicebus_queue hashicorp/terraform-provider-azurerm#151

Merged

tombuildsstuff pushed a commit to hashicorp/terraform-provider-azurerm that referenced this pull request Jul 4, 2017

Importing the work from hashicorp/terraform#14631 by @iljaskevic

18abbac

tombuildsstuff pushed a commit to hashicorp/terraform-provider-azurerm that referenced this pull request Jul 4, 2017

Importing the work from hashicorp/terraform#14631 by @iljaskevic

a6cb8ad

tombuildsstuff closed this Jul 4, 2017

sebastus mentioned this pull request Jul 12, 2017

catch up with the team. (#1) hashicorp/terraform-provider-azurerm#165

Closed

ghost locked and limited conversation to collaborators Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AzureRM Service Bus Queue resource #14631

Add AzureRM Service Bus Queue resource #14631

iljaskevic commented May 18, 2017

stack72 left a comment

stack72 May 19, 2017

stack72 May 19, 2017

stack72 May 19, 2017

stack72 May 19, 2017

stack72 May 19, 2017

stack72 May 19, 2017

stack72 May 19, 2017

stack72 May 19, 2017

iljaskevic May 21, 2017

stack72 commented May 24, 2017

stack72 commented Jun 9, 2017

iljaskevic commented Jun 9, 2017

tombuildsstuff commented Jul 4, 2017

ghost commented Apr 8, 2020

Add AzureRM Service Bus Queue resource #14631

Add AzureRM Service Bus Queue resource #14631

Conversation

iljaskevic commented May 18, 2017

stack72 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stack72 commented May 24, 2017

stack72 commented Jun 9, 2017

iljaskevic commented Jun 9, 2017

tombuildsstuff commented Jul 4, 2017

ghost commented Apr 8, 2020