Skip to content

azurerm_compute_fleet - add new resource#32174

Draft
v-yhyeo0202 wants to merge 21 commits intohashicorp:mainfrom
v-yhyeo0202:add-compute-fleet
Draft

azurerm_compute_fleet - add new resource#32174
v-yhyeo0202 wants to merge 21 commits intohashicorp:mainfrom
v-yhyeo0202:add-compute-fleet

Conversation

@v-yhyeo0202
Copy link
Copy Markdown
Collaborator

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

azurerm_compute_fleet resource is added. The related documentations are listed below.

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

This work is a continuation of #28758.

PR Checklist

  • I have followed the guidelines in our Contributing Documentation.
  • I have checked to ensure there aren't other open Pull Requests for the same update/change.
  • I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
  • I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
  • I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
    For example: “resource_name_here - description of change e.g. adding property new_property_name_here

Changes to existing Resource / Data Source

  • I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
  • I have written new tests for my resource or datasource changes & updated any relevant documentation.
  • I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
  • (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

  • My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

  • azurerm_compute_fleet - new resource

This is a (please select all that apply):

  • Bug Fix
  • New Feature (ie adding a service, resource, or data source)
  • Enhancement
  • Breaking Change

return &pluginsdk.Schema{
Type: pluginsdk.TypeList,
Required: true,
ForceNew: true,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The max number of network interface that can be configured is 8. Let's set MaxItems: 8 here.

https://learn.microsoft.com/en-us/azure/virtual-machine-scale-sets/virtual-machine-scale-sets-networking?tabs=portal1#multiple-ip-addresses-per-nic (doc is for VMSS, but applicable since fleet manages set of VMSS)

Optional: true,
ForceNew: true,
ValidateFunc: validation.StringInSlice([]string{
// NOTE: because there is a `None` value in the possible values, it's handled in the Create/Update and Read functions.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this comment, this is a well known pattern in azurerm. Please also remove the same comments in other places.

Suggested change
// NOTE: because there is a `None` value in the possible values, it's handled in the Create/Update and Read functions.


* `zones` - (Optional) Specifies a list of availability zones in which the Compute Fleet is available. Changing this forces a new resource to be created.

* `compute_api_version` - (Optional) Specifies the `Microsoft.Compute` API version to use when creating the Compute Fleet. Changing this forces a new resource to be created.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default value should be documented

Suggested change
* `compute_api_version` - (Optional) Specifies the `Microsoft.Compute` API version to use when creating the Compute Fleet. Changing this forces a new resource to be created.
* `compute_api_version` - (Optional) Specifies the `Microsoft.Compute` API version to use when creating the Compute Fleet. Defaults to latest supported API version if not provided. Changing this forces a new resource to be created.

"compute_api_version": {
Type: pluginsdk.TypeString,
Optional: true,
// NOTE: O+C Azure generates a value if not specified
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with with this O+C and ForceNew: true design but the explanation note should be improved:

  1. I have tested computeProfile.computeApiVersion is non-updatable (ForceNew: true is correct)
  2. Creating new fleet without providing this will have default value set (Optional: true and Computed: true is correct)
  3. The default value (latest supported version of Microsoft.Compute API) is not documented anywhere, it will be poor user experience to make this a Required arg

},
},
},
},
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire block has to be renamed to match portal

  • regular_priority_profile -> on_demand_capacity
  • capacity -> target_capacity
  • min_capacity -> minimum_starting_capacity (avoid abbreviations)
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants