-
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
azurerm_data_lake_store - support for encryption/firewall properties #1623
Conversation
Tests pass: ``` $ acctests azurerm TestAccAzureRMDataLakeStore_ === RUN TestAccAzureRMDataLakeStore_basic --- PASS: TestAccAzureRMDataLakeStore_basic (147.99s) === RUN TestAccAzureRMDataLakeStore_tier --- PASS: TestAccAzureRMDataLakeStore_tier (140.91s) === RUN TestAccAzureRMDataLakeStore_encryptionDisabled --- PASS: TestAccAzureRMDataLakeStore_encryptionDisabled (126.77s) === RUN TestAccAzureRMDataLakeStore_firewallUpdate --- PASS: TestAccAzureRMDataLakeStore_firewallUpdate (253.79s) === RUN TestAccAzureRMDataLakeStore_withTags --- PASS: TestAccAzureRMDataLakeStore_withTags (161.95s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 831.835s ```
Tests pass: ``` $ acctests azurerm TestAccDataSourceAzureRMDataLakeStore_ === RUN TestAccDataSourceAzureRMDataLakeStore_basic --- PASS: TestAccDataSourceAzureRMDataLakeStore_basic (150.71s) === RUN TestAccDataSourceAzureRMDataLakeStore_tier --- PASS: TestAccDataSourceAzureRMDataLakeStore_tier (143.18s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 294.220s ```
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 note about an optional attribute
@@ -37,6 +44,16 @@ The following arguments are supported: | |||
|
|||
* `tier` - (Optional) The monthly commitment tier for Data Lake Store. Accepted values are `Consumption`, `Commitment_1TB`, `Commitment_10TB`, `Commitment_100TB`, `Commitment_500TB`, `Commitment_1PB` or `Commitment_5PB`. | |||
|
|||
* `encryption_state` - (Optional) Is Encryption enabled on this Data Lake Store Account? Possible values are `Enabled` or `Disabled`. Defaults to `Enabled`. | |||
|
|||
* `encryption_type` - (Optional) The Encryption Type used for this Data Lake Store Account. Defaults to `SystemManaged` which is the only supported value at this time. |
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.
Does the API default this? It's not defaulted in the schema. https://github.com/terraform-providers/terraform-provider-azurerm/pull/1623/files#diff-5741558d3f6a1e548c0a362adc779447R67
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.
It depends on if Encryption's enabled or disabled, will push an update
@@ -65,15 +110,26 @@ func resourceArmDateLakeStoreCreate(d *schema.ResourceData, meta interface{}) er | |||
location := azureRMNormalizeLocation(d.Get("location").(string)) | |||
resourceGroup := d.Get("resource_group_name").(string) | |||
tier := d.Get("tier").(string) | |||
|
|||
encryptionState := account.EncryptionState(d.Get("encryption_state").(string)) | |||
encryptionType := account.EncryptionConfigType(d.Get("encryption_type").(string)) |
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 do a d.GetOk with this value being Optional?
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.
Nope, since otherwise there's no way to clear it (required when encryption is disabled)
Data Source Tests pass:
Resource Tests pass:
|
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 adds the ability to disable encryption and use user assigned encryption keys from a key vault.
Still a work in progress as the
UserAssigned
test fails:Fixes #1603