`azurerm_api_management_api`: split create and update method #28271

wuxu92 · 2024-12-13T08:30:06Z

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

Split Update method from CreateUpdate method, and an update test for it.

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”

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: TestAccApiManagementApi_completeUpdate (252.61s)
PASS

mbfrahry

Hey @wuxu92, this PR is nearly there but there is an issue with the update because we're using a new payload instead of the existing one. I've left some comments below but happy to answer any questions

mbfrahry · 2024-12-30T19:39:42Z

internal/services/apimanagement/api_management_api_resource.go


 	// If import is used, we need to send properties to Azure API in two operations.
 	// First we execute import and then updated the other props.
-	if vs, hasImport := d.GetOk("import"); hasImport {
-		importVs := vs.([]interface{})
+	if importVs := d.Get("import").([]interface{}); len(importVs) > 0 {


We should confirm we can cast to .([]interface{}) to prevent a potential panic

Suggested change

if importVs := d.Get("import").([]interface{}); len(importVs) > 0 {

if importVs, ok := d.Get("import").([]interface{}); ok && len(importVs) > 0 {

Get will return the zero value of the schema if the field is not set. Anyway, I'd add the check here to make it clear.

terraform-provider-azurerm/vendor/github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema/resource_data.go

Lines 565 to 567 in a74f82f

if result.Value == nil && schema != nil {

result.Value = result.ValueOrZero(schema)

}

mbfrahry · 2024-12-30T20:35:26Z

internal/services/apimanagement/api_management_api_resource.go

+		}
+	}
+
+	prop := &api.ApiCreateOrUpdateProperties{


TestAccApiManagementApi_importUpdate is failing because we're building a new payload rather than using the existing one.

Right, I've updated the Update method with existing payload and the acctest updated. some fields can be set by tf configuration or by import block. so I added an ignore_changes to this acc test.

--- PASS: TestAccApiManagementApi_importUpdate (336.19s) PASS```

Do we have to set every variable in a new struct or could we just use existing and change any values through d.HasChange?

The data type of existing is ApiContractProperties which is not the same as ApiCreateOrUpdateProperties. so a copy is required.

Ahhh shoot. Thanks for the added context!

mbfrahry · 2024-12-30T20:40:58Z

internal/services/apimanagement/api_management_api_resource_test.go

+  path                = "butter-parser-update"
+  protocols           = ["https"]
+  revision            = "3"
+  description         = "What is my purpose? You parse butter update."


Because this is changing every updatable attribute, we'll miss a bug in the current update method. I encourage you to keep this value the same and run this test again to see what happens

As mentioned, the update method is now working with the existing payload and has been tested successfully.

--- PASS: TestAccApiManagementApi_completeUpdate (389.37s) PASS

wuxu92 · 2025-01-28T01:14:56Z

@mbfrahry Could you review this PR again? These comments should have been resolved.

mbfrahry

Thanks for your patience on this review. There are some things that won't work quite right the way it's written that I've documented. Let me know if you have any questions

mbfrahry · 2025-01-16T18:47:09Z

internal/services/apimanagement/api_management_api_resource.go

+		return fmt.Errorf("creating/updating %s: %+v", *id, err)
+	}
+
+	d.SetId(id.ID())


We don't need to reset the ID after we update as it should be the same ID as when we set it during Create. This could cause issues if the IDs between Create and Update aren't the same for whatever reason

mbfrahry · 2025-01-16T18:48:28Z

internal/services/apimanagement/api_management_api_resource.go

+}
+
+func expandApiManagementApiImport(importVs []interface{}, apiType api.ApiType, soapApiType api.SoapApiType, path, serviceUrl, version, versionSetId string) api.ApiCreateOrUpdateParameter {
+	importV := importVs[0].(map[string]interface{})


There aren't any checks that len(importVs) isn't 0 or that importVs[0] isn't nil. We'll want to add those to prevent a potential crash

updated. also update the create method with this expand method.

mbfrahry · 2025-01-16T18:48:54Z

internal/services/apimanagement/api_management_api_resource.go

+
+	wsdlSelectorVs := importV["wsdl_selector"].([]interface{})
+	if len(wsdlSelectorVs) > 0 {
+		wsdlSelectorV := wsdlSelectorVs[0].(map[string]interface{})


And here, we want to check that wsdlSelectorVs[0] isn't nil

mbfrahry · 2025-01-16T18:54:47Z

internal/services/apimanagement/api_management_api_resource.go

+	}
+
+	if d.HasChange("version_set_id") {
+		prop.ApiVersionSetId = &versionSetId


Suggested change

prop.ApiVersionSetId = &versionSetId

prop.ApiVersionSetId = pointer.To(versionSetId)

mbfrahry · 2025-02-13T22:15:52Z

internal/services/apimanagement/api_management_api_resource.go

+	sourceApiId := d.Get("source_api_id").(string)
+	serviceUrl := d.Get("service_url").(string)
+
+	if version != "" && versionSetId == "" {


We can reduce code duplication and get the following checks checked at plan time rather than apply time by using doing CustomizeDiff

updated.

Plan: 2 to add, 0 to change, 1 to destroy. │ Error: 1 error occurred: │ * setting `version` without the required `version_set_id` │ Plan: 2 to add, 0 to change, 1 to destroy. ╷ │ Error: 1 error occurred: │ * `display_name`, `protocols` are required when `source_api_id` is not set

mbfrahry · 2025-02-13T22:18:30Z

internal/services/apimanagement/api_management_api_resource.go

+		subscriptionKeyParameterNamesRaw := d.Get("subscription_key_parameter_names").([]interface{})
+		prop.SubscriptionKeyParameterNames = expandApiManagementApiSubscriptionKeyParamNames(subscriptionKeyParameterNamesRaw)


Suggested change

subscriptionKeyParameterNamesRaw := d.Get("subscription_key_parameter_names").([]interface{})

prop.SubscriptionKeyParameterNames = expandApiManagementApiSubscriptionKeyParamNames(subscriptionKeyParameterNamesRaw)

prop.SubscriptionKeyParameterNames = expandApiManagementApiSubscriptionKeyParamNames(d.Get("subscription_key_parameter_names").([]interface{}))

mbfrahry · 2025-02-13T22:21:10Z

internal/services/apimanagement/api_management_api_resource.go

+		oAuth2AuthorizationSettingsRaw := d.Get("oauth2_authorization").([]interface{})
+		oAuth2AuthorizationSettings := expandApiManagementOAuth2AuthenticationSettingsContract(oAuth2AuthorizationSettingsRaw)
+		authenticationSettings.OAuth2 = oAuth2AuthorizationSettings
+		prop.AuthenticationSettings = authenticationSettings


The way this is written will reset openid_authentication if it doesn't change as it's setting oauth2_authorization into an empty AuthenticationSettingsContract

We should modify the existing AuthenticationSettings rather than creating a new one

Right. Updated.

mbfrahry · 2025-02-13T22:21:57Z

internal/services/apimanagement/api_management_api_resource.go

+		openIDAuthorizationSettingsRaw := d.Get("openid_authentication").([]interface{})
+		openIDAuthorizationSettings := expandApiManagementOpenIDAuthenticationSettingsContract(openIDAuthorizationSettingsRaw)
+		authenticationSettings.Openid = openIDAuthorizationSettings
+		prop.AuthenticationSettings = authenticationSettings


And the vice versa, this will reset oauth2_authorization if oauth2_authorization hasn't changed

We should modify the existing AuthenticationSettings rather than creating a new one

updated with above

mbfrahry · 2025-02-13T22:22:09Z

internal/services/apimanagement/api_management_api_resource.go

+		contactInfoRaw := d.Get("contact").([]interface{})
+		prop.Contact = expandApiManagementApiContact(contactInfoRaw)


Suggested change

contactInfoRaw := d.Get("contact").([]interface{})

prop.Contact = expandApiManagementApiContact(contactInfoRaw)

prop.Contact = expandApiManagementApiContact(d.Get("contact").([]interface{}))

mbfrahry · 2025-02-13T22:22:19Z

internal/services/apimanagement/api_management_api_resource.go

+		licenseInfoRaw := d.Get("license").([]interface{})
+		prop.License = expandApiManagementApiLicense(licenseInfoRaw)


Suggested change

licenseInfoRaw := d.Get("license").([]interface{})

prop.License = expandApiManagementApiLicense(licenseInfoRaw)

prop.License = expandApiManagementApiLicense(d.Get("license").([]interface{}))

wuxu92 · 2025-02-14T02:09:02Z

@mbfrahry Thanks for the comments! I updated the branch with the main and fixed the corresponding code.

mbfrahry

Hey @wuxu92, thanks for taking the time to go over my review. I left a couple more comments

mbfrahry · 2025-02-14T21:29:40Z

internal/services/apimanagement/api_management_api_resource.go

+		ApiVersion:                    existing.ApiVersion,
+		ApiVersionSetId:               existing.ApiVersionSetId,
+		TermsOfServiceURL:             existing.TermsOfServiceURL,
+		Type:                          existing.Type,


It does look like ApiType didn't get copied over. I just want to make sure that was intentional

mbfrahry · 2025-02-14T22:14:47Z

internal/services/apimanagement/api_management_api_resource.go

+
+		CustomizeDiff: pluginsdk.CustomDiffWithAll(
+			pluginsdk.CustomizeDiffShim(func(ctx context.Context, d *pluginsdk.ResourceDiff, v interface{}) error {
+				if d.Get("version").(string) != "" && d.Get("version_set_id").(string) == "" {


Looks like you saw the errors around

* setting `version` without the required `version_set_id`

That's because version_set_id is interpolated with azurerm_api_management_api_version_set.test.id and Terraform doesn't know what an interpolated value is at plan time so it marks it as an empty string.

What you can instead do is use d.GetRawConfig().AsValueMap()["version_set_id"].IsNull() which Terraform can look to see if it's interpolated or not.

You can make similar changes to the check below as well

wuxu92 requested a review from a team as a code owner December 13, 2024 08:30

github-actions bot added service/api-management size/XL labels Dec 13, 2024

wuxu92 mentioned this pull request Dec 13, 2024

azurerm_api_management_api: add costom poller for import openapi #28193

Open

14 tasks

mbfrahry requested changes Dec 30, 2024

View reviewed changes

github-actions bot added waiting-response documentation and removed waiting-response labels Dec 30, 2024

mbfrahry requested changes Feb 13, 2025

View reviewed changes

github-actions bot assigned mbfrahry Feb 13, 2025

github-actions bot added the waiting-response label Feb 13, 2025

wuxu92 added 5 commits February 14, 2025 08:44

split create and update method of apim/api

3ee82a1

parse id from state in update method

55b8eeb

code format

1acd8d3

use existing props for update and update acctest

cd58ce6

code linter and customize diff

4a2c639

wuxu92 force-pushed the apim/split-update branch from 84bb9e9 to 4a2c639 Compare February 14, 2025 02:03

github-actions bot removed the waiting-response label Feb 14, 2025

github-actions bot added the waiting-response label Feb 14, 2025

code lint

ad0b045

github-actions bot added waiting-response and removed waiting-response labels Feb 14, 2025

mbfrahry requested changes Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_api_management_api`: split create and update method #28271

`azurerm_api_management_api`: split create and update method #28271

wuxu92 commented Dec 13, 2024 •

edited

Loading

mbfrahry left a comment

mbfrahry Dec 30, 2024

wuxu92 Dec 31, 2024

mbfrahry Dec 30, 2024

wuxu92 Dec 31, 2024 •

edited

Loading

mbfrahry Feb 13, 2025

wuxu92 Feb 14, 2025

mbfrahry Feb 14, 2025

mbfrahry Dec 30, 2024

wuxu92 Dec 31, 2024

wuxu92 commented Jan 28, 2025 •

edited

Loading

mbfrahry left a comment

mbfrahry Jan 16, 2025

mbfrahry Jan 16, 2025

wuxu92 Feb 14, 2025

mbfrahry Jan 16, 2025

mbfrahry Jan 16, 2025

mbfrahry Feb 13, 2025

wuxu92 Feb 14, 2025

mbfrahry Feb 13, 2025

wuxu92 Feb 14, 2025

mbfrahry Feb 13, 2025

wuxu92 Feb 14, 2025

mbfrahry Feb 13, 2025

wuxu92 Feb 14, 2025

mbfrahry Feb 13, 2025

wuxu92 Feb 14, 2025

mbfrahry Feb 13, 2025

wuxu92 Feb 14, 2025

wuxu92 commented Feb 14, 2025

mbfrahry left a comment

mbfrahry Feb 14, 2025

mbfrahry Feb 14, 2025

	if importVs := d.Get("import").([]interface{}); len(importVs) > 0 {
	if importVs, ok := d.Get("import").([]interface{}); ok && len(importVs) > 0 {

	if result.Value == nil && schema != nil {
	result.Value = result.ValueOrZero(schema)
	}

	prop.ApiVersionSetId = &versionSetId
	prop.ApiVersionSetId = pointer.To(versionSetId)

		subscriptionKeyParameterNamesRaw := d.Get("subscription_key_parameter_names").([]interface{})
		prop.SubscriptionKeyParameterNames = expandApiManagementApiSubscriptionKeyParamNames(subscriptionKeyParameterNamesRaw)

		contactInfoRaw := d.Get("contact").([]interface{})
		prop.Contact = expandApiManagementApiContact(contactInfoRaw)

		licenseInfoRaw := d.Get("license").([]interface{})
		prop.License = expandApiManagementApiLicense(licenseInfoRaw)

azurerm_api_management_api: split create and update method #28271

Are you sure you want to change the base?

azurerm_api_management_api: split create and update method #28271

Conversation

wuxu92 commented Dec 13, 2024 • edited Loading

Community Note

Description

PR Checklist

Testing

mbfrahry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuxu92 Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuxu92 commented Jan 28, 2025 • edited Loading

mbfrahry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuxu92 commented Feb 14, 2025

mbfrahry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`azurerm_api_management_api`: split create and update method #28271

`azurerm_api_management_api`: split create and update method #28271

wuxu92 commented Dec 13, 2024 •

edited

Loading

wuxu92 Dec 31, 2024 •

edited

Loading

wuxu92 commented Jan 28, 2025 •

edited

Loading