-
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
Updates azurerm_storage_blob on content_md5 changes #7786
Updates azurerm_storage_blob on content_md5 changes #7786
Conversation
@katbyte - Can you please review this PR? |
Hi @Humoiz - Apologies, I somehow missed the notification on this one. Could you add a passing test to cover this new functionality and show it working? (there's a few examples elsewhere in the provider for including a small file as Thanks |
Hi @Humoiz |
Thanks! I will address the testing this week. |
Added the tests. |
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.
Thanks for the pr @Humoiz - overall this looks good but these tests are failing due to public accessnot being allowed on the storage account:
TestAccAzureRMStorageBlob_blockFromInlineContent
--
TestAccAzureRMStorageBlob_blockFromPublicBlob
TestAccAzureRMStorageBlob_requiresImport
TestAccAzureRMStorageBlob_updateBlockFromInlineContent
should be a simple config change to fix this 🙂
@katbyte - Thank you for pointing this out. I have updated TestAccAzureRMStorageBlob_updateBlockFromInlineContent test to have the container |
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.
@Humoiz - i think its the flag that needs to be changed:
=== CONT TestAccAzureRMStorageBlob_blockEmptyAzureADAuth
TestAccAzureRMStorageBlob_updateBlockFromInlineContent: testing.go:684: Step 0 error: errors during apply:
Error: failed creating container: containers.Client#Create: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="PublicAccessNotPermitted" Message="Public access is not permitted on this storage account.\nRequestId:2ee4abf8-601e-0005-1f61-903875000000\nTime:2020-09-21T21:53:23.8541083Z"
on /opt/teamcity-agent/temp/buildTmp/tf-test589256406/main.tf line 16:
(source code not available)
TestAccAzureRMStorageBlob_blockFromInlineContent: testing.go:684: Step 0 error: errors during apply:
Error: failed creating container: containers.Client#Create: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="PublicAccessNotPermitted" Message="Public access is not permitted on this storage account.\nRequestId:3ad9340a-001e-0075-2f61-90d775000000\nTime:2020-09-21T21:53:23.9088950Z"
on /opt/teamcity-agent/temp/buildTmp/tf-test588258110/main.tf line 16:
(source code not available)
TestAccAzureRMStorageBlob_blockFromPublicBlob: testing.go:684: Step 0 error: errors during apply:
Error: failed creating container: containers.Client#Create: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="PublicAccessNotPermitted" Message="Public access is not permitted on this storage account.\nRequestId:e3cfbba0-101e-0054-0661-90cddc000000\nTime:2020-09-21T21:53:23.8093032Z"
on /opt/teamcity-agent/temp/buildTmp/tf-test884844187/main.tf line 16:
(source code not available)
TestAccAzureRMStorageBlob_blockFromLocalFileWithContentMd5: testing.go:684: Step 0 error: errors during apply:
Error: failed creating container: containers.Client#Create: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="PublicAccessNotPermitted" Message="Public access is not permitted on this storage account.\nRequestId:70ee7031-e01e-00c9-6f61-90f196000000\nTime:2020-09-21T21:53:48.5732074Z"
on /opt/teamcity-agent/temp/buildTmp/tf-test894169348/main.tf line 16:
(source code not available)
TestAccAzureRMStorageBlob_requiresImport: testing.go:684: Step 0 error: errors during apply:
Error: failed creating container: containers.Client#Create: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="PublicAccessNotPermitted" Message="Public access is not permitted on this storage account.\nRequestId:68d53229-101e-0038-3561-90c398000000\nTime:2020-09-21T21:53:48.4836020Z"
on /opt/teamcity-agent/temp/buildTmp/tf-test072062594/main.tf line 16:
(source code not available)
@katbyte - The existing and new storage tests fails on my local environment. Unfortunately the failure does not give me any details on why it failed so I can not tests my new test changes. However, the same configuration works for me when ran outside of the tests. It would be helpful if you can exactly point me in the code on what needs to be changed to have these tests pass. This is the configuration I am using in the new tests and it works when I run the configuration outside of the tests.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Apologies this has taken so long to get back to. I'll be looking at the this later this week. |
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.
This LGTM @jackofallops 👍
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 @Humoiz
Apologies for the delayed re-review here!
Taking a look through this mostly LGTM - based on the Giovanni SDK conditionally setting the checksum based on whether the value is non-nil - I believe we'll need to conditionally set the ContentMD5
field rather than always setting this to an empty string, incase the behaviour of the API changes - WDYT? That said, if we can fix up the remaining minor comments then this otherwise LGTM 👍
Thanks!
* Conditionally setting the ContentMD5 field in requests, whilst overkill this avoids an expectation on the behaviour of the API ignoring an empty string, since Giovanni conditionally sends this if it's not-nil * Fixes a typo in the docs * Wraps a couple of exceptions to be clearer
@Humoiz I hope you don't mind, but so that we can get this merged into this release, I've pushed a commit to resolve the comments above |
dismissing since changes have been pushed
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 👍
Tests look good 👍 |
I'm using azure provider version |
This has been released in version 2.37.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.37.0"
}
# ... other configuration ... |
@jpveritone I believe so you need to be on version >= 2.37.0 |
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! |
Implementing feature mentioned in the feature request: #1990
Problem statement: Currently in order to update the blob, user has to change the blob name to trigger the update.
This PR adds new field content_md5 to have azurerm_storage_blob update on content_md5 changes. It forces a new deployment if the value for content_md5 is changed. This is only supported for blob type Block.
(fixes #1990)