Introduce `content_type` to `azurerm_storage_blob` #1304

JunyiYi · 2018-05-26T01:49:55Z

user can specify the blob MIME type (Azure accepts all kinds of string not only MIME, so I didn't add validation rule here)
the MIME type will be returned in HTTP header when user access the output url of the blob
the MIME type is updatable and unlike other properties in SDK, it is a plain string rather than a pointer

This resolves issue #999 .

metacpp · 2018-05-29T02:01:43Z

azurerm/resource_arm_storage_blob.go

@@ -149,6 +156,7 @@ func resourceArmStorageBlobCreate(d *schema.ResourceData, meta interface{}) erro
 	blobType := d.Get("type").(string)
 	cont := d.Get("storage_container_name").(string)
 	sourceUri := d.Get("source_uri").(string)
+	contentType := d.Get("content_type").(string)

 	log.Printf("[INFO] Creating blob %q in storage account %q", name, storageAccountName)
 	if sourceUri != "" {


Refer to this article to simplify the happy-path code block:
https://medium.com/@matryer/line-of-sight-in-code-186dd7cdea88

we can pull this out into separate methods - but I think that's outside of the scope of this PR tbh; so let's do that in a separate PR

Agree with @tombuildsstuff . The blob storage code is messy and the fields also make people confused. So let's do it in another PR if possible.

also, at some point in the future (read: when the SDK's are more stable) we'll be switching over to the new Storage SDKs; which would probably be a sensible time to do this refactoring

WodansSon · 2018-05-30T02:19:56Z

azurerm/resource_arm_storage_blob.go

@@ -53,6 +54,12 @@ func resourceArmStorageBlob() *schema.Resource {
 				Default:      0,
 				ValidateFunc: validateArmStorageBlobSize,
 			},
+			"content_type": {
+				Type:          schema.TypeString,


Should do a state migration test here? Just to make sure it doesn't break anything downstream?

I run the new terraform plan after the old terraform apply, and it turns out nothing has been changed. I think there is not needs to do state migration here. @katbyte @tombuildsstuff @paultyng what do you think?

related question below: where does this default value come from?

can we also document this new field in the docs (website/docs/r/storage_blob.html.markdown)

Sure. The default value comes from Content-Type section in https://docs.microsoft.com/en-us/rest/api/storageservices/put-blob#request-headers-all-blob-types. And blobs created by old plugin have the application/octet-stream content type as well.

WodansSon

Added a question, but other than that LGTM.

tombuildsstuff

A few minor comments (sorry, forgot to hit submit yesterday) - but this is otherwise off to a good start 👍

tombuildsstuff · 2018-05-29T13:02:35Z

azurerm/resource_arm_storage_blob.go

+		blob.Properties.ContentType = d.Get("content_type").(string)
+	}
+
+	err = blob.SetProperties(nil)


can we pass in the SetBlobPropertiesOptions object here? we should pass in a sensible timeout here too

The default timeout 30s should be just fine: https://docs.microsoft.com/en-us/rest/api/storageservices/setting-timeouts-for-blob-service-operations.

tombuildsstuff · 2018-05-29T13:02:43Z

azurerm/resource_arm_storage_blob.go

+		return fmt.Errorf("Error getting storage account %s: %+v", storageAccountName, err)
+	}
+	if !accountExists {
+		return fmt.Errorf("Storage account %s not found", storageAccountName)


can we append the resource group name in here?

tombuildsstuff · 2018-05-29T13:04:01Z

azurerm/resource_arm_storage_blob.go

@@ -556,6 +600,13 @@ func resourceArmStorageBlobRead(d *schema.ResourceData, meta interface{}) error

 	container := blobClient.GetContainerReference(storageContainerName)
 	blob := container.GetBlobReference(name)
+
+	err = blob.GetProperties(nil)


can we pass in the GetBlobPropertiesOptions object here? we should pass in a sensible timeout here too

The default timeout 30s should be just fine: https://docs.microsoft.com/en-us/rest/api/storageservices/setting-timeouts-for-blob-service-operations.

tombuildsstuff · 2018-05-31T10:05:16Z

azurerm/resource_arm_storage_blob.go

+			"content_type": {
+				Type:          schema.TypeString,
+				Optional:      true,
+				Default:       "application/octet-stream",


where does this default come from?

Sure. The default value comes from Content-Type section in https://docs.microsoft.com/en-us/rest/api/storageservices/put-blob#request-headers-all-blob-types. And blobs created by old plugin have the application/octet-stream content type as well.

tombuildsstuff · 2018-05-31T10:05:36Z

azurerm/resource_arm_storage_blob.go

@@ -53,6 +54,12 @@ func resourceArmStorageBlob() *schema.Resource {
 				Default:      0,
 				ValidateFunc: validateArmStorageBlobSize,
 			},
+			"content_type": {
+				Type:          schema.TypeString,


related question below: where does this default value come from?

tombuildsstuff · 2018-05-31T10:16:38Z

azurerm/resource_arm_storage_blob.go

@@ -149,6 +156,7 @@ func resourceArmStorageBlobCreate(d *schema.ResourceData, meta interface{}) erro
 	blobType := d.Get("type").(string)
 	cont := d.Get("storage_container_name").(string)
 	sourceUri := d.Get("source_uri").(string)
+	contentType := d.Get("content_type").(string)

 	log.Printf("[INFO] Creating blob %q in storage account %q", name, storageAccountName)
 	if sourceUri != "" {


also, at some point in the future (read: when the SDK's are more stable) we'll be switching over to the new Storage SDKs; which would probably be a sensible time to do this refactoring

tombuildsstuff · 2018-05-31T10:22:15Z

azurerm/resource_arm_storage_blob_test.go

@@ -666,3 +711,45 @@ resource "azurerm_storage_blob" "destination" {
 }
 `, rInt, location, rString, sourceBlobName)
 }
+
+func testAccAzureRMStorageBlobPage_blockContentType(rInt int, rString, sourceBlobName, location, contentType string) string {


minor we've got a vague convention here of making these (rInt, rString, location, [others])

tombuildsstuff · 2018-05-31T10:24:04Z

azurerm/resource_arm_storage_blob.go

@@ -53,6 +54,12 @@ func resourceArmStorageBlob() *schema.Resource {
 				Default:      0,
 				ValidateFunc: validateArmStorageBlobSize,
 			},
+			"content_type": {
+				Type:          schema.TypeString,


can we also document this new field in the docs (website/docs/r/storage_blob.html.markdown)

tombuildsstuff · 2018-05-31T10:25:28Z

azurerm/resource_arm_storage_blob.go

+	if err != nil {
+		return fmt.Errorf("Error getting properties of blob %s (container %s, storage account %s): %+v", name, storageContainerName, storageAccountName, err)
+	}
+	d.Set("content_type", blob.Properties.ContentType)


can we nil-check if Properties != nil here?

Properties is not a pointer type: https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/vendor/github.com/Azure/azure-sdk-for-go/storage/blob.go#L34. Actually both Properties and ContentType are not pointer types.

This reverts commit 86f17b7.

Dismiss the review since all comments have been resolved.

katbyte

@JunyiYi,

Thank you for the changes, LGTM 👍

katbyte · 2018-06-12T06:17:23Z

Tests pass:

ghost · 2020-03-30T15:36:47Z

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!

Junyi Yi added 3 commits May 24, 2018 18:21

Add 'content_type' to blob.

0cb75e6

Add test case for the new content type in blob.

57cfc2e

Remove the unneeded check.

da7b3a5

JunyiYi requested review from tombuildsstuff, katbyte, metacpp and WodansSon May 26, 2018 01:50

JunyiYi self-assigned this May 26, 2018

metacpp reviewed May 29, 2018

View reviewed changes

WodansSon reviewed May 30, 2018

View reviewed changes

WodansSon approved these changes May 30, 2018

View reviewed changes

tombuildsstuff previously requested changes May 31, 2018

View reviewed changes

tombuildsstuff added enhancement waiting-response service/storage labels May 31, 2018

tombuildsstuff added this to the Soon milestone May 31, 2018

Junyi Yi added 3 commits June 1, 2018 15:25

Fix coding styles according to the reviews.

7e6daf7

Move location parameter in test case.

86f17b7

Revert "Move location parameter in test case."

96d8258

This reverts commit 86f17b7.

JunyiYi removed the waiting-response label Jun 4, 2018

katbyte approved these changes Jun 12, 2018

View reviewed changes

katbyte merged commit 30077d9 into master Jun 12, 2018

katbyte deleted the blob_content_type branch June 12, 2018 06:18

katbyte modified the milestones: Soon, 1.7.0 Jun 12, 2018

katbyte mentioned this pull request Jun 12, 2018

Feature Request: Allow setting Content-Type on azurerm_storage_blob objects #999

Closed

katbyte added a commit that referenced this pull request Jun 12, 2018

Update CHANGELOG.md to include #1304

0c9bff6

JunyiYi mentioned this pull request Jun 13, 2018

Feature Request: Edit metadata to upload blob #1185

Closed

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `content_type` to `azurerm_storage_blob` #1304

Introduce `content_type` to `azurerm_storage_blob` #1304

JunyiYi commented May 26, 2018

metacpp May 29, 2018

tombuildsstuff May 29, 2018

JunyiYi May 29, 2018 •

edited

Loading

tombuildsstuff May 31, 2018

WodansSon May 30, 2018

JunyiYi May 31, 2018 •

edited

Loading

tombuildsstuff May 31, 2018

tombuildsstuff May 31, 2018

JunyiYi Jun 1, 2018 •

edited

Loading

WodansSon left a comment

tombuildsstuff left a comment

tombuildsstuff May 29, 2018

JunyiYi Jun 1, 2018

tombuildsstuff May 29, 2018

tombuildsstuff May 29, 2018

JunyiYi Jun 1, 2018

tombuildsstuff May 31, 2018

JunyiYi Jun 1, 2018 •

edited

Loading

tombuildsstuff May 31, 2018

tombuildsstuff May 31, 2018

tombuildsstuff May 31, 2018

tombuildsstuff May 31, 2018

tombuildsstuff May 31, 2018

JunyiYi Jun 1, 2018

katbyte left a comment

katbyte commented Jun 12, 2018

ghost commented Mar 30, 2020

Introduce content_type to azurerm_storage_blob #1304

Introduce content_type to azurerm_storage_blob #1304

Conversation

JunyiYi commented May 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunyiYi May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunyiYi May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunyiYi Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

WodansSon left a comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunyiYi Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte left a comment

Choose a reason for hiding this comment

katbyte commented Jun 12, 2018

ghost commented Mar 30, 2020

Introduce `content_type` to `azurerm_storage_blob` #1304

Introduce `content_type` to `azurerm_storage_blob` #1304

JunyiYi May 29, 2018 •

edited

Loading

JunyiYi May 31, 2018 •

edited

Loading

JunyiYi Jun 1, 2018 •

edited

Loading

JunyiYi Jun 1, 2018 •

edited

Loading