-
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
New Resource: azurerm_storage_share_directory
#3802
Conversation
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.
Aside from a couple comments this LGTM 👍
Type: schema.TypeString, | ||
Required: true, | ||
ForceNew: true, | ||
}, |
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.
Should we validate this with at least NoEmptyStrings
?
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.
👍
File share names can contain only lowercase letters, numbers, and hyphens, and must begin and end with a letter or a number. The name cannot contain two consecutive hyphens.
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 some questions/concerns
} | ||
|
||
if accounts.Value == nil { | ||
return nil, nil |
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.
Is there a reason we aren't returning an error here?
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 was torn on this - if we get an invalid api response for this feels like the right thing to do, but this would block terraform refresh
from detecting that the resource doesn't exist should the API return an invalid response; WDYT?
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.
My concern is that the error message from the resource call using an empty accounts.Value
could be confusing. Do you know what error message is returned when this happens? Does the api error and say that the resource group was empty/not found?
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.
we assume it's gone - so we'll d.SetID("")
and mark the resource as 'gone' - since if it's not there there's not a lot we can do
Co-Authored-By: Matthew Frahry <[email protected]>
…orm-provider-azurerm into f/storage-sdk
This has been released in version 1.32.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 = "~> 1.32.0"
}
# ... other configuration ... |
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! |
This PR adds support for creating a Directory within an Azure File Share
This functionality comes from the new Storage SDK that we've written (since we're unable to use the new Azure Storage SDK).
Since this new SDK is using Azure/go-autorest and thus dependent on Authorizers - for the moment I've duplicated the new Storage Authorizers from this PR into this project; once the PR adding these Authorizers has been merged we can switch over to using these.
Once this PR is merged we can start switching over the existing resources and then remove the old, deprecated Storage SDK - which also allows us to address some of the longer-running Storage feature requests.