-
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
New Resource: azurerm_compute_fleet
#28758
base: main
Are you sure you want to change the base?
Conversation
11921e3
to
9c5d809
Compare
9c5d809
to
b18cc54
Compare
b18cc54
to
12b281e
Compare
12b281e
to
16ace17
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.
Hi @sinbai ,
Thanks for this PR - I've taken a look through and left some comments. If we can fix those up, this should be good to go 👍
ForceNew: true, | ||
}, | ||
|
||
"vm_attributes": vmAttributesSchema(), |
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.
shall we define the schema inline as it's not reused in other places?
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.
Fixed.
Optional: true, | ||
ForceNew: true, | ||
Default: string(fleets.RegularPriorityAllocationStrategyLowestPrice), | ||
ValidateFunc: validation.StringInSlice([]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.
is there a PossibleValuesFor...
method for this enum?
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.
Fixed.
Optional: true, | ||
ForceNew: true, | ||
Default: string(fleets.SpotAllocationStrategyPriceCapacityOptimized), | ||
ValidateFunc: validation.StringInSlice([]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.
is there a PossibleValuesFor...
method for this enum?
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.
Fixed.
Optional: true, | ||
ForceNew: true, | ||
Default: string(fleets.EvictionPolicyDelete), | ||
ValidateFunc: validation.StringInSlice([]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.
is there a PossibleValuesFor...
method for this enum?
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.
Fixed.
} | ||
} | ||
|
||
if a := model.AdditionalLocationProfile; len(a) > 0 && len(a[0].VirtualMachineProfileOverride) > 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.
if a := model.AdditionalLocationProfile; len(a) > 0 && len(a[0].VirtualMachineProfileOverride) > 0 { | |
if additionalLocationProfile := model.AdditionalLocationProfile; len(additionalLocationProfile) > 0 && len(additionalLocationProfile[0].VirtualMachineProfileOverride) > 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.
Fixed.
|
||
* `additional_unattend_content` - (Optional) One or more `additional_unattend_content` blocks as defined above. | ||
|
||
* `automatic_updates_enabled` - (Optional) Should the automatic updates of the virtual machines be enabled? Defaults to `true`. |
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.
* `automatic_updates_enabled` - (Optional) Should the automatic updates of the virtual machines be enabled? Defaults to `true`. | |
* `automatic_updates_enabled` - (Optional) Whether to enable the automatic updates of the virtual machines. Defaults to `true`. |
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.
Fixed.
|
||
* `automatic_updates_enabled` - (Optional) Should the automatic updates of the virtual machines be enabled? Defaults to `true`. | ||
|
||
* `bypass_platform_safety_checks_enabled` - (Optional) Should the customer to schedule patching without accidental upgrades be enabled? Defaults to `false`. |
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.
same as linux_configuration
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.
Fixed.
|
||
* `bypass_platform_safety_checks_enabled` - (Optional) Should the customer to schedule patching without accidental upgrades be enabled? Defaults to `false`. | ||
|
||
* `hot_patching_enabled` - (Optional) Should the customers to patch the virtual machines without requiring a reboot be enabled? Defaults to `false`. |
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.
* `hot_patching_enabled` - (Optional) Should the customers to patch the virtual machines without requiring a reboot be enabled? Defaults to `false`. | |
* `hot_patching_enabled` - (Optional) Whether to enable the customers to patch the virtual machines without requiring a reboot. Defaults to `false`. |
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.
Fixed.
|
||
* `patch_mode` - (Optional) Specifies the mode of in-guest patching of the virtual machines. Possible values are `AutomaticByOS`, `AutomaticByPlatform` and `Manual`. | ||
|
||
* `provision_vm_agent_enabled` - (Optional) Should the virtual machine agent be provisioned on each virtual machine in the Scale Set? Defaults to `true`. |
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.
same as linux_configuration
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.
Fixed.
|
||
* `time_zone` - (Optional) Specifies the time zone of the windows virtual machine. Changing this forces a new resource to be created. | ||
|
||
* `vm_agent_platform_updates_enabled` - (Optional) Should the virtual machine agent platform updates be enabled for the windows virtual machine? Defaults to `false`. |
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.
same as linux_configuration
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.
Fixed.
Hi @ms-zhenhua thank you very much for your feedback. I have updated the code, could you please take another look? |
3e24366
to
5f293b9
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.
Hi @sinbai,
Thank you for the updates. Only one comment left. Others LGTM~
@@ -890,29 +876,31 @@ func (r ComputeFleetResource) Update() sdk.ResourceFunc { | |||
|
|||
// API requires `osProfile.adminPassword` when updating resource but the GET API does not return the sensitive data `osProfile.adminPassword` | |||
if props := properties.Properties; props != nil { | |||
if len(model.VirtualMachineProfile[0].OsProfile[0].LinuxConfiguration) > 0 { | |||
if v := props.ComputeProfile.BaseVirtualMachineProfile.OsProfile; v != nil { | |||
if v := props.ComputeProfile.BaseVirtualMachineProfile.OsProfile; v != 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.
what if ComputeProfile == nil
or BaseVirtualMachineProfile == 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.
ComputeProfile
and BaseVirtualMachineProfile
are not pointer types, so I assume they will not be equal to nil.
Hi @ms-zhenhua thanks for you time and feedback. Could you please take another look? |
Community Note
Description
API: https://github.com/Azure/azure-rest-api-specs/blob/main/specification/azurefleet/resource-manager/Microsoft.AzureFleet/stable/2024-11-01/azurefleet.json
Doc: https://learn.microsoft.com/en-us/azure/azure-compute-fleet/overview
PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_compute_fleet
- new resourceThis is a (please select all that apply):