-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Conversation
State migration tests pass:
Acceptance tests passed when I ran them earlier - but I'm re-running them now to confirm |
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 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 |
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.
Wrap the markdown at 80 characters?
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.
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
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.
I know that feeling 😉 👍
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 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) |
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.
Just want to confirm that we are guaranteed to have 2 keys
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.
Yep, afaik
@tombuildsstuff I have been working on contributing additional test cases and used the recent I noticed this pr breaks the test cases which involve a storage account, e.g.
I assume they need to be adapted to the changed introduced in this pr. |
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! |
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.