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_storage_account_blob_settings #3807

Conversation

benjamin37
Copy link
Contributor

@benjamin37 benjamin37 commented Jul 8, 2019

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)

@benjamin37
Copy link
Contributor Author

benjamin37 commented Jul 11, 2019

Hi @katbyte,

I guess this pull request is not related to #2046. This pull request is about soft delete for storage accounts and #2046 is about storage account encryption settings which needs soft delete for key vaults (not storage accounts). The Azure REST APIs are different ones.

@tombuildsstuff tombuildsstuff added this to the v1.33.0 milestone Jul 19, 2019
@tombuildsstuff tombuildsstuff self-assigned this Aug 19, 2019
@tombuildsstuff tombuildsstuff modified the milestones: v1.33.0, v1.34.0 Aug 21, 2019
@tombuildsstuff tombuildsstuff modified the milestones: v1.34.0, v1.35.0 Sep 12, 2019
@benjamin37
Copy link
Contributor Author

Hi @tombuildsstuff & @katbyte,

any news on this pr?

@tombuildsstuff tombuildsstuff modified the milestones: v1.35.0, v1.36.0 Oct 2, 2019
@tombuildsstuff tombuildsstuff modified the milestones: v1.36.0, v1.37.0 Oct 15, 2019
@tombuildsstuff tombuildsstuff modified the milestones: v1.37.0, v2.0.0 Oct 31, 2019
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.

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!

azurerm/resource_arm_storage_account_blob_settings.go Outdated Show resolved Hide resolved
azurerm/resource_arm_storage_account_blob_settings.go Outdated Show resolved Hide resolved
azurerm/resource_arm_storage_account_blob_settings.go Outdated Show resolved Hide resolved
azurerm/resource_arm_storage_account_blob_settings.go Outdated Show resolved Hide resolved
@ghost ghost added size/L and removed size/XL labels Nov 8, 2019
@benjamin37
Copy link
Contributor Author

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
Benjamin

@ghost ghost removed the waiting-response label Nov 8, 2019
@benjamin37 benjamin37 force-pushed the feature/azurerm_storage_account_blob_settings_v1.31.0 branch from 84e1fdd to 0723080 Compare November 8, 2019 14:09
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.

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!

azurerm/resource_arm_storage_account.go Outdated Show resolved Hide resolved
azurerm/resource_arm_storage_account.go Outdated Show resolved Hide resolved
azurerm/resource_arm_storage_account.go Outdated Show resolved Hide resolved
azurerm/resource_arm_storage_account.go Outdated Show resolved Hide resolved
azurerm/resource_arm_storage_account.go Outdated Show resolved Hide resolved
azurerm/resource_arm_storage_account.go Outdated Show resolved Hide resolved
website/docs/r/storage_account.html.markdown Outdated Show resolved Hide resolved
website/docs/r/storage_account.html.markdown Outdated Show resolved Hide resolved
@benjamin37
Copy link
Contributor Author

Hi @tombuildsstuff,

thanks for the re-review. I've added your proposals and got one questions about the days=0 (mentioned in the review comments).

Kind regards
Benjamin

@ghost ghost removed the waiting-response label Dec 16, 2019
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.

LGTM - thanks for pushing those changes @benjamin37

@tombuildsstuff
Copy link
Contributor

👋 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:

------- Stdout: -------
=== RUN   TestAccAzureRMStorageAccount_fileStorageWithUpdate
=== PAUSE TestAccAzureRMStorageAccount_fileStorageWithUpdate
=== CONT  TestAccAzureRMStorageAccount_fileStorageWithUpdate
--- FAIL: TestAccAzureRMStorageAccount_fileStorageWithUpdate (47.46s)
    testing.go:569: Step 0 error: errors during apply:
        
        Error: Error reading blob properties for AzureRM Storage Account "unlikely23exst2acctbhsf": storage.BlobServicesClient#GetServiceProperties: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="FeatureNotSupportedForAccount" Message="Blob is not supported for the account."

...

FAIL

@benjamin37
Copy link
Contributor Author

Ups, didn't know that. Blob settings won't be accepted for File Storage Accounts. The Test should pass now.

@katbyte katbyte added this to the v1.40.0 milestone Dec 17, 2019
@katbyte katbyte changed the title [azurerm_storage_account_blob_settings] New resource storage account blob settings New resource - azurerm_storage_account_blob_settings Dec 17, 2019
@katbyte katbyte merged commit 6def60f into hashicorp:master Dec 17, 2019
katbyte added a commit that referenced this pull request Dec 17, 2019
@benjamin37 benjamin37 deleted the feature/azurerm_storage_account_blob_settings_v1.31.0 branch December 18, 2019 07:12
@benjamin37
Copy link
Contributor Author

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 CHANGELOG.md.

Kind regards
Benjamin

tombuildsstuff added a commit that referenced this pull request Dec 18, 2019
@ghost
Copy link

ghost commented Jan 8, 2020

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

@ghost
Copy link

ghost commented Mar 28, 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 28, 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.

Storage Soft Delete Support
3 participants