New Resource: `resource_arm_servicebus_subscription_rule` #1124

jpovey · 2018-04-15T21:03:04Z

Add AzureRM Servicebus rule module, tests and documentation. Applies to feature request #145

…tion

tombuildsstuff

Thanks for this PR - I've taken a look through and left some comments in-line, but this is off to a good start. If we can clean up the comments then we should be good to run the tests :)

Thanks!

tombuildsstuff · 2018-04-16T07:55:40Z

azurerm/resource_arm_servicebus_rule.go

+		}
+
+		if properties.CorrelationFilter != nil {
+			if err := d.Set("correlation_filter", flattenAzureRmServiceBusCorrelationFilter(properties.CorrelationFilter)); err != nil {


can we update this to always set the correlation_filter field - but only to return items if properties.CorrelationFilter isn't nil? e.g.

if err := d.Set("correlation_filter", flattenAzureRmServiceBusCorrelationFilter(properties.CorrelationFilter)); err != nil { return fmt.Errorf("...") } func flattenAzureRmServiceBusCorrelationFilter(input *servicebus.CorrelationFilter) []interface{} { if input == nil { return []interface{}{} } output := make(map[string]interface{}, 0) // assignment return []interface{}{output} }

tombuildsstuff · 2018-04-16T07:56:08Z

azurerm/resource_arm_servicebus_rule.go

+
+			"correlation_filter": {
+				Type:     schema.TypeList,
+				Optional: true,


from what I can see from the schema below, there can only be one of these, can we add MaxItems: 1 to this?

we can also add ConflictsWith: []string{"sql_filter"} here which should handle only one of them being specified

tombuildsstuff · 2018-04-16T07:57:08Z

azurerm/resource_arm_servicebus_rule.go

+	}
+
+	rule := servicebus.Rule{
+		Ruleproperties: &ruleProperties,


minor: we can actually just inline these two objects e.g.

rule := servicebus.Rule{ Ruleproperties: &servicebus.Ruleproperties{ FilterType: servicebus.FilterType(filterType), // .. }, }

tombuildsstuff · 2018-04-16T07:58:39Z

azurerm/provider.go

@@ -190,6 +190,7 @@ func Provider() terraform.ResourceProvider {
 			"azurerm_servicebus_namespace":                resourceArmServiceBusNamespace(),
 			"azurerm_servicebus_queue":                    resourceArmServiceBusQueue(),
 			"azurerm_servicebus_subscription":             resourceArmServiceBusSubscription(),
+			"azurerm_servicebus_rule":                     resourceArmServiceBusRule(),


from what I can tell, these are known as ServiceBus Subscription Rules - as such would it make sense for this to be azurerm_servicebus_subscription_rule?

Hi Tom

I did consider this but in both the Service Bus documentation and in the go SDK they are known simply as 'rules' and i wanted to keep consistency with this terraform module.

https://docs.microsoft.com/en-us/rest/api/servicebus/rules/createorupdate

https://github.com/Azure/azure-sdk-for-go/blob/56332fec5b308fbb6615fa1af6117394cdba186d/services/servicebus/mgmt/2017-04-01/servicebus/rules.go

so the Documentation and Go SDK are actually both generated from the same Swagger file - so the Azure Portal tends to be a more reliable source of truth. Given we've already got support for ServiceBus Topic Authorization Rules, I think this would be best as Subscription Rule (given it's scoped to that level) in-case a Namespace-wide rule is added in the future - WDYT?

Ok, sounds good to me. Will get this sorted and updated soon

tombuildsstuff · 2018-04-16T07:59:20Z

azurerm/resource_arm_servicebus_rule.go

+				DiffSuppressFunc: ignoreCaseDiffSuppressFunc,
+			},
+
+			"location": deprecatedLocationSchema(),


I think we can remove this, since it's not being used?

tombuildsstuff · 2018-04-16T15:50:10Z

azurerm/resource_arm_servicebus_rule_test.go

+	topic_name          = "${azurerm_servicebus_topic.test.name}"
+	subscription_name   = "${azurerm_servicebus_subscription.test.name}"
+	resource_group_name = "${azurerm_resource_group.test.name}"	
+	%s


can we duplicate this into each test and make the spacing consistent here?

Is there any reason to duplicate the topic, subscription and resource group as these will be the same for each test?

we tend to point people towards the test cases as examples of how to use features when people ask for end-to-end examples; the main goal here is to make them - so the aim here is to make them as readable as possible; I think it's fine to duplicate the topic, subscription and resource group - but can we add the entire resource definition for the azurerm_servicebus_subscription_rule into each test? e.g.

resource "azurerm_servicebus_subscription_rule" "test" { # ... }

Ok sure, i get what you mean

tombuildsstuff · 2018-04-16T15:50:48Z

website/docs/r/servicebus_rule.html.markdown

+  Create a ServiceBus Rule.
+---
+
+# azurerm\_servicebus\_rule


minor we can remove the backslashes here

tombuildsstuff · 2018-04-16T15:51:12Z

website/docs/r/servicebus_rule.html.markdown

+
+* `sql_filter` - (Optional) Represents a filter written in SQL language-based syntax that to be evaluated against a BrokeredMessage. Must be set when `filter_type` is set to `SqlFilter`.
+
+* `correlation_filter` - (Optional) A `correlation_filter` block as documented below to be evaluated against a BrokeredMessage. Must be set when `filter_type` is set to `CorrelationFilter`.


Can we change Must be set -> Required for consistency purposes?

tombuildsstuff · 2018-04-16T15:51:17Z

website/docs/r/servicebus_rule.html.markdown

+
+* `filter_type` - (Required) Type of filter to be applied to a BrokeredMessage. Possible values are `SqlFilter` and `CorrelationFilter`.
+
+* `sql_filter` - (Optional) Represents a filter written in SQL language-based syntax that to be evaluated against a BrokeredMessage. Must be set when `filter_type` is set to `SqlFilter`.


Can we change Must be set -> Required for consistency purposes?

tombuildsstuff · 2018-04-16T15:52:09Z

azurerm/resource_arm_servicebus_rule.go

+			correlationFilter.ContentType = &contentType
+		}
+
+		return &correlationFilter


based on this comment: https://github.com/terraform-providers/terraform-provider-azurerm/pull/1124/files#diff-5ae250cddcb7acd8b53f87d1465954bfR168 - is it worth raising an error if no properties have been set?

jpovey · 2018-04-17T09:13:18Z

Morning Tom,
Thanks for the feedback, I have made the suggested minor fixes above and will look to change the rest over the next few days
Cheers

Jon

…om being set at the same time

…set in correlation block

jpovey · 2018-04-19T11:18:32Z

Hi @tombuildsstuff

I have updated the pull request with all the changes above apart from changing the modules name to azurerm_servicebus_subscription_rule as per my comment above

Let me know what you think

Cheers
Jon

…zure portal and dependency on service_bus_subscription

… more than 50 chars

jpovey · 2018-04-20T11:29:20Z

Hi @tombuildsstuff

All changes have been made including change the name of the module to resource_arm_servicebus_subscription_rule and pushed up to the pull request

Let me know if there is anything else which needs to be done and i will try and get on it over the weekend

cheers
Jon

tombuildsstuff · 2018-04-20T15:57:34Z

Acceptance Tests pass:

tombuildsstuff

hey @jpovey

Thanks for pushing those updates - I've taken a look through and whilst this needs some minor refactoring to make it consistent with the other resources (which I'll open a PR for) - this otherwise LGTM 👍

Thanks!

jpovey · 2018-04-20T20:24:40Z

Awesome, that's great news.

Thanks for the quick review process.

vantheshark · 2019-02-20T03:31:44Z

I could not find this in the terraform doc for azure. Could you please update the doc too?

katbyte · 2019-02-21T18:49:58Z

Are these the docs you're looking for @vanthoainguyen ?

vantheshark · 2019-02-21T23:41:47Z

These are, thanks 🤦‍♂️

ghost · 2019-03-05T13:51:12Z

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 👉 [email protected]. Thanks!

Add AzureRM Servicebus rule module, corresponding tests and documenta…

39fd30f

…tion

jpovey mentioned this pull request Apr 15, 2018

Feature Request: Service Bus Subscription Rule #145

Closed

tombuildsstuff changed the title ~~Add AzureRM Servicebus rule module~~ New Resource: azurerm_servicebus_rule Apr 16, 2018

tombuildsstuff added new-resource service/service-bus labels Apr 16, 2018

tombuildsstuff added this to the Soon milestone Apr 16, 2018

tombuildsstuff self-assigned this Apr 16, 2018

tombuildsstuff suggested changes Apr 16, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Apr 16, 2018

tombuildsstuff removed their assignment Apr 16, 2018

Minor fixes to the servicebus rule

4795beb

jpovey added 4 commits April 17, 2018 16:00

Explicitly define the service bus rule in each test

4511fe1

Set correlation_filter to conflict with sql_filter to prevent both fr…

e76dbc7

…om being set at the same time

Raise an error when creating a correlation_filter with no properties …

65107b9

…set in correlation block

Always return the correlation_filter but only include properties if set

6138226

jpovey added 3 commits April 20, 2018 09:41

Rename service_bus_rule to service_bus_subscription_rule to relfect a…

d79e461

…zure portal and dependency on service_bus_subscription

Update documentation for service bus subscription rule

f38c500

Service bus subscription rule: Add validation to rule name, cannot be…

40fd30f

… more than 50 chars

tombuildsstuff removed the waiting-response label Apr 20, 2018

Fixing the descriptions for the name fields

1f834d9

tombuildsstuff approved these changes Apr 20, 2018

View reviewed changes

tombuildsstuff merged commit 014cd6c into hashicorp:master Apr 20, 2018

tombuildsstuff added a commit that referenced this pull request Apr 20, 2018

Updating to include #1124

1b508d4

tombuildsstuff mentioned this pull request Apr 23, 2018

Refactoring ServiceBus Subscription Rule #1150

Merged

tombuildsstuff changed the title ~~New Resource: azurerm_servicebus_rule~~ New Resource: resource_arm_servicebus_subscription_rule Apr 25, 2018

tombuildsstuff changed the title ~~New Resource: resource_arm_servicebus_subscription_rule~~ New Resource: resource_arm_servicebus_subscription_rule Apr 25, 2018

katbyte modified the milestones: Soon, 1.4.0 May 9, 2018

ghost locked and limited conversation to collaborators Mar 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: `resource_arm_servicebus_subscription_rule` #1124

New Resource: `resource_arm_servicebus_subscription_rule` #1124

jpovey commented Apr 15, 2018

tombuildsstuff left a comment

tombuildsstuff Apr 16, 2018

tombuildsstuff Apr 16, 2018

tombuildsstuff Apr 16, 2018

tombuildsstuff Apr 16, 2018

tombuildsstuff Apr 16, 2018

jpovey Apr 17, 2018

tombuildsstuff Apr 19, 2018

jpovey Apr 20, 2018

tombuildsstuff Apr 16, 2018

tombuildsstuff Apr 16, 2018

jpovey Apr 17, 2018 •

edited

Loading

tombuildsstuff Apr 17, 2018

jpovey Apr 17, 2018

tombuildsstuff Apr 16, 2018

tombuildsstuff Apr 16, 2018

tombuildsstuff Apr 16, 2018

tombuildsstuff Apr 16, 2018

jpovey commented Apr 17, 2018

jpovey commented Apr 19, 2018

jpovey commented Apr 20, 2018

tombuildsstuff commented Apr 20, 2018

tombuildsstuff left a comment

jpovey commented Apr 20, 2018

vantheshark commented Feb 20, 2019

katbyte commented Feb 21, 2019

vantheshark commented Feb 21, 2019

ghost commented Mar 5, 2019


		* `sql_filter` - (Optional) Represents a filter written in SQL language-based syntax that to be evaluated against a BrokeredMessage. Must be set when `filter_type` is set to `SqlFilter`.

		* `correlation_filter` - (Optional) A `correlation_filter` block as documented below to be evaluated against a BrokeredMessage. Must be set when `filter_type` is set to `CorrelationFilter`.


		* `filter_type` - (Required) Type of filter to be applied to a BrokeredMessage. Possible values are `SqlFilter` and `CorrelationFilter`.

		* `sql_filter` - (Optional) Represents a filter written in SQL language-based syntax that to be evaluated against a BrokeredMessage. Must be set when `filter_type` is set to `SqlFilter`.

New Resource: resource_arm_servicebus_subscription_rule #1124

New Resource: resource_arm_servicebus_subscription_rule #1124

Conversation

jpovey commented Apr 15, 2018

tombuildsstuff 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

Choose a reason for hiding this comment

jpovey Apr 17, 2018 • edited Loading

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

jpovey commented Apr 17, 2018

jpovey commented Apr 19, 2018

jpovey commented Apr 20, 2018

tombuildsstuff commented Apr 20, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

jpovey commented Apr 20, 2018

vantheshark commented Feb 20, 2019

katbyte commented Feb 21, 2019

vantheshark commented Feb 21, 2019

ghost commented Mar 5, 2019

New Resource: `resource_arm_servicebus_subscription_rule` #1124

New Resource: `resource_arm_servicebus_subscription_rule` #1124

jpovey Apr 17, 2018 •

edited

Loading