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

New Resource: azurerm_storage_share_directory #3802

Merged
merged 9 commits into from
Jul 9, 2019
Merged

Conversation

tombuildsstuff
Copy link
Contributor

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.

@tombuildsstuff
Copy link
Contributor Author

Tests pass:

Screenshot 2019-07-07 at 22 30 02

Copy link
Collaborator

@katbyte katbyte left a 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,
},
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

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 some questions/concerns

azurerm/internal/authorizers/consts.go Show resolved Hide resolved
azurerm/internal/authorizers/helpers.go Show resolved Hide resolved
}

if accounts.Value == nil {
return nil, nil
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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?

Copy link
Contributor Author

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

azurerm/resource_arm_storage_share_directory.go Outdated Show resolved Hide resolved
@tombuildsstuff
Copy link
Contributor Author

Tests still pass:

Screenshot 2019-07-09 at 16 00 52

@tombuildsstuff tombuildsstuff merged commit 3c14444 into master Jul 9, 2019
@tombuildsstuff tombuildsstuff deleted the f/storage-sdk branch July 9, 2019 14:05
tombuildsstuff added a commit that referenced this pull request Jul 9, 2019
@ghost
Copy link

ghost commented Jul 30, 2019

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 ...

@ghost
Copy link

ghost commented Aug 8, 2019

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 Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants