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

support alias connection strings for eventhub resources #6708

Merged
merged 7 commits into from May 7, 2020

Conversation

neil-yechenwei
Copy link
Contributor

fixes #6593

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.

Hey @neil-yechenwei,

thanks for the PR. I was wondering if it would make more sense to. put alias on the end so the properties sort and read better? thanks!

@@ -100,7 +100,13 @@ func EventHubAuthorizationRuleSchemaFrom(s map[string]*schema.Schema) map[string
Default: false,
},

"primary_key": {
"alias_primary_connection_string": {
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 rename this to

Suggested change
"alias_primary_connection_string": {
"primary_connection_string_alias": {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Sensitive: true,
},

"alias_secondary_connection_string": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

and

Suggested change
"alias_secondary_connection_string": {
"secondary_connection_string_alias": {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@sebader
Copy link
Contributor

sebader commented May 5, 2020

thanks for the PR. I was wondering if it would make more sense to. put alias on the end so the properties sort and read better? thanks!

While I understand your argument, the way @neil-yechenwei implemented it is following the official SDK's naming: https://github.com/Azure/azure-sdk-for-go/blob/master/services/eventhub/mgmt/2017-04-01/eventhub/models.go#L226

what is the general guidance for the TF provider? Does it have its own naming strategy or does it try to follow the underlying APIs?

@ghost ghost removed the waiting-response label May 5, 2020
@neil-yechenwei
Copy link
Contributor Author

neil-yechenwei commented May 6, 2020

The goal is to make users better understand these properties in TF.

@neil-yechenwei
Copy link
Contributor Author

@katbyte , thanks for your comments. I've updated code.

@katbyte
Copy link
Collaborator

katbyte commented May 6, 2020

@sebader - we try to give properties the most clear accurate name we can, much like the portal often deviates from the SDK/APi we do as well. They are often poorly (and sometimes incorrectly!) named, and in this case putting it at the end allowed the propertied to group in documentation & schema provider a better UX all around.

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.

@neil-yechenwei, thanks for the changes. Looks like you missed some renames - ounce those have been updated this should be good to merge

* `alias_default_primary_connection_string` - The alias of the primary connection string for the authorization
rule `RootManageSharedAccessKey`, which is generated when disaster recovery is enabled.

* `alias_default_secondary_connection_string` - The alias of the secondary connection string for the
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you missed on here?

@@ -88,14 +88,20 @@ The following attributes are exported:
The following attributes are exported only if there is an authorization rule named
`RootManageSharedAccessKey` which is created automatically by Azure.

* `alias_default_primary_connection_string` - The alias of the primary connection string for the authorization
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you missed on here?

@@ -47,14 +47,20 @@ output "eventhub_namespace_id" {
The following attributes are exported only if there is an authorization rule named
`RootManageSharedAccessKey` which is created automatically by Azure.

* `default_primary_connection_string` - The primary connection string for the authorization
* `alias_default_primary_connection_string` - The alias of the primary connection string for the authorization
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you missed on here?

rule `RootManageSharedAccessKey`.

* `default_secondary_connection_string` - The secondary connection string for the
* `alias_default_secondary_connection_string` - The alias of the secondary connection string for the
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you missed on here?

@@ -158,13 +158,19 @@ func resourceArmEventHubNamespace() *schema.Resource {
},
},

"default_primary_connection_string": {
"alias_default_primary_connection_string": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you missed on here?

Type: schema.TypeString,
Computed: true,
Sensitive: true,
},

"default_secondary_connection_string": {
"alias_default_secondary_connection_string": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you missed on here?

"listen": {
Type: schema.TypeBool,
Computed: true,
"primary_connection_string_alias": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

these are not ordered correctly?

@@ -100,7 +100,13 @@ func EventHubAuthorizationRuleSchemaFrom(s map[string]*schema.Schema) map[string
Default: false,
},

"primary_key": {
"primary_connection_string_alias": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

these are not ordered correctly?

@jackofallops
Copy link
Member

Running acctests now

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.

thanks for the fixes @jackofallops! LGTM 👍

@katbyte katbyte changed the title Add support for alias connection string for eventhub support alias connection strings for eventhub resources May 7, 2020
@katbyte katbyte merged commit 776a23d into hashicorp:master May 7, 2020
katbyte added a commit that referenced this pull request May 7, 2020
@ghost
Copy link

ghost commented May 8, 2020

This has been released in version 2.9.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.9.0"
}
# ... other configuration ...

@sebader
Copy link
Contributor

sebader commented May 8, 2020

@neil-yechenwei quick question on the usage of this new feature: When I do not know whether an EventHub is geo-replicated, is it safe to do it this way now? I.e. will the alias be null if not geo-replicated?

value = data.azurerm_eventhub_authorization_rule.example.primary_connection_string_alias != null ? data.azurerm_eventhub_authorization_rule.example.primary_connection_string_alias : data.azurerm_eventhub_authorization_rule.example.primary_connection_string

@ghost
Copy link

ghost commented Jun 7, 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 👉 hashibot-feedback@hashicorp.com. Thanks!

@hashicorp hashicorp locked and limited conversation to collaborators Jun 7, 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.

Support for Alias connection string when using azurerm_eventhub_namespace_disaster_recovery_config
4 participants