-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
azurerm_function_app
- storage_connection_string
can now rotate keys but still a require recreate when changing the account
#5645
Conversation
fcbcbb6
to
9bb3ce6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @kraihn
Thanks for this PR :)
I've taken a look through and left a couple of comments inline about the design of this resource - ultimately I'm wondering if it'd be worth splitting the storage_connection_string
field into two to be able to apply different rules to each of the segments - WDYT?
Thanks!
@@ -78,7 +78,6 @@ func resourceArmFunctionApp() *schema.Resource { | |||
"storage_connection_string": { | |||
Type: schema.TypeString, | |||
Required: true, | |||
ForceNew: true, | |||
Sensitive: true, | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whilst this'd work it'd cause a bunch of runtime errors should someone opt to change the account being used - as such I'm wondering if we'd be better to split this field into two:
- "storage_account_id" for the ID of the Storage Account in question - which is ForceNew
- the "storage account access key" which is either the primary/secondary key and allows updating
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love that idea! It makes more sense to me. Should I make this change with 2.0 in mind and remove the old property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that we are post 2.0 what we will have to do is deprecate the old one and make them conflict.
@@ -869,3 +878,14 @@ func flattenFunctionAppSiteCredential(input *web.UserProperties) []interface{} { | |||
|
|||
return append(results, result) | |||
} | |||
|
|||
func getAccountName(storageConnectionString string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in Golang functions are shared across the entire package - as such this method would probably be better named as:
func getAccountName(storageConnectionString string) string { | |
func getAccountNameFromStorageConnectionString(storageConnectionString string) string { |
@@ -78,7 +78,6 @@ func resourceArmFunctionApp() *schema.Resource { | |||
"storage_connection_string": { | |||
Type: schema.TypeString, | |||
Required: true, | |||
ForceNew: true, | |||
Sensitive: true, | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that we are post 2.0 what we will have to do is deprecate the old one and make them conflict.
@kraihnm, going to close this in favour of #6304. thanks for opening the PR! |
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! |
Fixes #5435