-
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~
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 and @ms-zhenhua for the review.
Overall this looks good, could you take a look at the questions and comments left in-line?
.github/labeler-issue-triage.yml
Outdated
@@ -144,7 +144,7 @@ service/event-hubs: | |||
- '### (|New or )Affected Resource\(s\)\/Data Source\(s\)((.|\n)*)azurerm_eventhub((.|\n)*)###' | |||
|
|||
service/extended-location: | |||
- '### (|New or )Affected Resource\(s\)\/Data Source\(s\)((.|\n)*)azurerm_extended_custom_location((.|\n)*)###' | |||
- '### (|New or )Affected Resource\(s\)\/Data Source\(s\)((.|\n)*)azurerm_(extended_custom_location|fabric_capacity)((.|\n)*)###' |
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.
I know this is generated but this doesn't look right to me. I don't think fabric PRs should be having the service/extended-location
labels applied
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.
Yes, you are right, thanks for finding that. This issue has been fixed by adding AssociatedGitHubLabel
in registration.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 capacities 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.
"name": { | ||
Type: pluginsdk.TypeString, | ||
Required: true, | ||
ValidateFunc: validation.StringIsNotEmpty, |
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.
Are there no enums or validation we can use here?
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.
Swagger does not define possible values for name
, I hardcoded its possible values.
"administration_members": { | ||
Type: pluginsdk.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.
If this is unlikely to be referenced as input for anything else, then we could define this as a set instead of a list and remove the logic to check for duplicates since a set doesn't allow duplicates.
"administration_members": { | |
Type: pluginsdk.TypeList, | |
"administration_members": { | |
Type: pluginsdk.TypeSet, |
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.
} | ||
|
||
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.
What happens when we update the resource to delete administration_members
, is this resource still manageable/usable in the Portal or via Terraform?
} | ||
} | ||
|
||
func flattenSkuModel(input *fabriccapacities.RpSku) []SkuModel { |
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.
func flattenSkuModel(input *fabriccapacities.RpSku) []SkuModel { | |
func flattenSkuModel(input fabriccapacities.RpSku) []SkuModel { |
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 (r FabricCapacityResource) CustomizeDiff() sdk.ResourceFunc { | ||
return sdk.ResourceFunc{ | ||
Timeout: 5 * time.Minute, | ||
Func: func(ctx context.Context, metadata sdk.ResourceMetaData) error { | ||
var state FabricCapacityResourceModel | ||
if err := metadata.DecodeDiff(&state); err != nil { | ||
return fmt.Errorf("DecodeDiff: %+v", err) | ||
} | ||
|
||
if len(state.AdministrationMembers) > 0 { | ||
existing := make(map[string]bool) | ||
for _, str := range state.AdministrationMembers { | ||
if existing[str] { | ||
return fmt.Errorf("`administration_members` contains the duplicate value %q", str) | ||
} | ||
existing[str] = true | ||
} | ||
} | ||
|
||
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.
If administration_members
becomes a set then we don't need any of this. Please double check the point about whether this property needs to be referenced elsewhere before making that judgement call.
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
is not referenced anywhere else, so CustomizeDiff
is removed.
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.
In the majority of cases there is no need to check for 404 in the Exists function for tests so this is actually unnecessary.
I've been seeing this regularly as of late @ms-zhenhua could you please make sure to flag these in any future PR reviews that you do?
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.
Sure, thank you for the reminder.
BTW, according to the latest guidance, resp, err := client.Get(ctx, *id)
should be replaced with resp, err := clients.Fabric.FabricCapacitiesClient.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.
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.
That's right, thanks for pointing that out @ms-zhenhua
type Registration struct{} | ||
|
||
var _ sdk.TypedServiceRegistration = Registration{} | ||
|
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.
Regarding the comment I left on the generated file for the github issue labeler, I think you can add another method here where you can set the label that applies to resources in this service which should hopefully mean that the issue labeler file gets generated correctly.
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.
Thank you very much for your discovery. Indeed, the labeler file could be generated correctly after appending the AssociatedGitHubLabel
method.
|
||
* `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.
What is a member user?
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.
I assume that member user means the user type is Memeber
instead of Guest
.
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>
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):