Skip to content
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

Enhancement: Support Preflight Validation #501

Merged
merged 20 commits into from
Sep 5, 2024

Conversation

ms-zhenhua
Copy link
Collaborator

@ms-zhenhua ms-zhenhua commented May 10, 2024

Related Document:

https://armwiki.azurewebsites.net/api_contracts/preflight.html

Test Results

--- PASS: TestAccGenericResource_defaultParentId (169.21s)
--- PASS: TestAccGenericResource_disablePreflightValidation (173.48s)
--- PASS: TestAccGenericResource_preflightResourceGroupLevelValidation (186.22s)
--- PASS: TestAccGenericResource_ignoreChanges (213.73s)
--- PASS: TestAccGenericResource_basic (255.66s)
--- PASS: TestAccGenericResource_ignoreChangesArray (265.48s)
--- PASS: TestAccGenericResource_completeBody (271.28s)
--- PASS: TestAccGenericResource_defaultsNaming (341.81s)
--- PASS: TestAccGenericResource_requiresImport (150.80s)
--- PASS: TestAccGenericResource_defaultLocation (379.96s)
--- PASS: TestAccGenericResource_defaultsNotApplicable (213.14s)
--- PASS: TestAccGenericResource_complete (214.40s)
--- PASS: TestAccGenericResource_importWithApiVersion (228.25s)
--- PASS: TestAccGenericResource_locks (207.35s)
--- PASS: TestAccGenericResource_subscriptionScope (124.44s)
--- PASS: TestAccGenericResource_nullLocation (611.74s)
--- PASS: TestAccGenericResource_ignoreCasing (345.26s)
--- PASS: TestAccGenericResource_identity (459.51s)
--- PASS: TestAccGenericResource_defaultTags (507.41s)
--- PASS: TestAccGenericResource_invalidVersionUpdate (318.07s)
--- PASS: TestAccGenericResource_dynamicSchema (214.23s)
--- PASS: TestAccGenericResource_deleteLROEndsWithNotFoundError (344.94s)
--- PASS: TestAccGenericResource_secretsInAsterisks (748.64s)
--- PASS: TestAccGenericResource_ignoreMissingProperty (420.70s)
PASS
ok github.com/Azure/terraform-provider-azapi/internal/services 842.574s

@ms-zhenhua ms-zhenhua marked this pull request as ready for review May 10, 2024 06:28
@ms-zhenhua ms-zhenhua requested a review from ms-henglu May 10, 2024 06:29
Copy link
Member

@ms-henglu ms-henglu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! It mostly LGTM, just some minor suggestions.

requestBody["scope"] = requestPlan.ParentID.ValueString()
requestBody["resources"] = []map[string]interface{}{resourceBody}

_, err = client.Action(ctx, "/providers/Microsoft.Resources", "validateResources", "2020-10-01", "POST", requestBody)
Copy link
Member

Choose a reason for hiding this comment

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

Please define the request body struct for validateResources API instead of using the generic struct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated. Thanks.

@@ -929,6 +938,40 @@ func expandBody(body map[string]interface{}, model AzapiResourceModel) diag.Diag
return diag.Diagnostics{}
}

func preflightValidation(ctx context.Context, client *clients.ResourceClient, requestPlan *AzapiResourceModel, body map[string]interface{}) error {
Copy link
Member

Choose a reason for hiding this comment

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

Please use id parse.ResourceId instead of requestPlan *AzapiResourceModel, because the necessary information could be retrieved by the parse.ResourceId.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated. Thanks.

@@ -502,6 +502,15 @@ func (r *AzapiResource) CreateUpdate(ctx context.Context, requestPlan tfsdk.Plan
defer locks.UnlockByID(lockId)
}

// preflight validation: currently only supports the subscription scope validation
if r.ProviderData.Features.EnablePreflight && isNewResource && strings.Contains(strings.ToLower(plan.ParentID.ValueString()), "/subscriptions/") {
Copy link
Member

Choose a reason for hiding this comment

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

Could we use strings.HasPrefix() here as the /subscriptions/ must always be at the start?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated. Thanks.

@ms-henglu ms-henglu added this to the v1.14.0 milestone May 14, 2024
@ms-zhenhua ms-zhenhua requested a review from ms-henglu May 15, 2024 07:50
strings.EqualFold(arm.TenantResourceType.String(), resourceType) ||
strings.EqualFold("Microsoft.Management/managementGroups", resourceType)

return r.ProviderData.Features.EnablePreflight && isNewResource && isSupportedParentID
Copy link
Member

Choose a reason for hiding this comment

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

I think r.ProviderData.Features.EnablePreflight && isNewResource should be moved out from this function, because this function is about "whether preflight is supported on a give resource", once this is addressed, this PR should be good to go. Thanks!

@ms-henglu ms-henglu modified the milestones: v1.14.0, v1.15.0 Jul 4, 2024
@stemaMSFT stemaMSFT removed this from the v1.15.0 milestone Jul 23, 2024
@ms-zhenhua ms-zhenhua marked this pull request as draft July 24, 2024 02:45
return nil
}

if plan.Name.IsNull() || plan.Name.IsUnknown() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

use a random value for name if unknown

requestBody.Scope = parentId

if !plan.Location.IsNull() && !plan.Location.IsUnknown() {
requestBody.Location = plan.Location.ValueString()
Copy link
Collaborator Author

@ms-zhenhua ms-zhenhua Jul 24, 2024

Choose a reason for hiding this comment

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

if location does not exist, Preflight will skip the validation and return 200 ok without any error.

@ms-henglu ms-henglu added this to the v2.0.0 milestone Sep 3, 2024
@ms-henglu ms-henglu marked this pull request as ready for review September 4, 2024 06:29
@ms-henglu ms-henglu requested a review from magodo September 5, 2024 08:26
@ms-henglu ms-henglu merged commit c8014e7 into Azure:main Sep 5, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants