-
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
apimanagement : refactoring to use hashicorp/go-azure-sdk #22783
apimanagement : refactoring to use hashicorp/go-azure-sdk #22783
Conversation
a50ec02
to
8cab70f
Compare
8cab70f
to
15684a6
Compare
15684a6
to
c517ad6
Compare
@sinbai heads up I'm looking into the base layer issues the tests have picked up here:
|
Hi @tombuildsstuff thanks for letting me know about this. After updating this line to the following code,the test case
|
Most of API Management returns a 202 with a self reference containing an `asynctoken`, as such if a self-reference exists then we can use this URI directly. Fixes: ``` Error: creating/updating Api (Subscription: "*******" Resource Group Name: "acctestRG-230803055317928116" Service Name: "acctestAM-230803055317928116" Api: "acctestAMA-230803055317928116"): performing CreateOrUpdate: no applicable pollers were found for the response ``` From hashicorp/terraform-provider-azurerm#22783
@sinbai indeed, that's the same fix I've just pushed a commit to fix :) |
Most of API Management returns a 202 with a self reference containing an `asynctoken`, as such if a self-reference exists then we can use this URI directly. Fixes: ``` Error: creating/updating Api (Subscription: "*******" Resource Group Name: "acctestRG-230803055317928116" Service Name: "acctestAM-230803055317928116" Api: "acctestAMA-230803055317928116"): performing CreateOrUpdate: no applicable pollers were found for the response ``` From hashicorp/terraform-provider-azurerm#22783
This comment was marked as outdated.
This comment was marked as outdated.
@sinbai since the fix for this has been merged into |
c517ad6
to
c452244
Compare
Certainly, done:). |
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
Looks like there are quite a few new test failures in TC though
84b0ba5
to
c393bf8
Compare
"strconv" | ||
"strings" | ||
"time" | ||
|
||
"github.com/Azure/azure-sdk-for-go/services/apimanagement/mgmt/2021-08-01/apimanagement" // nolint: staticcheck | ||
"github.com/Azure/go-autorest/autorest" |
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.
hashicorp/go-azure-sdk
no longer uses Azure/go-autorest
for APIManagement - so we can't rely on casting errors using it - can we just check the Status Codes being returned instead?
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.
@tombuildsstuff thanks for your feedback:). The code has been updated.
@stephybun thanks for your feedback. I have fixed some issues and the code has been updated. Also, I started a new test on Teamcity as follows : https://hashicorp.teamcity.com/buildConfiguration/TF_AzureRM_AZURERM_SERVICE_PUBLIC_APIMANAGEMENT/8397?hideProblemsFromDependencies=false&hideTestsFromDependencies=false. |
Hi @stephybun , given the test results in the PR comments, I assume this PR is ready for review, what do you think? |
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. I have one comment around assigning the response Model and Properties to variables within the read. We do this for readability, so that we remain consistent could you please update those in all the read methods of the resources/data sources that have been migrated? (I've only pointed out two in-line)
internal/services/apimanagement/api_management_api_data_source.go
Outdated
Show resolved
Hide resolved
internal/services/apimanagement/api_management_api_diagnostic_resource.go
Outdated
Show resolved
Hide resolved
@stephybun thanks for your feedback. Code has been updated. 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 making those changes to bring this in line with the other resources @sinbai. LGTM 🥟
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Test results: There are 8 test cases failed in this test. However, all of them have passed after re-running. See below for details:
https://hashicorp.teamcity.com/buildConfiguration/TF_AzureRM_AZURERM_SERVICE_PUBLIC_APIMANAGEMENT/9967?hideProblemsFromDependencies=false&hideTestsFromDependencies=false&expandBuildTestsSection=true&expandBuildChangesSection=true
https://hashicorp.teamcity.com/buildConfiguration/TF_AzureRM_AZURERM_SERVICE_PUBLIC_APIMANAGEMENT/10094?buildTab=tests