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.

Sign up for GitHub

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

Jump to bottom

Introduce content_type to azurerm_storage_blob #1304

Merged
merged 6 commits into from
Jun 12, 2018
Merged

Introduce content_type to azurerm_storage_blob #1304

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0cb75e6
Add 'content_type' to blob.
May 25, 2018
57cfc2e
Add test case for the new content type in blob.
May 26, 2018
da7b3a5
Remove the unneeded check.
May 26, 2018
7e6daf7
Fix coding styles according to the reviews.
Jun 1, 2018
86f17b7
Move location parameter in test case.
Jun 2, 2018
96d8258
Revert "Move location parameter in test case."
Jun 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 55 additions & 4 deletions azurerm/resource_arm_storage_blob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ func resourceArmStorageBlob() *schema.Resource {
return &schema.Resource{
Create: resourceArmStorageBlobCreate,
Read: resourceArmStorageBlobRead,
Update: resourceArmStorageBlobUpdate,
Exists: resourceArmStorageBlobExists,
Delete: resourceArmStorageBlobDelete,

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@JunyiYi JunyiYi May 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@JunyiYi JunyiYi Jun 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Optional: true,
Default: "application/octet-stream",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where does this default come from?

Copy link
Author

@JunyiYi JunyiYi Jun 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ConflictsWith: []string{"source_uri"},
},
"source": {
Type: schema.TypeString,
Optional: true,
Expand Down Expand Up @@ -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 != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@JunyiYi JunyiYi May 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand All @@ -174,7 +182,7 @@ func resourceArmStorageBlobCreate(d *schema.ResourceData, meta interface{}) erro
if source != "" {
parallelism := d.Get("parallelism").(int)
attempts := d.Get("attempts").(int)
if err := resourceArmStorageBlobBlockUploadFromSource(cont, name, source, blobClient, parallelism, attempts); err != nil {
if err := resourceArmStorageBlobBlockUploadFromSource(cont, name, source, contentType, blobClient, parallelism, attempts); err != nil {
return fmt.Errorf("Error creating storage blob on Azure: %s", err)
}
}
Expand All @@ -183,7 +191,7 @@ func resourceArmStorageBlobCreate(d *schema.ResourceData, meta interface{}) erro
if source != "" {
parallelism := d.Get("parallelism").(int)
attempts := d.Get("attempts").(int)
if err := resourceArmStorageBlobPageUploadFromSource(cont, name, source, blobClient, parallelism, attempts); err != nil {
if err := resourceArmStorageBlobPageUploadFromSource(cont, name, source, contentType, blobClient, parallelism, attempts); err != nil {
return fmt.Errorf("Error creating storage blob on Azure: %s", err)
}
} else {
Expand All @@ -193,6 +201,7 @@ func resourceArmStorageBlobCreate(d *schema.ResourceData, meta interface{}) erro
container := blobClient.GetContainerReference(cont)
blob := container.GetBlobReference(name)
blob.Properties.ContentLength = size
blob.Properties.ContentType = contentType
err := blob.PutPageBlob(options)
if err != nil {
return fmt.Errorf("Error creating storage blob on Azure: %s", err)
Expand All @@ -210,7 +219,7 @@ type resourceArmStorageBlobPage struct {
section *io.SectionReader
}

func resourceArmStorageBlobPageUploadFromSource(container, name, source string, client *storage.BlobStorageClient, parallelism, attempts int) error {
func resourceArmStorageBlobPageUploadFromSource(container, name, source, contentType string, client *storage.BlobStorageClient, parallelism, attempts int) error {
workerCount := parallelism * runtime.NumCPU()

file, err := os.Open(source)
Expand All @@ -228,6 +237,7 @@ func resourceArmStorageBlobPageUploadFromSource(container, name, source string,
containerRef := client.GetContainerReference(container)
blob := containerRef.GetBlobReference(name)
blob.Properties.ContentLength = blobSize
blob.Properties.ContentType = contentType
err = blob.PutPageBlob(options)
if err != nil {
return fmt.Errorf("Error creating storage blob on Azure: %s", err)
Expand Down Expand Up @@ -387,7 +397,7 @@ type resourceArmStorageBlobBlock struct {
id string
}

func resourceArmStorageBlobBlockUploadFromSource(container, name, source string, client *storage.BlobStorageClient, parallelism, attempts int) error {
func resourceArmStorageBlobBlockUploadFromSource(container, name, source, contentType string, client *storage.BlobStorageClient, parallelism, attempts int) error {
workerCount := parallelism * runtime.NumCPU()

file, err := os.Open(source)
Expand Down Expand Up @@ -432,6 +442,7 @@ func resourceArmStorageBlobBlockUploadFromSource(container, name, source string,

containerReference := client.GetContainerReference(container)
blobReference := containerReference.GetBlobReference(name)
blobReference.Properties.ContentType = contentType
options := &storage.PutBlockListOptions{}
err = blobReference.PutBlockList(blockList, options)
if err != nil {
Expand Down Expand Up @@ -524,6 +535,39 @@ func resourceArmStorageBlobBlockUploadWorker(ctx resourceArmStorageBlobBlockUplo
}
}

func resourceArmStorageBlobUpdate(d *schema.ResourceData, meta interface{}) error {
armClient := meta.(*ArmClient)
ctx := armClient.StopContext

resourceGroupName := d.Get("resource_group_name").(string)
storageAccountName := d.Get("storage_account_name").(string)

blobClient, accountExists, err := armClient.getBlobStorageClientForStorageAccount(ctx, resourceGroupName, storageAccountName)
if err != nil {
return fmt.Errorf("Error getting storage account %s: %+v", storageAccountName, err)
}
if !accountExists {
return fmt.Errorf("Storage account %s not found", storageAccountName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we append the resource group name in here?

}

name := d.Get("name").(string)
storageContainerName := d.Get("storage_container_name").(string)

container := blobClient.GetContainerReference(storageContainerName)
blob := container.GetBlobReference(name)

if d.HasChange("content_type") {
blob.Properties.ContentType = d.Get("content_type").(string)
}

err = blob.SetProperties(nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

return nil
}

func resourceArmStorageBlobRead(d *schema.ResourceData, meta interface{}) error {
armClient := meta.(*ArmClient)
ctx := armClient.StopContext
Expand Down Expand Up @@ -556,6 +600,13 @@ func resourceArmStorageBlobRead(d *schema.ResourceData, meta interface{}) error

container := blobClient.GetContainerReference(storageContainerName)
blob := container.GetBlobReference(name)

err = blob.GetProperties(nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


url := blob.GetURL()
if url == "" {
log.Printf("[INFO] URL for %q is empty", name)
Expand Down
87 changes: 87 additions & 0 deletions azurerm/resource_arm_storage_blob_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,51 @@ func TestAccAzureRMStorageBlob_source_uri(t *testing.T) {
})
}

func TestAccAzureRMStorageBlobBlock_blockContentType(t *testing.T) {
resourceName := "azurerm_storage_blob.source"
ri := acctest.RandInt()
rs1 := strings.ToLower(acctest.RandString(11))
sourceBlob, err := ioutil.TempFile("", "")
if err != nil {
t.Fatalf("Failed to create local source blob file")
}

_, err = io.CopyN(sourceBlob, rand.Reader, 25*1024*1024)
if err != nil {
t.Fatalf("Failed to write random test to source blob")
}

err = sourceBlob.Close()
if err != nil {
t.Fatalf("Failed to close source blob")
}

config := testAccAzureRMStorageBlobPage_blockContentType(ri, rs1, sourceBlob.Name(), testLocation(), "text/plain")
updateConfig := testAccAzureRMStorageBlobPage_blockContentType(ri, rs1, sourceBlob.Name(), testLocation(), "text/vnd.terraform.acctest.tmpfile")

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testCheckAzureRMStorageBlobDestroy,
Steps: []resource.TestStep{
{
Config: config,
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMStorageBlobExists(resourceName),
resource.TestCheckResourceAttr(resourceName, "content_type", "text/plain"),
),
},
{
Config: updateConfig,
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMStorageBlobExists(resourceName),
resource.TestCheckResourceAttr(resourceName, "content_type", "text/vnd.terraform.acctest.tmpfile"),
),
},
},
})
}

func testCheckAzureRMStorageBlobExists(name string) resource.TestCheckFunc {
return func(s *terraform.State) error {

Expand Down Expand Up @@ -666,3 +711,45 @@ resource "azurerm_storage_blob" "destination" {
}
`, rInt, location, rString, sourceBlobName)
}

func testAccAzureRMStorageBlobPage_blockContentType(rInt int, rString, sourceBlobName, location, contentType string) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return fmt.Sprintf(`
resource "azurerm_resource_group" "test" {
name = "acctestRG-%d"
location = "%s"
}

resource "azurerm_storage_account" "source" {
name = "acctestacc%s"
resource_group_name = "${azurerm_resource_group.test.name}"
location = "${azurerm_resource_group.test.location}"
account_tier = "Standard"
account_replication_type = "LRS"

tags {
environment = "staging"
}
}

resource "azurerm_storage_container" "source" {
name = "source"
resource_group_name = "${azurerm_resource_group.test.name}"
storage_account_name = "${azurerm_storage_account.source.name}"
container_access_type = "blob"
}

resource "azurerm_storage_blob" "source" {
name = "source.vhd"

resource_group_name = "${azurerm_resource_group.test.name}"
storage_account_name = "${azurerm_storage_account.source.name}"
storage_container_name = "${azurerm_storage_container.source.name}"

type = "page"
source = "%s"
content_type = "%s"
parallelism = 3
attempts = 3
}
`, rInt, location, rString, sourceBlobName, contentType)
}