-
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
New resource - azurerm_storage_account_blob_settings #3807
New resource - azurerm_storage_account_blob_settings #3807
Conversation
…rage_account_blob_settings_v1.31.0
…rage_account_blob_settings_v1.31.0
…rage_account_blob_settings_v1.31.0 # Conflicts: # azurerm/provider.go
Hi @tombuildsstuff & @katbyte, any news on this pr? |
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 @benjamin37
Thanks for this PR - apologies for the delayed review here!
Taking a look through here since these settings can only be configured at the Storage Account level, I'm wondering if this wants to be merged into the azurerm_storage_account
resource rather than being a separate resource, I've left an example of how this could look inline - WDYT?
Thanks!
…rage_account_blob_settings_v1.31.0
Hi @tombuildsstuff, I've moved the soft delete resource to the storage account as you suggested. It feels to better fit there. Can you have an other look at the PR? Thanks and kind regards |
84e1fdd
to
0723080
Compare
…rage_account_blob_settings_v1.31.0
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 @benjamin37
Thanks for pushing those updates - apologies for the delayed re-review here!
I've taken a look through and left some comments inline but this is looking good to me, if we can fix those up (and the tests pass) then this should be good to merge 👍
Thanks!
Hi @tombuildsstuff, thanks for the re-review. I've added your proposals and got one questions about the Kind regards |
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.
LGTM - thanks for pushing those changes @benjamin37
👋 running the tests it appears this feature's only available for Blob Storage accounts - could we conditionally lookup this field, depending on the account type being used:
|
Ups, didn't know that. Blob settings won't be accepted for File Storage Accounts. The Test should pass now. |
Hi @katbyte, thanks for merging! It's not a new resource anymore, we decided to place this in the storage account itself. Maybe you want to rephrase the Kind regards |
This has been released in version 1.40.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 = "~> 1.40.0"
}
# ... other configuration ... |
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! |
I was not sure if it's the right choice to create a new resource for these storage account settings instead of putting it to the storage account resources. I decided to create a separate resource since it's a separate api call and there are many settings that can be done. I'm happy for any feedback regarding this point.
I only took care of the soft delete settings of the storage account. Other settings like Cors rules can also be set with this client call. so this resource can easily be extended.
Rest API Reference: https://docs.microsoft.com/en-us/rest/api/storagerp/blobservices
(fixes #1070)