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

new resource: azurerm_servicebus_namespace_authorization_rule #1498

Merged
merged 5 commits into from
Jul 9, 2018

Conversation

katbyte
Copy link
Collaborator

@katbyte katbyte commented Jul 5, 2018

  • New resource azurerm_servicebus_namespace_authorization_rule
  • fixed service bus example
  • added namespace name validation everywhere

Copy link
Contributor

@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.

otherwise LGTM 👍

d.Set("primary_key", keysResp.PrimaryKey)
d.Set("primary_connection_string", keysResp.PrimaryConnectionString)
d.Set("secondary_key", keysResp.SecondaryKey)
d.Set("secondary_connection_string", keysResp.SecondaryConnectionString)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we access rights -> secondary_connection_string via the [XX]Properties object & nil-check them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are no properties object returned by list keys.

}

func flattenServiceBusAuthorizationRuleRights(rights *[]servicebus.AccessRights) *schema.Set {
slice := make([]interface{}, 0, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor we don't actually need this last index

"github.com/hashicorp/terraform/terraform"

"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils"
"strconv"
Copy link
Contributor

Choose a reason for hiding this comment

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

minor can we fix the imports here?


for _, tc := range cases {
t.Run(tc.Name, func(t *testing.T) {
testAccAzureRMServiceBusNamespaceAuthorizationRule(t, tc.Rights)
Copy link
Contributor

Choose a reason for hiding this comment

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

whilst this makes it more readable - this means it's not possible to run these tests individually; can we split them out as regular tests?

string(servicebus.Manage),
}, true),
},
Set: set.HashStringIgnoreCase,
Copy link
Contributor

Choose a reason for hiding this comment

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

can we switch this over to using Bools rather than a Set, since this makes the diff clearer (and then these can be inferred)?

return fmt.Errorf("Not found: %s", name)
}

name := rs.Primary.Attributes["name"]
Copy link
Contributor

Choose a reason for hiding this comment

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

minor recently we've been renaming this field to avoid conflicting with the name parameter - it may be worth doing that here for consistency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rename the variable or the property?

Copy link
Contributor

Choose a reason for hiding this comment

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

the variable, but it's not a big deal/blocker


# azurerm_servicebus_namespace_authorization_rule

Manages a ServiceBus Namespace authorization Rule within a ServiceBus Topic.
Copy link
Contributor

Choose a reason for hiding this comment

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

would this make more sense as Manages a ServiceBus Authorization Rule scoped at the Namespace level?

page_title: "Azure Resource Manager: azurerm_servicebus_namespace_authorization_rule"
sidebar_current: "docs-azurerm-resource-servicebus-namespace-authorization-rule"
description: |-
Manages a ServiceBus Namespace authorization Rule within a ServiceBus Topic.
Copy link
Contributor

Choose a reason for hiding this comment

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

(same here)

@katbyte katbyte force-pushed the f-servicebus-namespace-sas branch 3 times, most recently from 41c9297 to 45110ab Compare July 7, 2018 03:35
@katbyte
Copy link
Collaborator Author

katbyte commented Jul 7, 2018

Requested updates made, mind giving it another quick review @tombuildsstuff ?

@katbyte katbyte force-pushed the f-servicebus-namespace-sas branch from 45110ab to d88b999 Compare July 7, 2018 03:50
@katbyte katbyte force-pushed the f-servicebus-namespace-sas branch from d88b999 to f387eac Compare July 7, 2018 03:55
Copy link
Contributor

@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.

A couple of minor nits but this otherwise LGTM 👍

"send": {
Type: schema.TypeBool,
Optional: true,
Computed: true, //because we set this to true if managed is chosen
Copy link
Contributor

Choose a reason for hiding this comment

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

send and listen want default values of false since otherwise it's not possible to disable send/listen by simply removing the code / explicitly setting these values to false, which is misleading (as is an option in other providers, but we're having to work around due to the SDK)


resource "azurerm_servicebus_namespace" "example" {
name = "${var.servicebus_name}"
location = "${var.location}"
Copy link
Contributor

Choose a reason for hiding this comment

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

neither of these variables exists in this context - can we hard-code example values here? e.g. myservicebusnamespace / ${azurerm_resource_group.example.location} respectively

@katbyte katbyte merged commit 21f9604 into master Jul 9, 2018
@katbyte katbyte deleted the f-servicebus-namespace-sas branch July 9, 2018 23:27
katbyte added a commit that referenced this pull request Jul 9, 2018
@ghost
Copy link

ghost commented Mar 30, 2020

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!

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
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.

2 participants