New Resource : `azurerm_fabric_capacity` by sinbai · Pull Request #28080 · hashicorp/terraform-provider-azurerm

sinbai · 2024-11-21T08:21:34Z

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

New resource azurerm_fabric_capacity.

Swagger: https://github.com/Azure/azure-rest-api-specs/blob/fc215e47785166d4c1103909380705a2dfd06a55/specification/fabric/resource-manager/Microsoft.Fabric/stable/2023-11-01/fabric.json#L303

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 relevent 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)

PASS: TestAccFabricFabricCapacity_basic (323.79s)
PASS: TestAccFabricFabricCapacity_requiresImport (215.83s)
PASS: TestAccFabricFabricCapacity_update (551.16s)
PASS: TestAccFabricFabricCapacity_complete (315.63s)

Change Log

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

New Resource : azurerm_fabric_capacity

This is a (please select all that apply):

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

ms-zhenhua

Hi @sinbai,

Thanks for this PR - I've taken a look through and left some comments inline. If we can fix those up, this should be good to go 👍

ms-zhenhua · 2024-11-25T02:25:48Z

internal/services/fabric/client/client.go

+func NewClient(o *common.ClientOptions) (*Client, error) {
+	fabricCapacitiesClient, err := fabriccapacities.NewFabricCapacitiesClientWithBaseURI(o.Environment.ResourceManager)
+	if err != nil {
+		return nil, fmt.Errorf("building fabric  client: %+v", err)


Suggested change

return nil, fmt.Errorf("building fabric client: %+v", err)

return nil, fmt.Errorf("building fabric client: %+v", err)

ms-zhenhua · 2024-11-25T02:39:55Z

internal/services/fabric/fabric_capacity_resource.go

+			Required: true,
+			ForceNew: true,
+			ValidateFunc: validation.StringMatch(
+				regexp.MustCompile(`^[a-z]([a-z\d]{1,61}[a-z\d])$`),


Suggested change

regexp.MustCompile(`^[a-z]([a-z\d]{1,61}[a-z\d])$`),

regexp.MustCompile(`^[a-z]([a-z\d]{2,62})$`),

ms-zhenhua · 2024-11-25T02:47:01Z

internal/services/fabric/fabric_capacity_resource.go

+		"administration_members": {
+			Type:     pluginsdk.TypeList,
+			Optional: true,
+			MaxItems: 1,


each instance can have only 1 administration member ?

@ms-zhenhua @sinbai Fabric Capacity can have multiple admin members (UPNs for users, GUIDs for Service Principals)

ms-zhenhua · 2024-11-25T02:49:21Z

internal/services/fabric/fabric_capacity_resource.go

+			}
+
+			if len(model.AdministrationMembers) == 0 {
+				return fmt.Errorf("`administration_members` is required when creating a fabric capacity")


why not set administration_members as Required and require at least one member?

administration_members should be required. At least one admin member is expected.

Set administration_members to Optional and add a required check in create function. This is because after verifying the behavior of the API, it was found that administration_members is required when creating resource, but administration_members can be unspecified or cleared when updating resource. In addition, administration_members can also be deleted on the Azure Portal.

What happens when we update the resource to delete administration_members, is this resource still manageable/usable in the Portal or via Terraform?

ms-zhenhua · 2024-11-25T02:51:45Z

internal/services/fabric/fabric_capacity_resource.go

+
+			properties.Properties.Administration = pointer.From(expandCapacityAdministrationModel(model.AdministrationMembers))
+
+			properties.Sku = pointer.From(expandSkuModel(model.Sku))


seems it can be put where initialize properties

ms-zhenhua · 2024-11-25T06:20:27Z

internal/services/fabric/fabric_capacity_resource.go

+	}
+}
+
+func expandCapacityAdministrationModel(inputList []string) *fabriccapacities.CapacityAdministration {


expand and flatten should appear in pair. Shall we directly put this logic in Create/Update ?

ms-zhenhua · 2024-11-25T06:35:44Z

internal/services/fabric/fabric_capacity_resource_test.go

+`, template, data.RandomInteger, data.Locations.Primary)
+}
+
+func (r FabricFabricCapacityResource) update(data acceptance.TestData) string {


does it make sense to remove all administration_members?

This test is added because the API allows to remove all administration_members and this is also allowed on the Azure Portal.

ms-zhenhua · 2024-11-25T06:37:48Z

internal/services/fabric/fabric_capacity_resource_test.go

+	})
+}
+
+func TestAccFabricFabricCapacity_update(t *testing.T) {


suggest add complete -> basic -> complete to test add/remove properties

sorry, I may not say it clearly. The update testcase should be complete -> update -> basic -> complete. complete -> update will test updating optional properties. update -> basic will test removing optional properties and basic -> complete will test adding optional properties

ms-zhenhua · 2024-11-25T06:43:05Z

website/docs/r/fabric_capacity.html.markdown

+
+* `name` - (Required) The name of the SKU to use for the Fabric Capacity.
+
+* `tier` - (Required) The tier of the SKU to use for the Fabric Capacity. Possible value is `Fabric`.


Suggested change

* `tier` - (Required) The tier of the SKU to use for the Fabric Capacity. Possible value is `Fabric`.

* `tier` - (Required) The tier of the SKU to use for the Fabric Capacity. The only possible value is `Fabric`.

ms-zhenhua · 2024-11-25T06:44:31Z

website/docs/r/fabric_capacity.html.markdown

+
+## Import
+
+Fabric Capacity can be imported using the `resource id`, e.g.


Suggested change

Fabric Capacity can be imported using the `resource id`, e.g.

Fabric Capacities can be imported using the `resource id`, e.g.

sinbai · 2024-11-27T05:41:08Z

Hi @sinbai,

Thanks for this PR - I've taken a look through and left some comments inline. If we can fix those up, this should be good to go 👍

Hi @ms-zhenhua thanks for your feedback. I have fixed/replied the comments. Could you please take another look?

ms-zhenhua

Thanks for the update. LGTM~

stephybun · 2024-12-03T08:49:32Z

internal/services/fabric/fabric_capacity_resource_test.go

+type FabricFabricCapacityResource struct{}
+
+func TestAccFabricFabricCapacity_basic(t *testing.T) {


Just noticed the naming of this, I don't think it's necessary to repeat the service name here..

Please rename the remaining tests

Suggested change

type FabricFabricCapacityResource struct{}

func TestAccFabricFabricCapacity_basic(t *testing.T) {

type FabricCapacityResource struct{}

func TestAccFabricCapacity_basic(t *testing.T) {

Thanks for your feedback, fixed.

stephybun · 2024-12-03T08:50:40Z

website/docs/r/fabric_capacity.html.markdown

+
+* `sku` - (Required) A `sku` block as defined below.
+
+* `administration_members` - (Optional) An array of administrator user identities. The member must be a member user or a service principal.


Can we qualify this as

Suggested change

* `administration_members` - (Optional) An array of administrator user identities. The member must be a member user or a service principal.

* `administration_members` - (Optional) An array of administrator user identities. The member must be an Entra member user or a service principal.

stephybun

Last thing @sinbai, we can't seem to run the tests in TC:

------- Stdout: -------
=== RUN   TestAccFabricFabricCapacity_basic
=== PAUSE TestAccFabricFabricCapacity_basic
=== CONT  TestAccFabricFabricCapacity_basic
    testcase.go:173: Step 1/3 error: Error running apply: exit status 1
        Error: creating Capacity (Subscription: "*******"
        Resource Group Name: "acctest-rg-241203102435562466"
        Capacity Name: "acctestffc241203102435562466"): performing CreateOrUpdate: unexpected status 400 (400 Bad Request) with error: BadRequest: Tenant '*******' wasn't recognized by Microsoft Fabric. Sign up for Microsoft Fabric and try again.

It looks like we can only sign up using one of our user accounts. Can you ask the service team how can proceed here to get our tenant registered without signing up with our a specific user account?

stephybun · 2024-12-03T11:01:18Z

internal/services/fabric/fabric_capacity_resource_test.go

+  administration_members = [data.azurerm_client_config.current.object_id]
+
+  sku {
+    name = "F32"


Also why aren't we using a cheaper sku here like F2?

F32 can cost up to 5139 USD a month if the test fails or there's a problem with deletion.

stephybun · 2024-12-03T11:02:22Z

internal/services/fabric/fabric_capacity_resource_test.go

+  administration_members = []
+
+  sku {
+    name = "F64"


Likewise here, there are cheaper skus available like F2/F4/F6

sinbai · 2024-12-04T03:07:49Z

Last thing @sinbai, we can't seem to run the tests in TC:

------- Stdout: -------
=== RUN   TestAccFabricFabricCapacity_basic
=== PAUSE TestAccFabricFabricCapacity_basic
=== CONT  TestAccFabricFabricCapacity_basic
    testcase.go:173: Step 1/3 error: Error running apply: exit status 1
        Error: creating Capacity (Subscription: "*******"
        Resource Group Name: "acctest-rg-241203102435562466"
        Capacity Name: "acctestffc241203102435562466"): performing CreateOrUpdate: unexpected status 400 (400 Bad Request) with error: BadRequest: Tenant '*******' wasn't recognized by Microsoft Fabric. Sign up for Microsoft Fabric and try again.

It looks like we can only sign up using one of our user accounts. Can you ask the service team how can proceed here to get our tenant registered without signing up with our a specific user account?

Hi @stephybun sure. However, I found the documentation for enabling tenant for microsoft fabric. Could you please try to follow the documentation before asking the service team?

stephybun · 2024-12-05T10:26:53Z

@sinbai we followed the documentation but the requesting user must be signed up as well as enabled on the Tenant. The signup link is bound to an Entra ID user, we need to be able to configure for an SP.

sinbai · 2024-12-06T02:26:34Z

@sinbai we followed the documentation but the requesting user must be signed up as well as enabled on the Tenant. The signup link is bound to an Entra ID user, we need to be able to configure for an SP.

Hi @stephybun thanks for your feedback. I have contacted the service team and look forward to their response.

badeamarjieh · 2024-12-10T11:54:10Z

The sign-up is a one-time setup for Microsoft Fabric that needs to be done by a user, preferably Fabric admin, on behalf of the tenant.
Once the sign-up is completed, then users and SPs in this tenant can use Fabric (and capacities). However, note that SPs will be disabled by default, and to enable them, you need to explicitly enable service principals to use Fabric APIs. see documentation

badeamarjieh · 2024-12-10T11:54:39Z

The sign-up is a one-time setup for Microsoft Fabric that needs to be done by a user, preferably Fabric admin, on behalf of the tenant.
Once the sign-up is completed, then users and SPs in this tenant can use Fabric (and capacities). However, note that SPs will be disabled by default, and to enable them, you need to explicitly enable service principals to use Fabric APIs. see documentation

stephybun

Thanks @sinbai tests are passing 👍

* Update CHANGELOG.md for #26304 * Update CHANGELOG.md for #28211 * Update for #28016 * Update for #28139 * Update for #27776 * Update for #28227 * Update for #28080 * Update for #28228 * Update for #27915 * reword nginx api upgrade * Update for #28160 * Update for #28043 * run changelog-update-for-release.sh --------- Co-authored-by: stephybun <steph@hashicorp.com> Co-authored-by: kt <kt@katbyte.me>

Lachlan-White · 2024-12-24T02:43:12Z

Hey, @sinbai currently getting the below error when following the documented example.

sinbai · 2024-12-24T05:10:46Z

Hey, @sinbai currently getting the below error when following the documented example.

Hi @Lachlan-White Terraform creates fabric by calling Azure API. The error seems to be returned by Azure API. Perhaps you could try calling the API directly to see if the same error occurs?

Lachlan-White · 2024-12-26T22:09:39Z

@sinbai Have tested this now, So using my UPN (x.x@x.com) works however using my object_id doesn't. Seems to be a bug?

sinbai · 2024-12-27T02:30:17Z

@sinbai Have tested this now, So using my UPN (x.x@x.com) works however using my object_id doesn't. Seems to be a bug?
Hi @Lachlan-White since using object_id works for me, I'm assuming this isn't a bug. Also, I'm not an expert on the API, so could you please contact Microsoft support for more help?

Lachlan-White · 2024-12-27T02:32:11Z

@sinbai The cause is because for Users only UPN works, for Service Principals & Managed Identities Object_ID works.

* support new resource azurerm_fabric_capacity * fix comments * fix comments * fix comments * fix comments * fix comments

* Update CHANGELOG.md for hashicorp#26304 * Update CHANGELOG.md for hashicorp#28211 * Update for hashicorp#28016 * Update for hashicorp#28139 * Update for hashicorp#27776 * Update for hashicorp#28227 * Update for hashicorp#28080 * Update for hashicorp#28228 * Update for hashicorp#27915 * reword nginx api upgrade * Update for hashicorp#28160 * Update for hashicorp#28043 * run changelog-update-for-release.sh --------- Co-authored-by: stephybun <steph@hashicorp.com> Co-authored-by: kt <kt@katbyte.me>

github-actions · 2025-01-27T02:13:17Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

github-actions bot added documentation dependencies size/XL waiting-response and removed waiting-response labels Nov 21, 2024

sinbai force-pushed the fabric/new_resource branch from 68ca5e8 to 59f690f Compare November 21, 2024 08:48

github-actions bot added waiting-response and removed waiting-response labels Nov 21, 2024

sinbai force-pushed the fabric/new_resource branch from 59f690f to b768099 Compare November 21, 2024 08:59

github-actions bot added waiting-response and removed waiting-response labels Nov 21, 2024

sinbai force-pushed the fabric/new_resource branch from b768099 to 47b1608 Compare November 21, 2024 10:07

github-actions bot added waiting-response and removed waiting-response labels Nov 21, 2024

support new resource azurerm_fabric_capacity

b646fce

sinbai force-pushed the fabric/new_resource branch from 47b1608 to b646fce Compare November 21, 2024 11:22

github-actions bot removed the waiting-response label Nov 21, 2024

rcskosir added the new-resource label Nov 21, 2024

ms-zhenhua suggested changes Nov 25, 2024

View reviewed changes

github-actions bot added the waiting-response label Nov 25, 2024

fix comments

bef6853

github-actions bot removed the waiting-response label Nov 26, 2024

github-actions bot added the waiting-response label Nov 27, 2024

fix comments

13b9ed3

github-actions bot removed the waiting-response label Nov 27, 2024

ms-zhenhua approved these changes Nov 27, 2024

View reviewed changes

sinbai marked this pull request as ready for review November 27, 2024 08:24

sinbai requested a review from a team as a code owner November 27, 2024 08:24

stephybun suggested changes Dec 3, 2024

View reviewed changes

github-actions bot added the waiting-response label Dec 3, 2024

fix comments

853f2fd

github-actions bot removed the waiting-response label Dec 3, 2024

stephybun reviewed Dec 3, 2024

View reviewed changes

github-actions bot added the waiting-response label Dec 3, 2024

stephybun reviewed Dec 3, 2024

View reviewed changes

fix comments

ce09193

github-actions bot removed the waiting-response label Dec 4, 2024

stephybun approved these changes Dec 10, 2024

View reviewed changes

stephybun merged commit 7a2f40e into hashicorp:main Dec 10, 2024

github-actions bot added this to the v4.14.0 milestone Dec 10, 2024

stephybun added a commit that referenced this pull request Dec 10, 2024

Update for #28080

aafb4c8

DariuszPorowski mentioned this pull request Dec 14, 2024

Support for [Microsoft Fabric] #24522

Closed

1 task

github-actions bot locked as resolved and limited conversation to collaborators Jan 27, 2025

	return nil, fmt.Errorf("building fabric client: %+v", err)
	return nil, fmt.Errorf("building fabric client: %+v", err)

	regexp.MustCompile(`^[a-z]([a-z\d]{1,61}[a-z\d])$`),
	regexp.MustCompile(`^[a-z]([a-z\d]{2,62})$`),


		properties.Properties.Administration = pointer.From(expandCapacityAdministrationModel(model.AdministrationMembers))

		properties.Sku = pointer.From(expandSkuModel(model.Sku))


		* `name` - (Required) The name of the SKU to use for the Fabric Capacity.

		* `tier` - (Required) The tier of the SKU to use for the Fabric Capacity. Possible value is `Fabric`.


		## Import

		Fabric Capacity can be imported using the `resource id`, e.g.

	Fabric Capacity can be imported using the `resource id`, e.g.
	Fabric Capacities can be imported using the `resource id`, e.g.

		type FabricFabricCapacityResource struct{}

		func TestAccFabricFabricCapacity_basic(t *testing.T) {


		* `sku` - (Required) A `sku` block as defined below.

		* `administration_members` - (Optional) An array of administrator user identities. The member must be a member user or a service principal.

Conversation

sinbai commented Nov 21, 2024

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Uh oh!

ms-zhenhua left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ms-zhenhua Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sinbai commented Nov 27, 2024

Uh oh!

ms-zhenhua left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephybun left a comment

Choose a reason for hiding this comment

Uh oh!

ms-zhenhua Nov 25, 2024 •

edited

Loading

stephybun commented Dec 5, 2024 •

edited

Loading