User Story 125263 #238

TomArcherMsft · 2023-07-17T19:27:02Z

lonegunmanb

Thanks @TomArcherMsft for opening this pr, a few comments.

lonegunmanb · 2023-07-17T23:45:27Z

quickstart/101-vm-with-infrastructure/ssh.tf

+resource "azapi_resource" "ssh_public_key" {
+  type      = "Microsoft.Compute/sshPublicKeys@2022-11-01"
+  name      = random_pet.ssh_key_name.id
+  location  = "westus3"


Is there any special reason we use hard coded location rather than var.resource_group_location here?

lonegunmanb · 2023-07-17T23:47:11Z

quickstart/101-vm-with-infrastructure/ssh.tf

+  response_export_values = ["publicKey"]
+}
+
+output "key_data" {


Could we move this output block to outputs.tf file?

And I've tried this output, it's value is empty. If we want to export the generated public key data as output, we can use the following code snippet:

output "key_data" { value = jsondecode(azapi_resource_action.ssh_public_key_gen.output).publicKey }

Since public key data is not a secret and meant to be shared, I just removed sensitive = true.

Right now, the ssh.tf file is fully encapsulated, meaning that I can drop it into any configuration and it works. I want to keep this pattern instead of having functionality spread across multiple files.

Right now, the ssh.tf file is fully encapsulated, meaning that I can drop it into any configuration and it works. I want to keep this pattern instead of having functionality spread across multiple files.

That makes sense to me.

But I think we still need to ensure that value = azapi_resource.ssh_public_key.body could output the public key, and I still think we could remove this sensitive = true because the public key is meant to be shared.

lonegunmanb · 2023-07-18T00:11:50Z

quickstart/101-vm-with-infrastructure/variables.tf

+variable "username" {
+  type        = string
+  description = "The username for the local account that will be created on the new VM."
+  default     = "azureadmin"


The default value is different than the corresponding default value in coalesce function.

lonegunmanb · 2023-07-18T00:12:54Z

quickstart/101-vm-with-infrastructure/main.tf

-  admin_username                  = "azureuser"
-  disable_password_authentication = true
+  computer_name                   = "hostname"
+  admin_username                  = coalesce(var.username, "azureuser")


Since we have set default value for var.username already I think we can skip coalesce here and use var.username directly.

Using the coalesce() function allows us to give the reader two options: variable or random. If the reader blanks out the var, they get a random value - such as if they're testing the code by running it multiple times and can't use the same value twice.

I understand that you'd like to set a default value for admin_username when var.username is absent, but things would go differently than you thought.

Using the coalesce() function allows us to give the reader two options: variable or random. If the reader blanks out the var, they get a random value

I didn't see any random factors in this case. According to the document:

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

So when var.username is null or empty string, coalesce function would return azureuser as default value.

variable "username" { type = string description = "The username for the local account that will be created on the new VM." default = "azureadmin" }

If the module's caller omits var.username, then var.username's value would be azureadmin.

admin_username = coalesce(var.username, "azureuser")

So when the caller omits var.username, admin_username would be azureadmin, not azureuser.

Btw this is also the reason I suggested a nullable = false for var.username, if we do so, when user set var.username to null on purpose, the var.username's value would still be azureadmin.

lonegunmanb · 2023-07-18T00:13:04Z

quickstart/101-vm-with-infrastructure/main.tf


  admin_ssh_key {
-    username   = "azureuser"
-    public_key = tls_private_key.example_ssh.public_key_openssh
+    username   = coalesce(var.username, "azureuser")


Same issue here.

Using the coalesce() function allows us to give the reader two options: variable or random. If the reader blanks out the var, they get a random value - such as if they're testing the code by running it multiple times and can't use the same value twice.

variable "username" { type = string description = "The username for the local account that will be created on the new VM." default = "azureadmin" }

If the module's caller omits var.username, then var.username's value would be azureadmin.

admin_username = coalesce(var.username, "azureuser")

So when the caller omits var.username, admin_username would be azureadmin, not azureuser.

lonegunmanb · 2023-07-18T00:14:03Z

quickstart/101-vm-with-infrastructure/variables.tf

+}
+
+
+variable "username" {


Since we've set default value for this variable I think we can set nullable = false for this block too.

grayzu

LGTM

lonegunmanb

Hi @TomArcherMsft:

It looks like you've made changes in 101-vm-cluster-linux folder, is that correct modification?
The issues I addressed in 101-vm-with-infrastructure are still there.
It'll be unnecessary to use coalesce along with variable's default value.
I've tested and verified that output "key_data" block would output an empty object {}. Could we change the value to the following code snippet?:

output "key_data" {
  value     = jsondecode(azapi_resource_action.ssh_public_key_gen.output).publicKey
}

lonegunmanb · 2023-07-21T00:25:24Z

quickstart/101-vm-with-infrastructure/main.tf

-  admin_username                  = "azureuser"
-  disable_password_authentication = true
+  computer_name                   = "hostname"
+  admin_username                  = coalesce(var.username, "azureuser")


I understand that you'd like to set a default value for admin_username when var.username is absent, but things would go differently than you thought.

Using the coalesce() function allows us to give the reader two options: variable or random. If the reader blanks out the var, they get a random value

I didn't see any random factors in this case. According to the document:

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

So when var.username is null or empty string, coalesce function would return azureuser as default value.

variable "username" { type = string description = "The username for the local account that will be created on the new VM." default = "azureadmin" }

If the module's caller omits var.username, then var.username's value would be azureadmin.

admin_username = coalesce(var.username, "azureuser")

So when the caller omits var.username, admin_username would be azureadmin, not azureuser.

Btw this is also the reason I suggested a nullable = false for var.username, if we do so, when user set var.username to null on purpose, the var.username's value would still be azureadmin.

lonegunmanb · 2023-07-21T00:25:57Z

quickstart/101-vm-with-infrastructure/main.tf


  admin_ssh_key {
-    username   = "azureuser"
-    public_key = tls_private_key.example_ssh.public_key_openssh
+    username   = coalesce(var.username, "azureuser")


variable "username" { type = string description = "The username for the local account that will be created on the new VM." default = "azureadmin" }

If the module's caller omits var.username, then var.username's value would be azureadmin.

admin_username = coalesce(var.username, "azureuser")

So when the caller omits var.username, admin_username would be azureadmin, not azureuser.

lonegunmanb · 2023-07-21T00:35:58Z

quickstart/101-vm-with-infrastructure/ssh.tf

+  response_export_values = ["publicKey"]
+}
+
+output "key_data" {


Right now, the ssh.tf file is fully encapsulated, meaning that I can drop it into any configuration and it works. I want to keep this pattern instead of having functionality spread across multiple files.

That makes sense to me.

But I think we still need to ensure that value = azapi_resource.ssh_public_key.body could output the public key, and I still think we could remove this sensitive = true because the public key is meant to be shared.

TomArcherMsft · 2023-07-21T02:05:08Z

@lonegunmanb I misspoke about the random value. I see your point now :)

lonegunmanb

Thanks @TomArcherMsft for the update, LGTM!

Updated to use AzApi to obtain SSH (instead of tls)

e605197

TomArcherMsft temporarily deployed to acctests July 17, 2023 19:30 — with GitHub Actions Inactive

lonegunmanb requested changes Jul 18, 2023

View reviewed changes

grayzu approved these changes Jul 18, 2023

View reviewed changes

Made edits per tech review.

bcd7557

TomArcherMsft temporarily deployed to acctests July 20, 2023 19:43 — with GitHub Actions Inactive

lonegunmanb requested changes Jul 21, 2023

View reviewed changes

More edits per review

05c5cc4

TomArcherMsft temporarily deployed to acctests July 21, 2023 02:37 — with GitHub Actions Inactive

TomArcherMsft added 4 commits July 21, 2023 07:27

Changes to output SSH key

369fed9

Changes to output SSH key (2)

2e3a878

Changes to output SSH key (3)

490c01d

Changes to output SSH key (4)

16950b8

TomArcherMsft temporarily deployed to acctests July 21, 2023 14:42 — with GitHub Actions Inactive

Removing private key

deb3ed5

TomArcherMsft temporarily deployed to acctests July 22, 2023 15:28 — with GitHub Actions Inactive

lonegunmanb approved these changes Jul 24, 2023

View reviewed changes

lonegunmanb merged commit 58245fa into Azure:master Jul 24, 2023

TomArcherMsft deleted the UserStory125263 branch July 24, 2023 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User Story 125263 #238

User Story 125263 #238

TomArcherMsft commented Jul 17, 2023

lonegunmanb left a comment

lonegunmanb Jul 17, 2023

TomArcherMsft Jul 20, 2023

lonegunmanb Jul 17, 2023

TomArcherMsft Jul 20, 2023 •

edited

Loading

lonegunmanb Jul 21, 2023

lonegunmanb Jul 18, 2023

TomArcherMsft Jul 20, 2023

lonegunmanb Jul 18, 2023

TomArcherMsft Jul 20, 2023

lonegunmanb Jul 21, 2023

lonegunmanb Jul 18, 2023

TomArcherMsft Jul 20, 2023

lonegunmanb Jul 21, 2023

lonegunmanb Jul 18, 2023

grayzu left a comment

lonegunmanb left a comment •

edited

Loading

lonegunmanb Jul 21, 2023

lonegunmanb Jul 21, 2023

lonegunmanb Jul 21, 2023

TomArcherMsft commented Jul 21, 2023

lonegunmanb left a comment

User Story 125263 #238

User Story 125263 #238

Conversation

TomArcherMsft commented Jul 17, 2023

lonegunmanb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomArcherMsft Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grayzu left a comment

Choose a reason for hiding this comment

lonegunmanb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomArcherMsft commented Jul 21, 2023

lonegunmanb left a comment

Choose a reason for hiding this comment

TomArcherMsft Jul 20, 2023 •

edited

Loading

lonegunmanb left a comment •

edited

Loading