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

sdk: provisioning state poller allow additional parameters #1090

Closed

Conversation

wuxu92
Copy link
Contributor

@wuxu92 wuxu92 commented Oct 9, 2024

This PR is to fix issues that provisioning state poller may need additional query parameters to request the resource.

Possibly unblocks #962, hashicorp/terraform-provider-azurerm#23322 and hashicorp/terraform-provider-azurerm#24379

Issue description

For the APIM API resource, the response to a create/update operation includes a location header with an asyncId parameter. This asyncId is needed to retrieve the actual error message if the create/update operation fails.

image

Create

If missing the asyncId parameter, the GET operation after a failed creation will return 404 Not Found, obscuring the actual ValidationError.

image

When this PR is applied, the error message will reflect the root cause as follows:

image

Update

The GET operation after a failed update will return 200 OK, mistakenly indicating the update was successful. More details can be found at hashicorp/terraform-provider-azurerm#24379.

When this PR is applied, the update operation will report the error if the swagger contains validation error:

image

@wuxu92 wuxu92 requested a review from a team as a code owner October 9, 2024 08:37
@github-actions github-actions bot added the release-once-merged The SDK should be released once this PR is merged label Oct 9, 2024
@sinbai
Copy link
Contributor

sinbai commented Oct 9, 2024

I have tested this PR, it fixes the issue mentioned in #23322 (the case where the imported file contains errors), terraform returns the error information returned by the API instead of a 404 error. Details are as follows.

Before fix:
image

After fix:
image

@jackofallops
Copy link
Member

Hi @wuxu92 - Are there other known provisioning cases requiring this modification? I'm just wondering why we're making the change here, rather than creating a custom poller for this as we have done for other unique cases?

@wuxu92
Copy link
Contributor Author

wuxu92 commented Nov 14, 2024

@jackofallops Currently, only this API requires additional parameters during provisioning. I prefer fixing it in the SDK layer rather than using a custom poller in the provider code, as this will allow us to provide a robust SDK and reduce custom logic in the provider.

@wuxu92
Copy link
Contributor Author

wuxu92 commented Nov 25, 2024

@jackofallops According to this comment, the delete API of the APIM API might face the same issue with the LRO polling URI of the asyncId parameter in future API versions.

@wuxu92
Copy link
Contributor Author

wuxu92 commented Dec 4, 2024

@jackofallops Since we can't pass additional parameters to the SDK's Get method, a custom poller doesn't seem possible for this case. Do you have any suggestions? Maybe I should build a raw *client.Request and execute with it in the provider, but I'm not sure if it's good practice.

https://github.com/hashicorp/terraform-provider-azurerm/blob/eb2d34534162d50ba71806fcded9110cc1cbbcfd/vendor/github.com/hashicorp/go-azure-sdk/resource-manager/apimanagement/2022-08-01/api/method_get.go#L21

@wuxu92
Copy link
Contributor Author

wuxu92 commented Dec 6, 2024

I created customr poller PR hashicorp/terraform-provider-azurerm#28193

@jackofallops
Copy link
Member

I created customr poller PR hashicorp/terraform-provider-azurerm#28193

Thanks @wuxu92 - I've reviewed the linked PR, and since we have away forward there I'm going to close this one out for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-once-merged The SDK should be released once this PR is merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants