azurerm_api_management_api_schema - prevent plan not empty after apply for json definitions

[wiktorn]

Add support for Swagger based schema definitions.

Additional changes:

• add checks in test that verify, that state includes requested schema
• wait in resourceApiManagementApiSchemaCreateUpdate until resource is created to avoid race with following GET, which results in inconsistent state (resource created but not present in state)
• add suppress.JsonDiff for suppression of non-essential changes in JSON's

Fixes #12002.

Test results:

TF_ACC=1 go test -v ./azurerm/internal/services/apimanagement -run=TestAccApiManagementApiSchema -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/06/12 03:19:48 [DEBUG] not using binary driver name, it's no longer needed
2021/06/12 03:19:48 [WARN] could not locate a terraform executable on system path; continuing
2021/06/12 03:19:53 [DEBUG] not using binary driver name, it's no longer needed
2021/06/12 03:19:53 [WARN] could not locate a terraform executable on system path; continuing
=== RUN   TestAccApiManagementApiSchema_basic
=== PAUSE TestAccApiManagementApiSchema_basic
=== RUN   TestAccApiManagementApiSchema_basicSwagger
=== PAUSE TestAccApiManagementApiSchema_basicSwagger
=== RUN   TestAccApiManagementApiSchema_requiresImport
=== PAUSE TestAccApiManagementApiSchema_requiresImport
=== CONT  TestAccApiManagementApiSchema_basic
=== CONT  TestAccApiManagementApiSchema_requiresImport
=== CONT  TestAccApiManagementApiSchema_basicSwagger
--- PASS: TestAccApiManagementApiSchema_basicSwagger (3998.26s)
--- PASS: TestAccApiManagementApiSchema_requiresImport (4125.60s)
--- PASS: TestAccApiManagementApiSchema_basic (4404.69s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/apimanagement       4413.150s

[wiktorn]

I'd welcome any guidance on how to prepare test cases for that issue. It's not clear for me, how to test for specific value in nested structures currently.

[katbyte]

Looks like we have some test failures now:

------- Stdout: -------
=== RUN   TestAccApiManagementApiSchema_basic
=== PAUSE TestAccApiManagementApiSchema_basic
=== CONT  TestAccApiManagementApiSchema_basic
    testing.go:620: Step 1/2 error: Error running pre-apply refresh: exit status 1
        
        Error: Invalid function argument
        
          on terraform_plugin_test.tf line 38, in resource "azurerm_api_management_api_schema" "test":
          38:   value               = file("testdata/api_management_api_pluginsdk.xml")
        
        Invalid value for "path" parameter: no file exists at
        testdata/api_management_api_pluginsdk.xml; this function works only with
        files that are distributed as part of the configuration source code, so if
        this file will be created by a resource in this configuration you must
        instead obtain this result from an attribute of that resource.
--- FAIL: TestAccApiManagementApiSchema_basic (7.35s)
FAIL

------- Stderr: -------
2021/06/03 16:50:35 [DEBUG] not using binary driver name, it's no longer needed
2021/06/03 16:50:35 [DEBUG] not using binary driver name, it's no longer needed

[wiktorn]

@katbyte
missing testdata/api_management_api_pluginsdk.xml is a result of f989e9b.

It changes testdata/api_management_api_schema.xml to testdata/api_management_api_pluginsdk.xml but doesn't provide necessary file in the commit.

[wiktorn]

@tombuildsstuff I reviewed Azure docs and added necessary logic
@katbyte For now, I used as a base 46174df (parent commit of the one that introduced breaking change in the tests. This PR of course needs adaptation to sdkv2 shim. But the tests will start to fail and I'm not sure how they should be fixed. I'd love if you could point me out, how these tests can be fixed.

[wiktorn]

I see that the tests were fixed, I've rebased the changes to recent master.

[katbyte]

Thanks @wiktorn - looks like we have a different type of test failure now:

------- Stdout: -------
=== RUN   TestAccApiManagementApiSchema_basic
=== PAUSE TestAccApiManagementApiSchema_basic
=== CONT  TestAccApiManagementApiSchema_basic
    testing.go:620: Step 1/2 error: After applying this test step, the plan was not empty.
        stdout:
        
        
        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
        
        Terraform will perform the following actions:
        
          # azurerm_api_management_api_schema.test will be updated in-place
          ~ resource "azurerm_api_management_api_schema" "test" {
                id                  = "/subscriptions/*******/resourceGroups/acctestRG-210611210113978406/providers/Microsoft.ApiManagement/service/acctestAM-210611210113978406/apis/acctestapi-210611210113978406/schemas/acctestSchema210611210113978406"
              + value               = <<-EOT
                    <s:schema elementFormDefault=\"qualified\" targetNamespace=\"http://ws.cdyne.com/WeatherWS/\" xmlns:tns=\"http://ws.cdyne.com/WeatherWS/\" xmlns:s=\"http://www.w3.org/2001/XMLSchema\" xmlns:soap12=\"http://schemas.xmlsoap.org/wsdl/soap12/\" xmlns:mime=\"http://schemas.xmlsoap.org/wsdl/mime/\" xmlns:soap=\"http://schemas.xmlsoap.org/wsdl/soap/\" xmlns:tm=\"http://microsoft.com/wsdl/mime/textMatching/\" xmlns:http=\"http://schemas.xmlsoap.org/wsdl/http/\" xmlns:soapenc=\"http://schemas.xmlsoap.org/soap/encoding/\" xmlns:wsdl=\"http://schemas.xmlsoap.org/wsdl/\" xmlns:apim-wsdltns=\"http://ws.cdyne.com/WeatherWS/\">\r\n  <s:element name=\"GetWeatherInformation\">\r\n    <s:complexType />\r\n  </s:element>\r\n  <s:element name=\"GetWeatherInformationResponse\">\r\n    <s:complexType>\r\n      <s:sequence>\r\n        <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"GetWeatherInformationResult\" type=\"tns:ArrayOfWeatherDescription\" />\r\n      </s:sequence>\r\n    </s:complexType>\r\n  </s:element>\r\n  <s:complexType name=\"ArrayOfWeatherDescription\">\r\n    <s:sequence>\r\n      <s:element minOccurs=\"0\" maxOccurs=\"unbounded\" name=\"WeatherDescription\" type=\"tns:WeatherDescription\" />\r\n    </s:sequence>\r\n  </s:complexType>\r\n  <s:complexType name=\"WeatherDescription\">\r\n    <s:sequence>\r\n      <s:element minOccurs=\"1\" maxOccurs=\"1\" name=\"WeatherID\" type=\"s:short\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"Description\" type=\"s:string\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"PictureURL\" type=\"s:string\" />\r\n    </s:sequence>\r\n  </s:complexType>\r\n  <s:element name=\"GetCityForecastByZIP\">\r\n    <s:complexType>\r\n      <s:sequence>\r\n        <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"ZIP\" type=\"s:string\" />\r\n      </s:sequence>\r\n    </s:complexType>\r\n  </s:element>\r\n  <s:element name=\"GetCityForecastByZIPResponse\">\r\n    <s:complexType>\r\n      <s:sequence>\r\n        <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"GetCityForecastByZIPResult\" type=\"tns:ForecastReturn\" />\r\n      </s:sequence>\r\n    </s:complexType>\r\n  </s:element>\r\n  <s:complexType name=\"ForecastReturn\">\r\n    <s:sequence>\r\n      <s:element minOccurs=\"1\" maxOccurs=\"1\" name=\"Success\" type=\"s:boolean\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"ResponseText\" type=\"s:string\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"State\" type=\"s:string\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"City\" type=\"s:string\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"WeatherStationCity\" type=\"s:string\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"ForecastResult\" type=\"tns:ArrayOfForecast\" />\r\n    </s:sequence>\r\n  </s:complexType>\r\n  <s:complexType name=\"ArrayOfForecast\">\r\n    <s:sequence>\r\n      <s:element minOccurs=\"0\" maxOccurs=\"unbounded\" name=\"Forecast\" nillable=\"true\" type=\"tns:Forecast\" />\r\n    </s:sequence>\r\n  </s:complexType>\r\n  <s:complexType name=\"Forecast\">\r\n    <s:sequence>\r\n      <s:element minOccurs=\"1\" maxOccurs=\"1\" name=\"Date\" type=\"s:dateTime\" />\r\n      <s:element minOccurs=\"1\" maxOccurs=\"1\" name=\"WeatherID\" type=\"s:short\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"Desciption\" type=\"s:string\" />\r\n      <s:element minOccurs=\"1\" maxOccurs=\"1\" name=\"Temperatures\" type=\"tns:temp\" />\r\n      <s:element minOccurs=\"1\" maxOccurs=\"1\" name=\"ProbabilityOfPrecipiation\" type=\"tns:POP\" />\r\n    </s:sequence>\r\n  </s:complexType>\r\n  <s:complexType name=\"temp\">\r\n    <s:sequence>\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"MorningLow\" type=\"s:string\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"DaytimeHigh\" type=\"s:string\" />\r\n    </s:sequence>\r\n  </s:complexType>\r\n  <s:complexType name=\"POP\">\r\n    <s:sequence>\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"Nighttime\" type=\"s:string\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"Daytime\" type=\"s:string\" />\r\n    </s:sequence>\r\n  </s:complexType>\r\n  <s:element name=\"GetCityWeatherByZIP\">\r\n    <s:complexType>\r\n      <s:sequence>\r\n        <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"ZIP\" type=\"s:string\" />\r\n      </s:sequence>\r\n    </s:complexType>\r\n  </s:element>\r\n  <s:element name=\"GetCityWeatherByZIPResponse\">\r\n    <s:complexType>\r\n      <s:sequence>\r\n        <s:element minOccurs=\"1\" maxOccurs=\"1\" name=\"GetCityWeatherByZIPResult\" type=\"tns:WeatherReturn\" />\r\n      </s:sequence>\r\n    </s:complexType>\r\n  </s:element>\r\n  <s:complexType name=\"WeatherReturn\">\r\n    <s:sequence>\r\n      <s:element minOccurs=\"1\" maxOccurs=\"1\" name=\"Success\" type=\"s:boolean\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"ResponseText\" type=\"s:string\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"State\" type=\"s:string\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"City\" type=\"s:string\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"WeatherStationCity\" type=\"s:string\" />\r\n      <s:element minOccurs=\"1\" maxOccurs=\"1\" name=\"WeatherID\" type=\"s:short\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"Description\" type=\"s:string\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"Temperature\" type=\"s:string\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"RelativeHumidity\" type=\"s:string\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"Wind\" type=\"s:string\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"Pressure\" type=\"s:string\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"Visibility\" type=\"s:string\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"WindChill\" type=\"s:string\" />\r\n      <s:element minOccurs=\"0\" maxOccurs=\"1\" name=\"Remarks\" type=\"s:string\" />\r\n    </s:sequence>\r\n  </s:complexType>\r\n  <s:element name=\"ArrayOfWeatherDescription\" nillable=\"true\" type=\"tns:ArrayOfWeatherDescription\" />\r\n  <s:element name=\"ForecastReturn\" nillable=\"true\" type=\"tns:ForecastReturn\" />\r\n  <s:element name=\"WeatherReturn\" type=\"tns:WeatherReturn\" />\r\n</s:schema>
                EOT
                # (5 unchanged attributes hidden)
            }
        
        Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccApiManagementApiSchema_basic (4026.44s)
FAIL

------- Stderr: -------
2021/06/11 21:01:13 [DEBUG] not using binary driver name, it's no longer needed
2021/06/11 21:01:13 [DEBUG] not using binary driver name, it's no longer needed

[wiktorn]

Sorry, linting fixes broke the acceptance tests and I didn't rerun the tests (my bad).

TF_ACC=1 go test -v ./azurerm/internal/services/apimanagement -run=TestAccApiManagementApiSchema -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/06/16 13:43:25 [DEBUG] not using binary driver name, it's no longer needed
2021/06/16 13:43:25 [WARN] could not locate a terraform executable on system path; continuing
2021/06/16 13:43:29 [DEBUG] not using binary driver name, it's no longer needed
2021/06/16 13:43:29 [WARN] could not locate a terraform executable on system path; continuing
=== RUN   TestAccApiManagementApiSchema_basic
=== PAUSE TestAccApiManagementApiSchema_basic
=== RUN   TestAccApiManagementApiSchema_basicSwagger
=== PAUSE TestAccApiManagementApiSchema_basicSwagger
=== RUN   TestAccApiManagementApiSchema_requiresImport
=== PAUSE TestAccApiManagementApiSchema_requiresImport
=== CONT  TestAccApiManagementApiSchema_basic
=== CONT  TestAccApiManagementApiSchema_requiresImport
=== CONT  TestAccApiManagementApiSchema_basicSwagger
--- PASS: TestAccApiManagementApiSchema_basicSwagger (4114.53s)
--- PASS: TestAccApiManagementApiSchema_basic (4116.91s)
--- PASS: TestAccApiManagementApiSchema_requiresImport (4370.86s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/apimanagement       4378.512s

[wiktorn]

@katbyte And one more idea come to my mind, as in this area - there Consumption based instance is enough to test, maybe it's worth to use it? It reduces dramatically time needed to run the tests. In my recent run it took 267 seconds with Consumption tier compared to 4378 seconds with Developer tier.

The only drawback is, that you need more comprehensive knowledge about specific service to know, when Consumption tier is enough and when you need to use Developer tier.

Are such optimizations welcomed?

[katbyte]

@wiktorn - i'm not very familiar with the api management service, what is the difference between the two tiers and would it compromise the test integrity? if it is still testing the exact same thing optimizations are always welcome 🙂

(looks like i had this in an open tab and forgotten to hit comment! 😅 )

[wiktorn]

@wiktorn - i'm not very familiar with the api management service, what is the difference between the two tiers and would it compromise the test integrity? if it is still testing the exact same thing optimizations are always welcome 🙂

So the difference is, that Consumption_0 uses shared instance (hence quick provisioning) and Developer_1 means using dedicated instance. In case of shared instance, there are some limitation what can be configured. To the list of unsupported features belong:

• assigning to dedicated VNET
• developer portal
• external cache configuration
• custom domain names

And if I remember correctly, the IP address of the instance is differently returned. And I'm not sure, if you can set your own CA certificates.

The plan then would be, to use the Consumption_0 in these places, where functionality is supported and fallback to Developer_1 when not. This would improve time on most of the tests, leaving few that will wait for provisioning of dedicated instance.

[katbyte]

oh 100% if Consumption_0 works i'm all for it! APIM tests always take a long time to run during our nightly CI - but we can do that in a separate PR so i can get this merged for todays release 🙂

[katbyte]

Thanks @wiktorn - LGTM and provided tests pass will merge shortly 🚀

[github-actions]

This functionality has been released in v2.64.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

[github-actions]

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.