-
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_virtual_machine_scale_set: make os_profile.0.admin_password property optional #1958
Conversation
eef3dba
to
461eb96
Compare
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.
A few comments we should fix up - like the Set->List change which I think wants reverting, but this otherwise LGTM 👍
} | ||
|
||
return 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.
Admin Password is a field which (IME) is almost always interpolated outside of simple scenarios - in which case this won't work, as such can we remove the CustomizeDiff here?
ssh_keys := make([]map[string]interface{}, 0, len(*config.SSH.PublicKeys)) | ||
for _, i := range *config.SSH.PublicKeys { | ||
key := make(map[string]interface{}) | ||
key["path"] = *i.Path |
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.
could we nil-check this?
if config != nil { | ||
result["disable_password_authentication"] = *config.DisablePasswordAuthentication | ||
|
||
if config.SSH != nil && len(*config.SSH.PublicKeys) > 0 { |
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.
could we also nil-check config.SSH.PublicKeys
here?
@@ -270,7 +271,7 @@ func resourceArmVirtualMachineScaleSet() *schema.Resource { | |||
}, | |||
|
|||
"os_profile_linux_config": { | |||
Type: schema.TypeSet, | |||
Type: schema.TypeList, |
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.
this could be a bigger scope than we anticipated, given how the ARM API's return the latest snapshot of a resource; I think it'd be worth reverting this back to a Set for the moment (with a separate test function, and the hash function; and then spending some time on both VM/VMSS for 2.0?)
8eb204a
to
0444aa3
Compare
Addressed comments @tombuildsstuff |
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 👍
@tombuildsstuff Thanks for fix. |
* Requires terraform-provider-azurerm v1.16.0 or higher hashicorp/terraform-provider-azurerm#1958
* Requires terraform-provider-azurerm v1.16.0 or higher hashicorp/terraform-provider-azurerm#1958
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! |
fixes #1956
Also makes
os_profile_linux_config
a list