User Story 60501: 101-databricks-cmk-dbfs

[TomArcherMsft]

No description provided.

[grayzu]

Thanks for the PR! Looking great!

Few comments, suggestions and questions.

[TomArcherMsft]

#please-hold-off I'm still making changes. Please don't review yet.

[TomArcherMsft]

@grayzu @lonegunmanb I made all the edits and tested the changes. Please review. Thanks!

[lonegunmanb]

Good job @TomArcherMsft, only one tiny suggestion

[TomArcherMsft]

Good job @TomArcherMsft, only one tiny suggestion

Thanks @lonegunmanb. I made the edit.

[grayzu]

LGTM

[lonegunmanb]

Hi @TomArcherMsft , a few comments

[lonegunmanb]

In line 72:

resource "azurerm_key_vault_key" "key" {
  name         = coalesce(var.key_name, random_pet.azurerm_key_vault_key_name[0].id)

So when var.key_name equals to empty string("") it would lead to random_pet.azurerm_key_vault_key_name[0], but random_pet.azurerm_key_vault_key_name would be created only when var.key_name is null.

We can use the following toggle expression:

count = var.key_name == null || var.key_name == "" ? 1 : 0

[TomArcherMsft]

@stemaMSFT @lonegunmanb I made the requested changes. Please review.

[lonegunmanb]

Apology for the confusion @TomArcherMsft, I didn't make my suggestion clear, would you please adjust your code? Thanks!

[TomArcherMsft]

@lonegunmanb Thanks for the clarification! I made the changes.

[lonegunmanb]

Hi @TomArcherMsft thanks for the update, we still got one issue here.

resource "azurerm_key_vault_key" "key" {
  name         = coalesce(var.key_name, random_pet.azurerm_key_vault_key_name[0].id)

coalesce takes any number of arguments and returns the first one that isn't null or an empty string.

So if var.key_name is set to "", coalesce would return random_pet.azurerm_key_vault_key_name[0].id. But random_pet.azurerm_key_vault_key_name's declaration is:

resource "random_pet" "azurerm_key_vault_key_name" {
  count  = var.key_name == null ? 1 : 0

var.key_name = "" would lead to 0, that would cause an error. So we should change it to:

resource "random_pet" "azurerm_key_vault_key_name" {
  count  = var.key_name == null || var.key_name == "" ? 1 : 0

[lonegunmanb]

Thanks @TomArcherMsft, almost LGTM but only one description question.

---

The e2e test passed.

[lonegunmanb]

Would the following description be better?:

description = "The Managed Service Identity ID, set this value if you're running this example by using Managed Identity as authentication method."