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

additions to azurerm_storage_account #363

Merged
merged 1 commit into from
Sep 29, 2017
Merged

Conversation

tombuildsstuff
Copy link
Contributor

Unfortunately testing Custom Domains is harder than anticipated - since it needs a static DNS name and storage name, which need to be globally unique. For this reason I've manually confirmed this works, but haven't included an acceptance test for it at the moment.

- Support for File Encryption. Fixes #80
- Adding Import tests
- Support for Custom Domains on Storage Accounts. Fixes #15
- Splitting the storage account Tier and Replication out into separate fields. Incorporating #117 & Fixing #110
@tombuildsstuff
Copy link
Contributor Author

State migration tests pass:

 $ acctests azurerm TestAzureRMStorageAccountMigrateState
=== RUN   TestAzureRMStorageAccountMigrateState
2017/09/28 19:12:45 [INFO] Found AzureRM Storage Account State v0; migrating to v1
2017/09/28 19:12:45 [DEBUG] ARM Storage Account Attributes before Migration: map[string]string{"account_type":"Standard_LRS"}
2017/09/28 19:12:45 [DEBUG] ARM Storage Account Attributes after State Migration: map[string]string{"account_tier":"Standard", "account_replication_type":"LRS", "account_type":"Standard_LRS"}
2017/09/28 19:12:45 [INFO] Found AzureRM Storage Account State v0; migrating to v1
2017/09/28 19:12:45 [DEBUG] ARM Storage Account Attributes before Migration: map[string]string{"account_type":"Premium_GRS"}
2017/09/28 19:12:45 [DEBUG] ARM Storage Account Attributes after State Migration: map[string]string{"account_type":"Premium_GRS", "account_tier":"Premium", "account_replication_type":"GRS"}
--- PASS: TestAzureRMStorageAccountMigrateState (0.00s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	0.014s

Acceptance tests passed when I ran them earlier - but I'm re-running them now to confirm

@tombuildsstuff
Copy link
Contributor Author

screen shot 2017-09-28 at 19 21 42

Copy link
Contributor

@vancluever vancluever left a comment

Choose a reason for hiding this comment

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

LGTM with a minor nitpick about the markdown wrapping 👍

`Standard_RAGRS`, `Premium_LRS`. Changing this is sometimes valid - see the Azure
documentation for more information on which types of accounts can be converted
into other types.
* `account_tier` - (Required) Defines the Tier to use for this storage account. Valid options are `Standard` and `Premium`. Changing this forces a new resource to be created
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrap the markdown at 80 characters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taking a look into this - we've got a ton of cases which have gone beyond this limit - so I'm going to leave this as-is for now and we'll do a provider wide cleanup in a bit

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that feeling 😉 👍

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

LGTM with a minor comment

d.Set("primary_file_endpoint", endpoints.File)

pscs := fmt.Sprintf("DefaultEndpointsProtocol=https;BlobEndpoint=%s;AccountName=%s;AccountKey=%s",
*endpoints.Blob, *resp.Name, *accessKeys[0].Value)
Copy link
Member

Choose a reason for hiding this comment

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

Just want to confirm that we are guaranteed to have 2 keys

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, afaik

@tombuildsstuff tombuildsstuff merged commit ed0e114 into master Sep 29, 2017
@tombuildsstuff tombuildsstuff deleted the storageaccount branch September 29, 2017 19:43
tombuildsstuff added a commit that referenced this pull request Sep 29, 2017
@dominik-lekse
Copy link
Contributor

@tombuildsstuff I have been working on contributing additional test cases and used the recent master with this feature merged.

I noticed this pr breaks the test cases which involve a storage account, e.g. TestAccAzureRMVirtualMachine_basicLinuxMachine. Those tests fail with the following message.

make testacc TEST=./azurerm TESTARGS='-run=^TestAccAzureRMVirtualMachine_basicLinuxMachine$$'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -run=^TestAccAzureRMVirtualMachine_basicLinuxMachine$ -timeout 120m
=== RUN   TestAccAzureRMVirtualMachine_basicLinuxMachine
--- FAIL: TestAccAzureRMVirtualMachine_basicLinuxMachine (0.01s)
	testing.go:434: Step 0 error: Configuration is invalid.

		Warnings: []string{"azurerm_storage_account.test: \"account_type\": [DEPRECATED] This field has been split into `account_tier` and `account_replication_type`"}

		Errors: []string{"azurerm_storage_account.test: \"account_replication_type\": required field is not set", "azurerm_storage_account.test: \"account_tier\": required field is not set"}
FAIL
exit status 1
FAIL	github.com/terraform-providers/terraform-provider-azurerm/azurerm	0.034s
make: *** [testacc] Error 1

I assume they need to be adapted to the changed introduced in this pr.

@ghost
Copy link

ghost commented Apr 1, 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 Apr 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants