-
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_fabric_capacity
#28080
Conversation
68ca5e8
to
59f690f
Compare
59f690f
to
b768099
Compare
b768099
to
47b1608
Compare
47b1608
to
b646fce
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 inline. If we can fix those up, this should be good to 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) |
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.
return nil, fmt.Errorf("building fabric client: %+v", err) | |
return nil, fmt.Errorf("building fabric client: %+v", err) |
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.
Required: true, | ||
ForceNew: true, | ||
ValidateFunc: validation.StringMatch( | ||
regexp.MustCompile(`^[a-z]([a-z\d]{1,61}[a-z\d])$`), |
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.
regexp.MustCompile(`^[a-z]([a-z\d]{1,61}[a-z\d])$`), | |
regexp.MustCompile(`^[a-z]([a-z\d]{2,62})$`), |
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.
"administration_members": { | ||
Type: pluginsdk.TypeList, | ||
Optional: true, | ||
MaxItems: 1, |
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.
each instance can have only 1 administration member ?
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.
@ms-zhenhua @sinbai Fabric Capacity can have multiple admin members (UPNs for users, GUIDs for Service Principals)
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 len(model.AdministrationMembers) == 0 { | ||
return fmt.Errorf("`administration_members` is required when creating a fabric capacity") |
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.
why not set administration_members
as Required
and require at least one member?
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.
administration_members
should be required
. At least one admin member is expected.
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.
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.
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 happens when we update the resource to delete administration_members
, is this resource still manageable/usable in the Portal or via Terraform?
|
||
properties.Properties.Administration = pointer.From(expandCapacityAdministrationModel(model.AdministrationMembers)) | ||
|
||
properties.Sku = pointer.From(expandSkuModel(model.Sku)) |
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.
seems it can be put where initialize properties
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.
} | ||
} | ||
|
||
func expandCapacityAdministrationModel(inputList []string) *fabriccapacities.CapacityAdministration { |
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.
expand and flatten should appear in pair. Shall we directly put this logic in Create/Update ?
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.
`, template, data.RandomInteger, data.Locations.Primary) | ||
} | ||
|
||
func (r FabricFabricCapacityResource) update(data acceptance.TestData) 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.
does it make sense to remove all administration_members
?
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 test is added because the API allows to remove all administration_members
and this is also allowed on the Azure Portal.
}) | ||
} | ||
|
||
func TestAccFabricFabricCapacity_update(t *testing.T) { |
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.
suggest add complete -> basic -> complete
to test add/remove properties
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.
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.
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
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.
|
||
* `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`. |
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.
* `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`. |
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.
|
||
## Import | ||
|
||
Fabric Capacity can be imported using the `resource id`, e.g. |
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.
Fabric Capacity can be imported using the `resource id`, e.g. | |
Fabric Capacities can be imported using the `resource id`, e.g. |
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 thanks for your feedback. I have fixed/replied the comments. Could you please take another look? |
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.
Thanks for the update. LGTM~
Hi @stephybun thanks for your time and feedback. I have fixed/replied the comments, could you please take another look? For |
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.
Thanks @sinbai. One minor comment that @ms-zhenhua pointed out about the tests needs to be fixed up, but then this should be good to go.
client := clients.Fabric.FabricCapacitiesClient | ||
resp, err := client.Get(ctx, *id) |
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.
client := clients.Fabric.FabricCapacitiesClient | |
resp, err := client.Get(ctx, *id) | |
resp, err := clients.Fabric.FabricCapacitiesClient(ctx, *id) |
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 response.WasNotFound(resp.HttpResponse) { | ||
return pointer.To(false), 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.
That's right, thanks for pointing that out @ms-zhenhua
Hi @stephybun the comment pointed out by Zhenhua have been fixed along with the removal of the resource not found comment, see below for details. Could you please take another look? |
type FabricFabricCapacityResource struct{} | ||
|
||
func TestAccFabricFabricCapacity_basic(t *testing.T) { |
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.
Just noticed the naming of this, I don't think it's necessary to repeat the service name here..
Please rename the remaining tests
type FabricFabricCapacityResource struct{} | |
func TestAccFabricFabricCapacity_basic(t *testing.T) { | |
type FabricCapacityResource struct{} | |
func TestAccFabricCapacity_basic(t *testing.T) { |
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.
Thanks for your feedback, fixed.
|
||
* `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. |
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.
Can we qualify this as
* `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. |
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.
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.
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?
administration_members = [data.azurerm_client_config.current.object_id] | ||
|
||
sku { | ||
name = "F32" |
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.
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.
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.
administration_members = [] | ||
|
||
sku { | ||
name = "F64" |
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.
Likewise here, there are cheaper skus available like F2/F4/F6
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 @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? |
@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. |
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. |
1 similar comment
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. |
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.
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>
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? |
Community Note
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
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Testing
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.
azurerm_fabric_capacity
This is a (please select all that apply):